| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
“hoping” is doing a lot of work here 🙂 This was actually spec’d behavior in previous POSIX versions, and I was quite surprised to learn (through this PR) that the spec actually changed in this regard. I’d leave a comment in the source code about this, since others might be surprised by this as well. |
Sorry, something went wrong.
Got you. I'll do that in a little bit ✌️ |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
TBH, this change adds more failure paths and I don't think it's actually necessary.
The "open() returns the lowest available file descriptor" convention is so deeply codified in the UNIX ecosystem that it's never going away, no matter what POSIX says. There's simply too much software built around that assumption.
Sorry, something went wrong.
While I do agree with this, there still are specific cases where "available" means something different to what's expected. Case in point: #include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
int main(void) {
system("echo && fstat | grep a.out");
int fd = open("/dev/null", O_RDWR);
printf("\n/dev/null: %d\n", fd);
return 0;
}Running this on FreeBSD in a separate session with daemon -o log.log -r /tmp/a.out and then closing said session (thus making stdin invalid): root a.out 89214 text [restricted] 519 -rwxr-xr-x 15424 r root a.out 89214 wd / 107129 drwxr-xr-x 16 r root a.out 89214 root / 107106 drwxr-xr-x 24 r root a.out 89214 jail / 107106 drwxr-xr-x 24 r root a.out 89214 0 - - bad - root a.out 89214 1* pipe fffff8002e0e7d00 <-> fffff8002e0e7ba0 0 rw root a.out 89214 2* pipe fffff8002e0e7d00 <-> fffff8002e0e7ba0 0 rw root a.out 89214 3 [restricted] 24 crw-rw-rw- null rw /dev/null: 3 (This is actually the issue nodejs/help#2411 had.) Here, 0 should be available, yet /dev/null is still opened as 3. Something else that works is closing 0 before opening /dev/null, but IMHO at that point it's kinda relying on flaky behaviour. |
Sorry, something went wrong.
When checking for the validity of the stdio file descriptors (nodejs#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: nodejs#875
|
Ah okay, fair enough. Fd 0 points to a kind of zombie file description in your example, i.e., it's technically still open but not actually usable? |
Sorry, something went wrong.
AFAIU the file descriptor is never actually closed, so it isn't ever marked as unused (fdunused()), which is why it's never removed from the file descriptor table. |
Sorry, something went wrong.
|
Wait, but fstat() still fails with EBADF? How does that work? |
Sorry, something went wrong.
|
I don't know, there are plenty of places along the line where fstat(2) can fail with EBADF, not just because a file descriptor is not found in the file descriptor table (you can check out kern_fstat(), my quick guess is that fp->f_ops is set to &badfileops at some point before being unused). I could investigate further if you'd like an exact answer but I don't see how it's too relevant outside of the FreeBSD kernel :P |
Sorry, something went wrong.
|
"Zombie file descriptor" is good enough for me. :-) |
Sorry, something went wrong.
Sorry, something went wrong.
|
Is there anything left for me to do here? |
Sorry, something went wrong.
Sorry, something went wrong.
When checking for the validity of the stdio file descriptors (#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: #875 PR-URL: #44461 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When checking for the validity of the stdio file descriptors (#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: #875 PR-URL: #44461 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When checking for the validity of the stdio file descriptors (#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: #875 PR-URL: #44461 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When checking for the validity of the stdio file descriptors (#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: #875 PR-URL: #44461 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
| Back | FazBrowse Home | New Git URL |
When checking for the validity of the stdio file descriptors in PlatformInit(), this was the code previously used (as in #875):
As I understand it, the intention behind this is to map file descriptors to /dev/null if they don't exist, by hoping the next file descriptor to be given will be the one which doesn't exist. This is imperfect as it is very platform dependent and breaks when /dev/null has already been given a different file descriptor in the same process, but e.g. stdin has disappeared in the meantime (as is the case with this issue on FreeBSD with the daemon tool when not passing -f: nodejs/help#2411).
Instead, this patch uses the dup2(2) syscall to properly remap the missing stdio file descriptor to what /dev/null's file descriptor actually is.