fs.close twice different behavior on Windows 10

[thisconnect]

The following test case opens a file, and closes it twice expecting to get an Error.

var assert = require('assert');
var fs = require('fs');

// 'w+' - Open file for reading and writing.
// The file is created (if it does not exist)
// or truncated (if it exists).

fs.open(__dirname + '/test-to-open-a-file.txt', 'w+', function(err1, fd){

    console.log('open', fd);
    fs.close(fd, function(err2){

        console.log('close - should have no error', fd);
        assert.ifError(err2);

        fs.close(fd, function(err3){
            console.log('close2 - expect an error', fd);
            console.error(err3);
        });
    });
});

On Linux/Mac the second close callback err3 has the following Error: { [Error: EBADF: bad file descriptor, close] errno: -9, code: 'EBADF', syscall: 'close' }

On Windows 10 the second callback err3 has null

[thisconnect]

Tested with Node.js 5.0.0

[bnoordhuis]

What value does fd have on Windows? Libuv will silently ignore it if you try to close a file descriptor <= 2.

[thisconnect]

Updated the test to log fd after each close, the value of fd is always 3.

open 3
close - should have no error 3
close2 - expect an error 3
null 

[bnoordhuis]

/cc @nodejs/platform-windows - can someone confirm? The second fs.close() should fail with EBADF.

[seishun]

I confirm this on Windows 7.

[bnoordhuis]

Thanks. Next question: why does it happen? fs.close() ends up calling _close(), which should set _doserrno = EBADF. Either there is a bug in libuv or node.js somewhere or fd 3 is still (or again) alive.

[seishun]

It happens because _doserrno returns 0. _errno returns 9 (aka EBADF) as expected. Why does libuv read _doserrno?

Edit: _close(fd) does return -1.
Edit2: nodejs/node-v0.x-archive#4574 appears to be the same issue.

[bnoordhuis]

@piscisaureus?

[piscisaureus]

It happens because _doserrno returns 0. _errno returns 9 (aka EBADF) as expected. Why does libuv read _doserrno?

Libuv has no routines for mapping a CRT error to a libuv error code. Therefore it reads _doserrno (sometimes, more often it just calls GetLastError()) to get the win32 error code and maps that to a libuv error.

This works most of the time, but in the case of an invalid file descriptor there is no such thing as a 'win32 error code', since the file descriptor table is maintained entirely in user space by the CRT. That's why _doserrno is 0 here - no syscalls were made hence no win32 error was produced.

A simple solution would be to add a special case for EBADF (and potentially ENFILE/EMFILE) to libuv where it deals with file descriptors directly.

On a side note, using invalid file descriptors is normally a reason for the CRT to immediately crash the application, like a segmentation fault would. Node explicitly turns off this behavior, which I consider a really questionable practice, and it makes me wonder whether there are adverse security implications when e.g. certain libraries don't expect this behavior to be overridden.

[seishun]

Libuv has no routines for mapping a CRT error to a libuv error code. Therefore it reads _doserrno (sometimes, more often it just calls GetLastError()) to get the win32 error code and maps that to a libuv error.

Is it problematic to add a routine for mapping Win32 error codes to libuv error codes?

[piscisaureus]

Is it problematic to add a routine for mapping Win32 error codes to libuv error codes?

That already exists: https://github.com/libuv/libuv/blob/v1.x/src/win/error.c#L66

What you mean is, "is it problematic to add a routine got mapping CRT error codes to libuv error codes?"
That'd be fine, although we shouldn't use it, because (most of the time) the CRT itself does a mapping from a WIN32 error code, but it isn't always right! So if we have a WIN32 error we want to do the mapping directly.

[piscisaureus]

A poor man's diagram illustrating how error codes are mapped on windows:

           /-(1)- winsock error code -----------------------(3)-\
NTSTATUS -<                                                      \
           \-(2)- win32 error code -------------------------(3)--->- libuv error code
                                    \                            /
                                     \-(4)- CRT error code -(5)-/

(1) RtlNtStatusToDosError()
(2) SockNtStatusToSocketError() and uv_ntstatus_to_winsock_error()
(3) uv_translate_sys_error()
(4) _dosmaperr() (internal CRT function)
(5) uv_translate_crt_error() (to be added)

[seishun]

Is fs__close the only case where SET_REQ_RESULT is broken?

[piscisaureus]

Is fs__close the only case where SET_REQ_RESULT is broken?

It looks like it. fs__open() already deals with the EMFILE case properly, and there are no other functions that use a CRT function in conjunction with a file descriptor.

... except fs__sendfile (sigh, when are we removing this?)

[seishun]

Per the MSDN docs, _close always sets errno to EBADF if it returns -1. Do you think it would be a good enough fix to just do SET_REQ_UV_ERROR(req, UV_EBADF, ERROR_INVALID_HANDLE) when _close returns -1?

On a side note, I dislike artificially setting a Win32 error code, but that's the way it's done in other places.

[piscisaureus]

Per the MSDN docs, _close always sets errno to EBADF if it returns -1. Do you think it would be a good enough fix to just do SET_REQ_UV_ERROR(req, UV_EBADF, ERROR_INVALID_HANDLE) when _close returns -1?

You're right; I just looked it up in the crt source code (not on github unfortunately) -- _close() ignores the return value from CloseHandle(). The only error that ever gets reported is if the file descriptor isn't valid. So yes what you propose is good enough, but add comments or asserts to express the assumptions that the code makes.

On a side note, I dislike artificially setting a Win32 error code, but that's the way it's done in other places.

I agree.

[seishun]

Fixed in libuv. Will be fixed here automatically when libuv is upgraded.

[thisconnect]

Thanks a lot!