Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

unix: Close signal_lock_pipe FDs on unload #875

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

If libuv is being dynamically loaded (and unloaded) as a module, the
global initialization routines actually run multiple times. This allows
the FDs to be closed when the library is unloaded.

(I've signed the CLA already)

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit mnunberg/libuv@d77bd93 has the following error(s):

  • Commit message must indicate the subsystem this commit changes

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

unix: Close signal_lock_pipe FDs on unload
If libuv is being dynamically loaded (and unloaded) as a module, the
global initialization routines actually run multiple times. This allows
the FDs to be closed when the library is unloaded
Contributor

bnoordhuis commented Aug 11, 2013

I think this patch needs some more work. There's a number of style issues but more importantly:

  • It closes fd 0 when uv__signal_lock_pipefd hasn't been initialized (twice, actually!)
  • uv__signal_lock_pipefd is shared across threads. Closing its file descriptors without proper synchronization introduces a window for nasty racy conditions.

It's Sunday morning and I'm still on my first cup of coffee so I haven't thought it through deeply yet but the second issue probably requires some judicious application of atomic operations to get and set the file descriptors.

Member

piscisaureus commented Aug 11, 2013

Curious - do all non-msvc compilers support attribute(destructor) ?

Any cleanup functionality should ensure that all libuv signal handlers are removed before closing the signal pipe and signal lock pipe.
Also, what about the uv_async pipe?

@bnoordhuis If you are unloading the library while libuv is still running you have bigger problems.

Contributor

bnoordhuis commented Aug 11, 2013

Curious - do all non-msvc compilers support __attribute__((destructor)) ?

The ones we care about do: gcc, clang, Sun Studio. I think icc does too. It's a property of the dynamic linker more than anything else, the compiler only needs to pass it on.

If you are unloading the library while libuv is still running you have bigger problems.

I don't disagree. I'm mostly concerned with the scenario where the main thread exits when there are still threads running. The interaction of threads and destructors is only weakly defined so I want to err on the side of caution.

If atomic operations are to be used explicitly, why pthread_once?

In any event, it's wrapped in an #if defined( __GNUC__) in src/unix/threadpool.c.

There's certainly some more things to craft around, like initializing pipefds as { -1, -1 }.

Another solution would be to somehow keep a dlopen(3) reference in this global initialization function... that comes with other issues..

Contributor

bnoordhuis commented Aug 12, 2013

Tentative fix that switches from file descriptors to spinlocks in bnoordhuis/libuv@e009454.

I'll swap this out for some kind of global cross platform unload dtor hook instead. I've detected some other unload issues on Windows..

Contributor

saghul commented Dec 18, 2013

Superseded by #967.

@saghul saghul closed this Dec 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment