Aix, ibmi: Handle server hang when remote sends TCP RST

[V-for-Vasili]

Workaround for getsockname() not working for a TCP handle that has received RST from the remote.

Issue: #3481

[V-for-Vasili]

@vtjnash included your comment suggestion

[vtjnash]

Can you add a test for this? I suspect it is also an issue on macOS (per the man page), and might return ECONNRESET here on FreeBSD (per the man page)

[vtjnash]

(e.g. take an existing shutdown test, and add a call to guess the handle type after the shutdown is sent/received)

[vtjnash]

Semi-relatedly: anyone know how we managed to end up with almost nearly the same code in uv_guess_handle and uv__handle_type for unix? We likely should merge those two functions here, so that we can write aforementioned test.

[vtjnash]

Still needs a test, and to merge this code path with uv_guess_handle

[V-for-Vasili]

Working on a test case; Wasn't able to reproduce given failure by modifying existing shutdown test so far;

Another approach I am thinking of is declaring an additional test helper server, that would work much like tcp4_blackhole_server, except force TCP Reset immediately after new connection is received (by setting connection sockfd SO_LINGER value to 0 and calling close()). I believe this would reproduce the error I observed with Nessus scanner more accurately because connection would likely be already reset by the time uv__handle_type is first called.

[vtjnash]

The echo_server will already call uv_tcp_close_reset if you need it, though this test should not require a separate process.

[vtjnash]

Also, you must first merge the uv__handle_type and uv_guess_handle code paths

[V-for-Vasili]

Please take a look! I had to make TCP RST handling more robust on IBMi by using getsockname(), the same way it is done on Aix; This works for both tcp_rst test and the Nessus security scan; Getsockopt can be unreliable in this case, since getting SO_ERROR value resets the it to 0, meaning if additional getsockopt(SO_ERROR) call is introduced in future patches, uv_guess_handle could become unreliable in this specific case.

[V-for-Vasili]

Looks like uv_guess_handle adjustment for Windows/MSVC needs to be made as well. As of now it apparently returns UV_UNKNOWN_HANDLE after RST as well, based on test_rst output.

[V-for-Vasili]

Interestingly, uv_guess_handle does not return UV_TCP option on Windows - it looks like it is only used to differentiate between UV_TTY, UV_FILE and UV_NAMED_PIPE there: Source.

The method uv_pipe_pending_type() does not rely on uv_guess_handle on Windows the way it does on unix: https://github.com/libuv/libuv/blob/v1.x/src/win/pipe.c#L2505 vs. https://github.com/libuv/libuv/blob/v1.x/src/unix/pipe.c#L315

Would it be better if we skip tcp_rst test on Windows because of the above? (Or use uv_pipe_pending_type call in the test instead of uv_guess_handle, which should be equivalent in either case as far as I understand)

[vtjnash]

Yes, let's make it TEST_SKIP on Win32, since SOCKET is a different underlying type from HANDLE and fd.

[V-for-Vasili]

Skipped on Win32

[V-for-Vasili]

Should I request anyone else to review it as well?

[richardlau]

CI: https://ci.nodejs.org/job/libuv-test-commit/2235/

[V-for-Vasili]

@richardlau fs_event_watch_file_current_dir appears flaky on osx - it failed in # 405 and $ 403 - doesn't look related to this PR

[V-for-Vasili]

fs_event_watch_file_current_dirappears to fail on osx for libuv/libuv v1.x at the moment too: https://ci.nodejs.org/job/libuv-test-commit/2236/

[vtjnash]

Did you mean to call uv_shutdown? Currently, this variable and called_shutdown_cb are never used.

[V-for-Vasili]

Did not mean to call uv_shutdown - only meant to check the handle type in the read callback; Should I make a separate PR to remove shutdown_req & called_shutdown_cb?

[vtjnash]

That would be great

[V-for-Vasili]

Please take a look #3574