win: Fix pipe resource leak if closed during connect (and other bugs) #3611

vtjnash · 2022-04-28T17:34:00Z

test.parallel/test-http-client-abort-unix-socket timeout

this seems somewhat concerning and needs to be investigated

Investigation revealed this to be a somewhat uncommon resource leak. There is a relatively direct fix for that of just adding this to the happy path of uv__process_pipe_connect_req:

    if (handle->flags & UV_HANDLE_CLOSING)
      close_pipe(handle);

But that might risk hanging for a long time in the background thread waiting for the 30 second (recurring) timeout. Thus I have reordered the logic to check and set the flag early (uv__pipe_connection_init ) that is required for correct cleanup. The old code also had a significant data race mistake, which we fix here by introducing new fields into the connect_req object. There was also a variety of other resource management bugs, which turned out to make this a lot larger than initially expected.

n.b. the commits are loosely separated, but not independently functional and should be squashed

If uv_close was called while connect was pending, we would fail to release the resources for the connection, since we had not yet set the type of the struct.

Only permitted to write to `req` on threads, as anything else causes data race corruption.

vtjnash · 2022-04-29T04:12:19Z

Attempting CI+node: https://ci.nodejs.org/view/libuv/job/libuv-in-node/213/

vtjnash · 2022-05-02T18:55:22Z

Bump. This is release-blocking currently, so happy to jump on a call with someone to walk them through the logic design here and review it together. Then we can continue with the current release process effort.

CI+node seems happy with this: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/14534/

vtjnash · 2022-05-02T18:59:30Z

n.b. this does not address any part of #3578, even though it is fixing some other data races in the same vicinity

bnoordhuis

I was hoping someone more familiar with win32 would review but LGTM insofar I can be the judge of that.

include/uv/win.h

src/win/pipe.c

Co-authored-by: Ben Noordhuis <[email protected]>

src/win/pipe.c

Co-authored-by: Ben Noordhuis <[email protected]>

If `uv_close` was called while a connect was pending, we would fail to release the resources for the connection, since we had not yet set the type of the struct. Fix a thread data-race on slow connect path code: only permitted to write to `req` on threads, as anything else causes data race corruption. There seemed be a small variety of other resource management bugs in edge cases, which turned out to make this a lot larger than initially expected. Refs: libuv#3598 (comment)

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: nodejs#42290

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: #42290 PR-URL: #42340 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: nodejs#42290 PR-URL: nodejs#42340 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: #42290 PR-URL: #42340 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

vtjnash added 2 commits April 28, 2022 11:50

pipe,win: fix resource leak

3730cda

If uv_close was called while connect was pending, we would fail to release the resources for the connection, since we had not yet set the type of the struct.

pipe,thread: fix thread race on slow connect

42432b6

Only permitted to write to `req` on threads, as anything else causes data race corruption.

vtjnash mentioned this pull request Apr 28, 2022

Release proposal: v1.44.2 #3586

Closed

11 tasks

fix yet more resource cleanup and error handling mistakes

a2ebb24

vtjnash force-pushed the jn/pipe-leaks-win branch from 3558f90 to a2ebb24 Compare April 28, 2022 19:24

fixup! fix yet more resource cleanup and error handling mistakes

ee482e3

bnoordhuis approved these changes May 10, 2022

View reviewed changes

include/uv/win.h Show resolved Hide resolved

src/win/pipe.c Outdated Show resolved Hide resolved

src/win/pipe.c Outdated Show resolved Hide resolved

src/win/pipe.c Outdated Show resolved Hide resolved

src/win/pipe.c Show resolved Hide resolved

Apply suggestions from code review

bfa1eee

Co-authored-by: Ben Noordhuis <[email protected]>

vtjnash commented May 10, 2022

View reviewed changes

src/win/pipe.c Outdated Show resolved Hide resolved

Apply suggestions from code review

15b79fe

Co-authored-by: Ben Noordhuis <[email protected]>

vtjnash merged commit ec67735 into libuv:v1.x May 13, 2022

vtjnash deleted the jn/pipe-leaks-win branch May 13, 2022 10:40

lpinca mentioned this pull request Jul 12, 2022

deps: upgrade to libuv 1.44.2 nodejs/node#42340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

win: Fix pipe resource leak if closed during connect (and other bugs) #3611

win: Fix pipe resource leak if closed during connect (and other bugs) #3611

Uh oh!

vtjnash commented Apr 28, 2022

Uh oh!

vtjnash commented Apr 29, 2022

Uh oh!

vtjnash commented May 2, 2022

Uh oh!

vtjnash commented May 2, 2022

Uh oh!

bnoordhuis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

win: Fix pipe resource leak if closed during connect (and other bugs) #3611

win: Fix pipe resource leak if closed during connect (and other bugs) #3611

Uh oh!

Conversation

vtjnash commented Apr 28, 2022

Uh oh!

vtjnash commented Apr 29, 2022

Uh oh!

vtjnash commented May 2, 2022

Uh oh!

vtjnash commented May 2, 2022

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants