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

[LumaKernel]

Describe the bug A clear and concise description of what the bug is.

fs.exists/fs.existsSync throws when querying subpath of existing file.

To Reproduce Steps to reproduce the behavior:

1. In any directory,
2. touch a.txt
3. deno --unstable
4. > import * as fs from "https://deno.land/[email protected]/fs/mod.ts";
5. > await fs.exists("a.txt/b");
6. Error is thrown. See error

Expected behavior A clear and concise description of what you expected to
happen.

Return false.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: Ubuntu 20.04 with WSL on Windows11
• Version

deno 1.13.2 (release, x86_64-unknown-linux-gnu)
v8 9.3.345.11
typescript 4.3.5

Additional context Add any other context about the problem here.

Other implementations:

• Python3 os.path.exists: Return false.
• Node.js v14 require("fs").exists: Resolves false.
• Ruby 2.7 File.exist?: Return false.

Simple alternative implementation suggest:

const exists = async (filePath: string): Promise<boolean> => {
  try {
    await Deno.lstat(filePath);
    return true;
  } catch (_e: unknown) {
    return false;
  }
};

fs/exists_test.ts doesn't include any throws/rejects test.

[LumaKernel]

Related: nodejs/node#39960

Implementing exists generally seems problematic. Deprecation is another option. Maybe, user of exists should be aware of its implementation and maybe user should use stat itself depending on the situations.

[kt3k]

Deprecating fs.exists might make sense. It always causes TOCTOU issue and fs.exists can be considered as an anti-pattern.

What do you think? @bnoordhuis @piscisaureus @ry

[bnoordhuis]

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 nodejs/node#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!

[martin-braun]

@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.

[kt3k]

@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.

[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.

@kt3k You are right. In theory the binary that needs the file should at least exit with an error code when it catches such exception, I just like to point out that in real life scenarios things are not always in control of us.

I do not have a specific case to explore, but I know that 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.

It's really an edge case and I agree that we should teach about TOCTOU by deprecating "exists", but node provides an alternative for special cases which I would appreciate in Deno as well.

[kt3k]

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]

@kt3k Thank you and yes I agree, it's a rare situation, yet not avoidable in all cases.

[dev-nicolaos]

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.

[martin-braun]

@kt3k After a while I figured out that Deno has a way to check for file existence and uses this approach itself in the std lib.

await Deno.lstat(filename: string) will throw if the file/folder does not exist. A proper way to do what fs.access() did in node, but it will also give information about the file system item which can be a file or a folder.

@dev-nicolaos Use fs.ensureFile to create a file if it does not exist. Whatsoever, I agree that TOCTOU is not solved, but we ended up with an uglier version of exists, but actually the lack of exists has a clear benefit:

Since checking for a file/folder is more annoying, since I have to use try/catch I actively search for alternate ways from the std lib, such as fs.ensureFile. I know how I programmed in past. When I was in doubt about a file-handling function (i.e. "will it create the file for me or do I have to create it myself?"), I used exists inflationary to be sure my script works, but also introduced unnecessary TOCTOUs that could've been avoided.

I know this is subjective, but this mindset made the lack of exists not a big of a deal, there are definitely ways to handle each situation properly. At the very end Deno.lstat is superior of exists, because you will also know what kind of file entry you are dealing with (file or folder).

[kt3k]

@martin-braun Do you now agree with the deprecation?

[martin-braun]

Do you now agree with the deprecation?

@kt3k Yes, but it still feels subjective. The deprecation of exists does not solve or prevent TOCTOU, but pushes people to a programming style that has less TOCTOU potential.

The deprecation warning should be longer and more detailed, listing up all common scenarios and how to solve them, i.e. ensureFile, ensureFolder, directly writing to a file without prior check, because it is unnecessary and as a last straw lstat within try/catch. This is the least the documentation should feature in my opinion, but it must be done cleverly. I don't want people to just convert exists to lstat, but this might happen with this change. They wouldn't understand and thus be annoyed by these changes, because exists is so much more convenient than try, catch, lstat.

[dev-nicolaos]

@dev-nicolaos Use fs.ensureFile to create a file if it does not exist

@martin-braun this still doesn't cover the scenario I outlined above. Using fs.ensureFile as opposed to fs.exists with those requirements would mean the script never generates anything (always hits # 2). The key piece there is that the per-existence of the file has meaningful value.

[martin-braun]

@dev-nicolaos Sorry, I have problems to understand what you try to achieve. Please quote what you "outlined above", so it's easier to follow.

Above you said:

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

And fs.ensureFile will do both things for you, or is X is a different path compared to the file existence you check?

If you really need to know if the file exists beforehand or you want to create a file X when file Y exists, then yes, you have to use lstat, ensureFile really just does the same.

But think it this way: lstat allows you to figure out if it is a file, a directory or if the file system entry does not exist in one go. I think you wouldn't want to proceed when the path is a folder instead of a file, so using exists would be redundant, since you check the file system entry in any case.

@kt3k I think it would be nice if ensureFile, ensureFileSync, ensureDir and ensureDirSync would return a boolean that tells the user if the file was just created or if it already existed? I'm not sure if this is common practice, but it would help in some cases to avoid lstat.

[dev-nicolaos]

The sudo code for that example is roughly...

if no file exists at path X <-- what goes here???
   create file at path X with content Y
else
  do nothing (except maybe log a message)

ensureFile isn't helpful in this circumstance. It could be used inside the conditional once you know a file doesn't already exist (although I think just using Deno.writeFile with the create: true is easier for that), but that doesn't help with the conditional itself. exists feels like the natural thing to use, without it...

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

or...

options could be added to Deno.WriteFileOptions to account for this circumstance

This could be modifying how the existing options (append, create) work, or adding in something like

Deno.writeTextFileSync('path/to/maybe-file', data, { errorIfExists: true }

Bottom Line

For the use case where you want to create a file only if no file already exists at a path, exists is the cleanest option in Deno right now. It does have the TOCTOU issue, but there are real world scenarios where that isn't a concern. It'd be nice to have a better way to do this than just re-implementing exists in each project, but I don't really care if that solution is keeping exists or something else.

[martin-braun]

@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?

[kt3k]

@martin-braun

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?

What API (option name) do you suggest more exactly? I feel it'd be a little too complex and too specific to certain scenario as a Deno namespace API.

In general deno_std is more open to the addition of functions which are specific to some scenarios.

[martin-braun]

@kt3k I'm not sure, playing with my thoughts, at the moment. I agree though, altering Deno.writeTextFile is not really a good idea for such a specific case. Hmm, maybe fs.ensureFile could take an optional argument with the contents that will be written to the file, when it's created? Something like await fs.ensureFile("myfile.txt", "Hello World") (<- same encoding like on Deno.writeTextFile), but I fear we are giving the impression that it will be ensured that the file has such contents, even when it exists.

When I suggest with question marks, I really want to poke, maybe one of you have some idea how to cover this case properly. But, when it doubt, leave it out ..

[bnoordhuis]

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).

[bartlomieju]

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

[martin-braun]

@bnoordhuis @bartlomieju

"create file only when not already there"

is basically { create: true } and { createNew: true }. The difference is the latter will throw an error if the file already exists.

[lambdalisue]

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.

[martin-braun]

@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.

[lambdalisue]

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.

[martin-braun]

@lambdalisue So you say git should handle the error that .git is a file not a folder, but it should not handle the error that .git does not exist? This sounds very inconsistent to me, but it would be reasonable if .git can be a file that contains a path, but is this officially supported by git? I couldn't find any documentation that git can handle a .git file.

Anyway, for the sake of dealing with an example in your favor, let's give you a proper case that supports your stance: If you have a situation in which it does not matter if you have a file or a folder but need to spawn a process if either of those exists under a given path/name, but for performance reasons avoid spawning such process if neither of those exist under a given path/name, then - so far I see - exists would have legitimacy to not be deprecated as it would resolve in shorter code compared to lstat.

The official counter argument is that you should spawn such process without checking with exists and catch the error of git which is certainly slower than checking for a file system entry, I agree. My former argument was that such process also might fail to report, accordantly, so I even backup your claim in some sense.

But, the honest truth is that this situation is: It is very rare. If you check for a path for good reason you also should check if you are dealing with a file or folder, because in 99% of the time you expect one or another, but not any of them.

In my personal subjective opinion for the 1% I'd say that you should just bite the bullet and use lstat instead. I know from personal experience that people implement unnecessary TOCTOU using exists, because it feels natural to do it this way, but that's my personal opinion.

[lambdalisue]

@lambdalisue So you say git should handle the error that .git is a file not a folder, but it should not handle the error that .git does not exist? This sounds very inconsistent to me, but it would be reasonable if .git can be a file that contains a path, but is this officially supported by git? I couldn't find any documentation that git can handle a .git file.

It's off topic so I don't want to dig it deeper but see https://git-scm.com/docs/git-init#Documentation/git-init.txt---separate-git-dirltgit-dirgt if you need some evidence.

Anyway, for the sake of dealing with an example in your favor, let's give you a proper case that supports your stance: If you have a situation in which it does not matter if you have a file or a folder but need to spawn a process if either of those exists under a given path/name, but for performance reasons avoid spawning such process if neither of those exist under a given path/name, then - so far I see - exists would have legitimacy to not be deprecated as it would resolve in shorter code compared to lstat.

The official counter argument is that you should spawn such process without checking with exists and catch the error of git which is certainly slower than checking for a file system entry, I agree. My former argument was that such process also might fail to report, accordantly, so I even backup your claim in some sense.

But, the honest truth is that this situation is: It is very rare. If you check for a path for good reason you also should check if you are dealing with a file or folder, because in 99% of the time you expect one or another, but not any of them.

I'm sorry but my point of view is not an exact case what I mentioned. I'd like to point out that there is situation that we need to check presence of a file/directory and we need to use deprecated code in that case because there is no other way.
If we need to use a deprecated code anyway, why that code should be deprecated? Documentation or warning should be distinctive from deprecation.

In my personal subjective opinion for the 1% I'd say that you should just bite the bullet and use lstat instead. I know from personal experience that people implement unnecessary TOCTOU using exists, because it feels natural to do it this way, but that's my personal opinion.

In my opinion, using exists and using lstat is same. If using exists is deprecated, using lstat (to check file/directory presence) should be deprecated as well because both does same thing internally. So using lstat instead is out point of my view.

Ok, another example. I'm sorry but git again.

Git create a file MERGE_HEAD or similar in merging phase and git itself use the presence of these files to determine the current git status (ref).
So if I'd like to create a some program to show a current git status, I need to check presence of MERGE_HEAD or whatever and I need to use exists anyway.

[martin-braun]

@lambdalisue Thanks for the evidence in regards of git.

I'd like to point out that there is situation that we need to check presence of a file/directory and we need to use deprecated code in that case because there is no other way. [...] In my opinion, using exists and using lstat is same.

lstat can definitely do all and more compared to exist, I hope this is a point you can agree on, because after all exists is just an abstraction.

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? Do you also think the deprecation was wrong in node.js for the same reasons you are clarifying?

It feels exists is an abstraction that has been deprecated for the sake of preventing bad programming when handling files/folders, but you are right that TOCTOU can also happen using lstat. Worst would be if people simply implement exists on their own and still stepping in the same pitfalls.

I would suggest you open a new issue to push a potential change. Best is to explain the very specific case in which you want to check for the existence of a file system entry and don't care about if it's a file or a folder to launch a sub process that handles that file system entry properly. I think your example in which you want to check if .git exists before launching git to prevent slow down is a valid case, since .git can be a file or folder, so using lstat instead would just cause more code without having a benefit down the line.

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

Just saying exists and lstat is the same won't get this anywhere, because it is not. 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. (I know it's a little picky, who would touch the .git contents, right?)

[lambdalisue]

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.

[lambdalisue]

I would suggest you open a new issue to push a potential change. Best is to explain the very specific case in which you want to check for the existence of a file system entry and don't care about if it's a file or a folder to launch a sub process that handles that file system entry properly. I think your example in which you want to check if .git exists before launching git to prevent slow down is a valid case, since .git can be a file or folder, so using lstat instead would just cause more code without having a benefit down the line.

Thanks for the suggestion. I added a discussion for this https://github.com/denoland/deno_std/discussions/2102