fs: ignore fstat errors on user fds in fs.readFile{Sync}

[joyeecheung]

On Windows fstat cannot be called on std fds. In those cases,
we already know that the fd is not for a regular file and can
just use a list of buffers to store the data.

Also makes sure that we don't close fds provided by users
if we cannot call fstat on them.

Fixes: #19831

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Note that this does not actually fix #19831 (comment) (on Windows calling fstat on those fds returns EISDIR..even if errors are intended, EISDIR is a rather odd error for this)

[joyeecheung]

CI: https://ci.nodejs.org/job/node-test-pull-request/14927/

[joyeecheung]

cc @nodejs/fs

[mscdex]

If it's just Windows, should we be checking process.platform === 'win32' && isStdFD(fd) instead?

[joyeecheung]

@mscdex hmm, yes, it would probably favor the case on non-windows platforms where the stdin is redirected from a regular file.

[joyeecheung]

Or, the check probably is unnecessary since on Windows fstat still works on stdin if it's redirected. The isUserFd check should already be enough in case of errors.

[joyeecheung]

Updated to just ignore fstat errors on user-provided fds. In cases where the std fds are redirected from regular files, we may still be able to know the size of the buffer to allocate.

CI: https://ci.nodejs.org/job/node-test-pull-request/14928/

[mscdex]

I'm confused as to why 0 size is now being forced for all user-supplied file descriptors if this is specifically about stdin/stdout/stderr. Also, if this is only a problem on Windows, I still think we should only be bypassing fstat() on that platform.

[mscdex]

Lastly, if this is only a problem in only some situations with stdin/stdout/stderr on Windows, why not just try fstat() first and if it results in an error and is one of stdin/stdout/stderr (and/or the error message is unique enough), only then use 0 for the size and continue?

[joyeecheung]

I'm confused as to why 0 size is now being forced for all user-supplied file descriptors if this is specifically about stdin/stdout/stderr. Also, if this is only a problem on Windows, I still think we should only be bypassing fstat() on that platform.

The size is only assumed 0 when the user-provided fd cannot be fstated. If it cannot actually be read, then there will be an error when the read call happens.

[joyeecheung]

Lastly, if this is only a problem in only some situations with stdin/stdout/stderr on Windows, why not just try fstat() first and if it results in an error and is one of stdin/stdout/stderr (and/or the error message is unique enough), only then use 0 for the size and continue?

That is more-or-less what this PR does at the moment. Difference is it does not error out right away when fstat returns an error even if the fd is not 0/1/2. If the fd ends up being unreadable, there will be an error when read is invoked.

[mscdex]

I still think we should be more strict when enabling this behavior, checking platform, error type, whether it's an std{in,out,err} fd, etc. No matter what I think this should be semver-major.

[joyeecheung]

I still think we should be more strict when enabling this behavior, checking platform, error type, whether it's an std{in,out,err} fd, etc. No matter what I think this should be semver-major.

I agree but the complexity should probably be best addressed in uv_fs_fstat. For one thing, EISDIR on those fds seems pretty odd.

[mscdex]

I'm not so sure that it makes sense to change it in libuv, since reporting a size of 0 but the fd still being readable seems contradictory?

[joyeecheung]

I'm not so sure that it makes sense to change it in libuv, since reporting a size of 0 but the fd still being readable seems contradictory?

stdin without being directed on non-windows platforms also has size 0 in fstat so that doesn't seem to be an issue. (also, the fact that you can get an EISDIR with fstat is...odd)

[addaleax]

This might be a silly question, but why are we trying to get the file size in advance anyway? I could imagine it is to save on the # of Buffer allocations, but I don’t think it’s really necessary, right?

[joyeecheung]

This might be a silly question, but why are we trying to get the file size in advance anyway? I could imagine it is to save on the # of Buffer allocations, but I don’t think it’s really necessary, right?

@addaleax Definitely not a silly question. The size is used to

1. Throw a ERR_FS_FILE_TOO_LARGE early if it's larger than Buffer.kMaxLength
2. Allocate the buffer beforehand with the size if it's a regular file
3. Allocate a list of buffers of kReadFileBufferLength if it's not a regular file (size = 0, which applies to unredirected stdin, which is also set when mode & S_IFREG is 0) and concatenate them when the read ends.

[joyeecheung]

If we want to eliminate the need to know the size beforehand I think we could only go with 3 (always use list of buffers to deal with unknown size), but that adds the overhead of concatenation for the most common cases (reading a regular file)

[BridgeAR]

Ping @addaleax

[addaleax]

Not sure about the ping (I didn’t block this & my questions had been answered) but LGTM. Still needs a rebase, though. :)

Note that on Windows, calling fstat on std fds that are not redirected returns EISDIR but they can still be read, so we just need to avoid treating them as regular files.

Is that a bug in libuv?

[joyeecheung]

@addaleax

Is that a bug in libuv?

I thinks so, at least EISDIR does not even make sense for fstat given that the returned result is supposed to tell you if that fd is for a directory or not.

[joyeecheung]

Rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/16024/

[jasnell]

Ping @joyeecheung

[Trott]

@joyeecheung Still working on this?

[joyeecheung]

@Trott Not really...I'll close this