deno_std: bug(fs): fs.exists/fs.existsSync throws when querying subpath of existing file.

I’ve never seen actually valid uses of fs.exists(), either in Deno or in Node, so I’m 100% in favor of deprecating and removing it.

Let me enumerate what its flaws are:

1. TOCTOU bugs and race conditions. Don’t check if a file exists, then try to open it - open the file and handle “not found” errors.

2. “Exists” is a nebulous concept. node -p 'fs.existsSync("/root/.ssh/id_rsa")' prints false even though the file exists on my system. It’s there, it’s just not accessible.

3. “Accessible” is a nebulous concept. Often impossible to tell without actually opening the file (because of ACLs, LSMs, etc.) - so just open it already.

Go through https://github.com/nodejs/node/issues/1592 if you have 30 minutes to spare and observe how pretty much all the people arguing for un-deprecation manage to screw up their examples. Peak facepalm!

Hi.

I feel that “making fs.exists() deprecated” is too aggressive. You all talking about file operations after fs.exists() and some race condition caused by that but sometime there is a situation that just want to know if a file or directory exist. In that case, I anyway need to use fs.exists() and deal deprecation warning messages always.

For example, I need to check presence of a .git directory to roughly found a git working tree root directory. I will spawn git rev-parse or whatever after that so the first detection can be rough. This rough step is required because spawning a process on Windows is cost a bit. In this case, I don’t perform any further file operations on Deno itself so “race condition” is not good reason to me for making fs.exists() deprecated (because git process would handle that situation outside of Deno).

So please re-consider this deprecation again.

P.S.

I totally agree that documenting “file operations after fs.exists() may cause some race condition” on documentation of fs.exists() but I believe that documentation and deprecation is different and should be distinctive.

I would like to know: Can you confirm exists being a pitfall for people that want to check if a file exists, who end up checking for its existence before calling lstat to verify it’s a file and not a folder? How would you approach this problem?

Of course, people write problematic code from time to time, myself included, but I don’t understand why exists is the only victim. For example, I don’t think anyone would deprecate delay() even if the following code cause TOCTOU issues.

const info = await Deno.lstat("hello.txt");

// Something...
await delay(1000);

if (info.isFile) {
  // Maybe `hello.txt` become a directory at this point.
  await Deno.run(["rm", "-rf", "hello.txt"]).output();
}

Do you also think the deprecation was wrong in node.js for the same reasons you are clarifying?

I think fs.exists() in node.js is deprecated because of inconsistencies in the callback design, not because of TOCTOU issues, according to the node.js documentation

The parameters for this callback are not consistent with other Node.js callbacks. Normally, the first parameter to a Node.js callback is an err parameter, optionally followed by other parameters. The fs.exists() callback has only one boolean parameter. This is one reason fs.access() is recommended instead of fs.exists().

In addition, the above link also stated the following

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile() or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file’s state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.

I think this paragraph is the correct way to alert users that improper use of the function can cause TOCTOU issues. In my opinion, there should be a distinction between documentation and deprecation.

If it comes back, it should be named access instead of exists, because it will return false, if there are no permissions to read the file/folder.

I totally agree with this.

Just saying exists and lstat is the same won’t get this anywhere, because it is not.

If lstat is used to check whether a file or directory exists, then I think it is the same.

Btw, your final MERGE_HEAD example would require you to use lstat, because in that case it is relevant if you have a file or folder, again. Unless you love the wild west of course.

Since git itself does not care whether it is a file or a directory, in order to mimic it’s behavior, it must not care wheter it is a file or a directory.

(I know it’s a little picky, who would touch the .git contents, right?)

People like me would touch the contents in .git directory to make git related tools.

It doesn’t matter if you do something with the folder or not. Another process in the background could do something with it between your check and your action.

Yes. That’s why I’m trying to say “race condition” is not the matter what Deno library should care.

So why not using lstat? You have to check if .git is a folder anyways. I don’t think you should ignore the case of finding a .git file instead. Remember, the user will cause anything to break what can be broken, always.

It’s just an example case so I don’t dig it deeper but I don’t check if .git is a folder because that’s git’s job (sometime .git is NOT folder but a file that points a bare repository path)

Additionally, using lstat means that I need to write a code like

export async function exists(filePath: string): Promise<boolean> {
  try {
    await Deno.lstat(filePath);
    return true;
  } catch (err) {
    if (err instanceof Deno.errors.NotFound) {
      return false;
    }

    throw err;
  }
}

And this code is already exist in deno_std/fs/exists.ts right? So I anyway need to use this deprecated code so I don’t think using lstat solves the situation.

The “create file only when not already there” case would benefit from an { exclusive: true } flag that maps to std::fs::OpenOptions::create_new(true).open(path) a.k.a. open(path, O_CREAT + O_EXCL).

@bnoordhuis @kt3k A valid use-case is to check if a specific file exists that a third-party binary needs before starting that binary eventually. node introduced fs.accessSync() for that case.

@lambdalisue

In this case, I don’t perform any further file operations on Deno itself so “race condition” is not good reason to me for making fs.exists() deprecated

It doesn’t matter if you do something with the folder or not. Another process in the background could do something with it between your check and your action.

For example, I need to check presence of a .git directory to roughly found a git working tree root directory. I will spawn git rev-parse or whatever after that so the first detection can be rough. This rough step is required because spawning a process on Windows is cost a bit.

So why not using lstat? You have to check if .git is a folder anyways. I don’t think you should ignore the case of finding a .git file instead. Remember, the user will cause anything to break what can be broken, always.

Which maps directly to Deno.open("filepath", { createNew: true });, see https://doc.deno.land/deno/stable/~/Deno.OpenOptions for details

@dev-nicolaos Now I got what you mean, you mean the case: Create file X with content Y if it does not exist

Using exists or even lstat will cause a race condition. The node.js docs also deprecated exists for the same reason.. stats has the same recommendation, btw.

Using fs.exists or fs.stat to check for the existence of a file before calling fs.open(), fs.readFile() or fs.writeFile() is not recommended. Instead, user code should open/read/write the file directly and handle the error raised if the file is not available.

So use neither of those methods. Instead open the file using Deno.open and read its contents, so you have a proper file handler that won’t change if the file is messed around outside of your program. If the file size is 0, you know that the file is empty and should be populated with your content.

It also seems that you still don’t acknowledge the potential issue of having a directory instead of a file at your given path in your example. If you would use exists in the way you want to use it, you have to end up with this to address this as well:

const x = "myfile.txt"
const y = "Hello World"
if(fs.exists(x)) { // first race condition
	const stat = await Deno.lstat(x) // redundancy + 2nd race condition
	if(stat.isDirectory) {
		console.error("File does not exist, but path is blocked by folder")
	}
} else {
	await Deno.writeTextFile(x, y)
}

(untested, probably bugged, but you get the idea)

This code above is terrible. Deprecating exists will remove one potential race condition problem. Deno.lstat solves two problems at once: It’s checking for a valid file system entry and it’s giving your the type (file or folder) you are messing with. It’s still not a good approach for your case, please read on.

Using Deno.open your problem can be solved with something like this:

const x = "myfile.txt"
const y = "Hello World"
const encoder = new TextDecoder("utf-8") // to write the file contents
const file = await Deno.open(x, { read: true, write: true, create: true }) // "create" will only create a new file when it does not exist
const stat = await file.stat()
if(stat.isDirectory) {
	console.error("Path is blocked by a folder.")
} else if(stat.size == 0) { // file is empty?
	await file.write(encoder.encode(y))
}
file.close()

(untested, probably bugged, but you get the idea)

This has exactly 0 race conditions / TOCTOU bugs. As soon as you open the file, its handler stays consistent. It should be save to move the file around on your system while the code runs. It also enables you to handle the “directory, not file” case. All that for just one line more code, compared to the full exists example I posted above.

@kt3k @bnoordhuis @piscisaureus @ry I think we all would really welcome to have a flag on Deno.writeTextFile (or something like that) that abstracts the above case to only write the file if it does not exist. The code above is really a little too much for that common scenario, what do you think about that?

I’ve come across another possible use case for this API, a templating helper script (like you might find in some framework CLIs) with this logic:

1. If no file exists at the specified path, create file X at that location
2. If a file already exists at the specified path, the script should not create file X

The advice given above…

Don’t check if a file exists, then try to open it - open the file and handle “not found” errors.

…assumes there is some action that the script wants to perform on the file, but in this case there is nothing to do with the file if it already exists. The script just needs to know that its not there. Sure, you can try Deno.lStat or Deno.readFile and then perform the write after catching Deno.errors.NotFound, but then you’ve just implemented an uglier version of exists. This usage technically can fall prey to the TOCTOU issue, but in the real world it seems basically impossible for it to come up in this scenario (a developer runs this script to generate some boilerplate files).

Alternatively (and this might be better approach), options could be added to Deno.WriteFileOptions to account for this circumstance. WriteFileOptions.create doesn’t work help here because setting it to false means point 1 above will fail and setting it to true means that point 2 above will fail.

Thanks for the further input

I had to check for file existences in the past, because foreign binaries failed to do so or to provide a seamless experience and give more details, i.e. which file is missing for the external task which is about to trigger. At the end the external binary should and often will take care of TOCTOU, but checking the existence of those files still might be useful for gateways like wizard or wrapper applications etc.

This is sort of understandable scenario of using fs.exists. The point is probably how common that kind of situation is…

I try to bring up this topic again at least before actually deleting these APIs.

@martin-braun Sounds curious to me. Can you link to more specific context?

A valid use-case is to check if a specific file exists that a third-party binary needs before starting that binary eventually.

This still causes TOCTOU bugs. So I think the ideal solution to that situation is that the 3rd party binary handles the situation correctly and propagate the error to the user.