10 ms·

This reminds me of one of the most epic bugs I've ever run into: mkdir("/foo", 0700); chdir("/foo"); recursively_delete_everything_in_current_direc

This reminds me of one of the most epic bugs I've ever run into: mkdir("/foo", 0700); chdir("/foo"); recursively_delete_everything_in_current_directory(); Running as root, this usually worked fine: It would create a directory, move into it, and clean out any garbage left behind by a previous run before doing anything new. Running as non-root, the mkdir failed, the chdir failed, and it started eating my home directory.

stinos 12y ago
When you see chdir, or any notion of the current working directory being used for anything: run as fast as you can. (or refactor if it's not too late). Things I've seen because of software relying on it.. Sometimes it's just directories/files it creates popping up all over the place, sometimes it's 'just' crashing, but yes sometimes it starts to erase and all hell really breaks loose.
- cperciva 12y ago
  In this particular case, I didn't see the chdir until after the fact. This wasn't my code...
- dllthomas 12y ago
  Right, better to use the *at() functions (e.g. http://linux.die.net/man/2/unlinkat http://linux.die.net/man/2/unlinkat) where applicable.
- agwa 12y ago
  Sometimes there are legitimate reasons for relying on the CWD, such as avoiding time-of-check/time-of-use race conditions. The *at syscalls provide a better alternative, but (at least as of a few years ago) they weren't widely implemented outside of Linux and Solaris.
- pachydermic 12y ago
  If you can't rely on current working directories then you have to specify any file locations absolutely? That doesn't seem like a good idea because then your code quickly turns into a hot mess if you ever have to change where stuff lives. This is such a stupid problem I run into a lot. Both alternatives (doing things with absolute paths vs doing things entirely with relative paths) seem to have a lot of downsides. Overall relative paths seems to be way better, but then you leave yourself open to problems which "rhyme" with the one OP was talking about.
  - dllthomas 12y ago
    As agwa and I mentioned in sibling comments, there are the ...at() functions, which let you specify actions on paths relative to a specific directory you have a file descriptor for. This not only avoids issues like the above (failing to open the directory and then failing to check for failure will mean you're passing -1 into unlinkat, which would simply fail) but will also keep you talking about the same place if links are moved around somewhere up the tree from where you are working.
    - tedunangst 12y ago
      Rearranging the tree above CWD isn't a problem, CWD will follow along just fine (as in, be the same directory). Also, you're assuming AT_FDCWD won't be -1, which could be reasonable but isn't guaranteed afaik.
      - dllthomas 12y ago
        "Rearranging the tree above CWD isn't a problem, CWD will follow along just fine (as in, be the same directory)." I was citing rearranging the tree is a potential issue with absolute directories, not with relying on CWD - that certainly could have been clearer. "Also, you're assuming AT_FDCWD won't be -1, which could be reasonable but isn't guaranteed afaik." Interesting point regarding guarantees. It's not -1 on any existing OS that I can find (it seems to be -100 on Linux and FreeBSD, -3041965 on Solaris, -2 on AIX), and shouldn't be for precisely this reason, but something to bear in mind if you are working on something more obscure that nonetheless has these functions. Of course, you shouldn't be relying on reasonable behavior from functions passed a bad FD in general. It's just nice to have the additional defense when that does get missed.
  - nuxi 12y ago
    There's a good reason you can't rely on CWD other than root - the directory can be on a different mountpoint and possibly even on a remote mountpoint. If the remote server goes down or the mountpoint is forcefully unmounted, what would your CWD point to then? Root is the only directory guaranteed to always exist, that's why you'd see a chdir('/') as one of the first steps in properly written unix daemons.
- bodyfour 12y ago
  The real problem with chdir is when that code ends up refactored into a library and ends up used in a multithreaded program. Then you've got an ugly bug.
  - dllthomas 12y ago
    That's yet another real problem with chdir, on top of the 10 other real problems with chdir, of which we've mentioned a couple here.
  - ygra 12y ago
    Which probably can be summarized as »global state is a really bad idea«.
    - dllthomas 12y ago
      I wonder if it would be worthwhile to forego the notion of a CWD for processes entirely.
      - ygra 12y ago
        You'd have to use absolute paths everywhere, but that probably doesn't hurt that much in programs or scripts. The CWD seems most useful in interactive shells, I guess. A fun thing is PowerShell on Windows where you have two CWDs, one from the process and another one from the shell which had is own VFS handling (e.g. the registry is a place that has no representation in the normal file system). Cmdlets use one of them and external commands and .NET APIs use the other. So in the latter case you always need Resolve-Path foo.bar instead of just foo.bar for things to work properly.
        dllthomas 12y ago
        You wouldn't have to use absolute paths everywhere - you could use paths relative to any file descriptor that pointed at a directory. The shell itself wouldn't seem to have any problem - it could maintain a logical CWD without assistance. Utilities would probably need some other convention - maybe fd 4 points at the directory they are to operate in at start? In a sense, this is "still a CWD" - but the differences would be 1) you can maintain multiple at the same time, and 2) you could close it.
- jrockway 12y ago
  I chdir("/") before daemonizing so that the filesystem the process was using as its cwd can be unmounted :)
- peterwwillis 12y ago
  The exception is chdir("/"). This will always give you the desired behavior, only error on EACCES, and prevent many nasty filesystem problems. This also removes the idea of relative paths, so the user is forced to use full paths (or, every relative path is a full path). Not very practical but it does work.
swah 12y ago
In those times I wish I could use the emacs lisp way: (let (dir "/foo") (create-directory dir) (with-current-directory dir (delete-all-files-recursively))) Factor recognized the value of dynamically scoped variables: http://concatenative.org/wiki/view/Factor/FAQ/What's%20Factor%20like%3F http://concatenative.org/wiki/view/Factor/FAQ/What's%20Facto... A lot of code became much simpler because of that decision.
- Alupis 12y ago
  This isn't really fork() failing per se -- but rather a failed program/script that did not understand the well defined and clearly documented behavior of fork().
  - dllthomas 12y ago
    The man page says, "On failure, -1 is returned in the parent, no process is created, and errno is set appropriately." So if fork is behaving as documented, returning -1 is because of "fork failing".
    - Alupis 12y ago
      True -- I guess fork() has failed at that point. I was more getting at the article's authors scenarios of careless scripts treating -1 as a valid pid (which should always be > 0), which would be a failure of the script instead.
      - dllthomas 12y ago
        Well, it's a failure of the script and the POSIX API - it is unfortunate that a pid_t inhabited by -1 means "failure" in one case and "everything" in another. It is certainly not fork, narrowly, to blame.
        M00n_Sl47r 12y ago
        I think the point that the author was making, outside of any criticism to the API itself, is that the hapless programmer may not know that fork() could even return -1. The article is pointing out that possibility as documented by the API, not criticizing a failure of the API. Whether or not the API is faulty in this--and I tend to agree with you--is outside of the scope of the article, though only just.
Alupis 12y ago
Could you have not just checked to see if you actually created the directory and/or check to make sure you moved into the directory before proceeding with your destructive function? (Defensive Programming 101 really).
- ColinDabritz 12y ago
  They could have, but then they wouldn't have a great 'epic bug' story to share! Missing the basics is part of what leads to mistakes like this, and I'd wager that after getting bit by that they are much more careful today. Also, don't rely on implicit state (the "current directory") for a destructive command, pass the dependency in: recursively_delete_directory("/foo")
  - Alupis 12y ago
    Very true. Reminds me of a time when I was working on a package system for an in-house linux os build and carelessly had my fakeroot directory set wrong in my configurations, which ended up treating my local root / directory as the root of the fakeroot, which is as bad as it sounds. Running the script over-wrote my entire /etc directory among other important and un-recoverable things... Gladly, it was a development system so nothing crucial was lost. Needless to say, I am way more careful today in-part due to this mishap (and hours of setting up a new dev system!)
- cperciva 12y ago
  There's lots of things they could have done. As I said elsewhere, this wasn't my code,
- jwr 12y ago
  You do realize that this, in turn, creates a race condition, right?
  - Alupis 12y ago
    ? not at all -- you make the deletion of the directory dependent on whether or not you tested true for the directory being present. I don't see why this would cause any race condition...
MaulingMonkey 12y ago
The variant of this I ran into was significantly less destructive. recursively_find_everything_in_current_directory() crashed due to a stack overflow when SetCurrentDirectory failed due to a single corrupt NTFS directory.
blueskin_ 12y ago
Normal users can't make a directory at the root of the filesystem by design. To write that safely, you'd test for the result of mkdir and cd to return success before deleting. In a shell, you'd just go mkdir /foo -m 0700 && cd /foo && ${deleteallfiles}

stinos 12y ago

When you see chdir, or any notion of the current working directory being used for anything: run as fast as you can. (or refactor if it's not too late). Things I've seen because of software relying on it.. Sometimes it's just directories/files it creates popping up all over the place, sometimes it's 'just' crashing, but yes sometimes it starts to erase and all hell really breaks loose.

cperciva 12y ago
In this particular case, I didn't see the chdir until after the fact. This wasn't my code...
dllthomas 12y ago
Right, better to use the *at() functions (e.g. http://linux.die.net/man/2/unlinkat http://linux.die.net/man/2/unlinkat) where applicable.
agwa 12y ago
Sometimes there are legitimate reasons for relying on the CWD, such as avoiding time-of-check/time-of-use race conditions. The *at syscalls provide a better alternative, but (at least as of a few years ago) they weren't widely implemented outside of Linux and Solaris.
pachydermic 12y ago
If you can't rely on current working directories then you have to specify any file locations absolutely? That doesn't seem like a good idea because then your code quickly turns into a hot mess if you ever have to change where stuff lives. This is such a stupid problem I run into a lot. Both alternatives (doing things with absolute paths vs doing things entirely with relative paths) seem to have a lot of downsides. Overall relative paths seems to be way better, but then you leave yourself open to problems which "rhyme" with the one OP was talking about.
- dllthomas 12y ago
  As agwa and I mentioned in sibling comments, there are the ...at() functions, which let you specify actions on paths relative to a specific directory you have a file descriptor for. This not only avoids issues like the above (failing to open the directory and then failing to check for failure will mean you're passing -1 into unlinkat, which would simply fail) but will also keep you talking about the same place if links are moved around somewhere up the tree from where you are working.
  - tedunangst 12y ago
    Rearranging the tree above CWD isn't a problem, CWD will follow along just fine (as in, be the same directory). Also, you're assuming AT_FDCWD won't be -1, which could be reasonable but isn't guaranteed afaik.
    - dllthomas 12y ago
      "Rearranging the tree above CWD isn't a problem, CWD will follow along just fine (as in, be the same directory)." I was citing rearranging the tree is a potential issue with absolute directories, not with relying on CWD - that certainly could have been clearer. "Also, you're assuming AT_FDCWD won't be -1, which could be reasonable but isn't guaranteed afaik." Interesting point regarding guarantees. It's not -1 on any existing OS that I can find (it seems to be -100 on Linux and FreeBSD, -3041965 on Solaris, -2 on AIX), and shouldn't be for precisely this reason, but something to bear in mind if you are working on something more obscure that nonetheless has these functions. Of course, you shouldn't be relying on reasonable behavior from functions passed a bad FD in general. It's just nice to have the additional defense when that does get missed.
- nuxi 12y ago
  There's a good reason you can't rely on CWD other than root - the directory can be on a different mountpoint and possibly even on a remote mountpoint. If the remote server goes down or the mountpoint is forcefully unmounted, what would your CWD point to then? Root is the only directory guaranteed to always exist, that's why you'd see a chdir('/') as one of the first steps in properly written unix daemons.
bodyfour 12y ago
The real problem with chdir is when that code ends up refactored into a library and ends up used in a multithreaded program. Then you've got an ugly bug.
- dllthomas 12y ago
  That's yet another real problem with chdir, on top of the 10 other real problems with chdir, of which we've mentioned a couple here.
- ygra 12y ago
  Which probably can be summarized as »global state is a really bad idea«.
  - dllthomas 12y ago
    I wonder if it would be worthwhile to forego the notion of a CWD for processes entirely.
    - ygra 12y ago
      You'd have to use absolute paths everywhere, but that probably doesn't hurt that much in programs or scripts. The CWD seems most useful in interactive shells, I guess. A fun thing is PowerShell on Windows where you have two CWDs, one from the process and another one from the shell which had is own VFS handling (e.g. the registry is a place that has no representation in the normal file system). Cmdlets use one of them and external commands and .NET APIs use the other. So in the latter case you always need Resolve-Path foo.bar instead of just foo.bar for things to work properly.
      - dllthomas 12y ago
        You wouldn't have to use absolute paths everywhere - you could use paths relative to any file descriptor that pointed at a directory. The shell itself wouldn't seem to have any problem - it could maintain a logical CWD without assistance. Utilities would probably need some other convention - maybe fd 4 points at the directory they are to operate in at start? In a sense, this is "still a CWD" - but the differences would be 1) you can maintain multiple at the same time, and 2) you could close it.
jrockway 12y ago
I chdir("/") before daemonizing so that the filesystem the process was using as its cwd can be unmounted :)
peterwwillis 12y ago
The exception is chdir("/"). This will always give you the desired behavior, only error on EACCES, and prevent many nasty filesystem problems. This also removes the idea of relative paths, so the user is forced to use full paths (or, every relative path is a full path). Not very practical but it does work.

swah 12y ago

In those times I wish I could use the emacs lisp way: (let (dir "/foo") (create-directory dir) (with-current-directory dir (delete-all-files-recursively))) Factor recognized the value of dynamically scoped variables: http://concatenative.org/wiki/view/Factor/FAQ/What's%20Factor%20like%3F http://concatenative.org/wiki/view/Factor/FAQ/What's%20Facto... A lot of code became much simpler because of that decision.

Alupis 12y ago
This isn't really fork() failing per se -- but rather a failed program/script that did not understand the well defined and clearly documented behavior of fork().
- dllthomas 12y ago
  The man page says, "On failure, -1 is returned in the parent, no process is created, and errno is set appropriately." So if fork is behaving as documented, returning -1 is because of "fork failing".
  - Alupis 12y ago
    True -- I guess fork() has failed at that point. I was more getting at the article's authors scenarios of careless scripts treating -1 as a valid pid (which should always be > 0), which would be a failure of the script instead.
    - dllthomas 12y ago
      Well, it's a failure of the script and the POSIX API - it is unfortunate that a pid_t inhabited by -1 means "failure" in one case and "everything" in another. It is certainly not fork, narrowly, to blame.
      - M00n_Sl47r 12y ago
        I think the point that the author was making, outside of any criticism to the API itself, is that the hapless programmer may not know that fork() could even return -1. The article is pointing out that possibility as documented by the API, not criticizing a failure of the API. Whether or not the API is faulty in this--and I tend to agree with you--is outside of the scope of the article, though only just.

Alupis 12y ago

Could you have not just checked to see if you actually created the directory and/or check to make sure you moved into the directory before proceeding with your destructive function? (Defensive Programming 101 really).

ColinDabritz 12y ago
They could have, but then they wouldn't have a great 'epic bug' story to share! Missing the basics is part of what leads to mistakes like this, and I'd wager that after getting bit by that they are much more careful today. Also, don't rely on implicit state (the "current directory") for a destructive command, pass the dependency in: recursively_delete_directory("/foo")
- Alupis 12y ago
  Very true. Reminds me of a time when I was working on a package system for an in-house linux os build and carelessly had my fakeroot directory set wrong in my configurations, which ended up treating my local root / directory as the root of the fakeroot, which is as bad as it sounds. Running the script over-wrote my entire /etc directory among other important and un-recoverable things... Gladly, it was a development system so nothing crucial was lost. Needless to say, I am way more careful today in-part due to this mishap (and hours of setting up a new dev system!)
cperciva 12y ago
There's lots of things they could have done. As I said elsewhere, this wasn't my code,
jwr 12y ago
You do realize that this, in turn, creates a race condition, right?
- Alupis 12y ago
  ? not at all -- you make the deletion of the directory dependent on whether or not you tested true for the directory being present. I don't see why this would cause any race condition...

MaulingMonkey 12y ago

The variant of this I ran into was significantly less destructive. recursively_find_everything_in_current_directory() crashed due to a stack overflow when SetCurrentDirectory failed due to a single corrupt NTFS directory.

blueskin_ 12y ago

Normal users can't make a directory at the root of the filesystem by design. To write that safely, you'd test for the result of mkdir and cd to return success before deleting. In a shell, you'd just go mkdir /foo -m 0700 && cd /foo && ${deleteallfiles}