Issue #649 init_env shouldn't guess the return value of open(2)

#49 Declined
Repository
asomers
Branch
issue_649
Repository
tildeslash
Branch
master
Author
  1. asomers
Reviewers
Description

Don't try to guess what file descriptor will be returned by open(2). It's unreliable. For example, it isn't thread safe. Use dup2(2) instead. dup2 also closes the destination file descriptor, which is something this function wants to do anyway.

While I'm here, fix a file descriptor leak.

Comments (9)

  1. Tildeslash repo owner

    This is done before any threads starts so thread-safe is not an issue here. On open, Unix returns the lowest available file descriptor. We start with 0 and count upwards and if descriptor is not open and then opens it the returned descriptor should match. Otherwise it is true that in general one cannot assume the descriptor number. What is STAF? If this does not work there, the simplest solution is probably to just remove the original if-test on line 93, as we don't really care what the file descriptor is, just that descriptors 0,1,2 are open. Ps. Of course, this still depends on that open returns lowest file descriptors. If STAF (whatever that is), returns just "random" descriptors on open, then your patch with dup seems like a good solutions. However, not returning the lowest file descriptor on open seems like a very un-Unixy way to behave

  2. asomers author

    STAF is the Software Test and Automation Framework. It's got a big grab-bag of functionality, but in this case I'm using it to spawn a process on a remote system. The STAF daemon forks and execs something else, which then forks and execs "monit restart". STAF is not an emulator or a replacement C library, so it doesn't implement fstat or open itself. Those go to the operating system as usual. I'm not sure why, but when I use STAF in this way I consistently see fstat(0,...) return EBADF followed by open return 3. Using dup2 fixes the problem.

  3. Tildeslash repo owner

    This means that stdin is not recognized as a valid open file descriptor. That is what the error from fstat means. This seems very wrong. If true, any call to stdio functions would fail. I'm not sure if this is a problem in STAF or the combination of STAF and Monit. I guess STAF is not able to start Monit at all then sinceinit_env is only called at Monit startup? In any case, I'm not sure about merging this patch since it seems to be caused by a very specific problem which might not be in Monit

  4. asomers author

    This certainly can't be a problem in STAF, because STAF doesn't interfere with either fstat or open. All it does is connect monit's stdio file descriptors to something that fstat doesn't like. With this patch, Monit works fine when invoked by STAF. Also, Monit 5.20 worked fine with no patch.

    That said, I realize now that there is an error in the patch. I assumed that `Util_closeFds` closed the stdio file descriptors. But now I see that it closes everything else instead. I'll fix it.

  5. Tildeslash repo owner

    In the comment to issue #649 you wrote

    STAF detached from its controlling terminal, but never closed stdin. At the end of the boot process, the OS revoked that terminal, invalidating all its open file descriptors . STAF passed that invalid file descriptor to monit. fstat returned EBADF not because 0 wasn't a file descriptor, but because it was now invalid.

    I.e. dup'ing an invalid file descriptor does not seem to be a good solution. To me, this looks like a problem in STAF and how STAF handle daemonizing. Wouldn't it be better to try fixing STAF?

  6. asomers author

    I think you read the patch wrong. I use dup2 to dup the "/dev/null" file descriptor, assigning it the value of the invalid file descriptor.