Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double initialization of uv_tcp_t (probably any other uv_handle_t) cause random segfaults #3266

Closed
deniszykov opened this issue Aug 6, 2021 · 8 comments
Labels

Comments

@deniszykov
Copy link

Double initialization of uv_tcp_t (probably any other uv_handle_t) cause screw on handle->handle_queue and random segfaults in uv_*_init or uv_close.

I know this is obvious. But it took me a few days to find out in a large codebase that this was happening.
My suggestion is to make a flag that protects against re-initialization and to return an error code if this flag is set. In addition, this will save you from those who try to reuse handle's memory without zeroing it.

  • Version: 1.42.1
  • Platform: Windows
@vtjnash
Copy link
Member

vtjnash commented Aug 6, 2021

this cannot be done, since we make no assumption about the prior state of the memory

@vtjnash vtjnash added the invalid label Aug 6, 2021
@deniszykov
Copy link
Author

deniszykov commented Aug 6, 2021

Ok, docs doesn't mention zeroing memory before init.
At least make a list of what lead to segfault somewhere in FAQ or docs. As far as I know:

  • Allocating handles and requests on stack.
  • Freeing handle memory before close callback.
  • Freeing memory for in-flight requests.
  • Calling any API not from loop thread, except uv_async_send.
  • Double initializing handles.

@vtjnash
Copy link
Member

vtjnash commented Aug 6, 2021

I have proposed fixing this in v2 (#2630), but not yet sure it will get support

@deniszykov
Copy link
Author

You got my thumbs-up :)
Looks safer, and if you need to pass additional info for allocator, this could be done with additional pointer parameter.

uv_tty_new(UV_STDIN_FD, arbitaryPointerForAllocator)

@bnoordhuis
Copy link
Member

W.r.t. #3266 (comment) - I think most are mentioned throughout the docs already, just not in a centralized location.

I somewhat feel they're really just special cases of the general ownership issues you have to think about in C but I won't object to a "do's and don'ts" section in the intro if you want to send a pull request.

@stale
Copy link

stale bot commented Aug 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 28, 2021
@Matheus28
Copy link

Matheus28 commented Sep 4, 2021

For future reference:

Ok, docs doesn't mention zeroing memory before init.

You don't have to zero memory before init

Allocating handles and requests on stack.

This is okay, as long as you run the loop on the same stack or deeper until they're closed (used during tests, can't imagine very many uses of this in real code)

Freeing handle memory before close callback.
Freeing memory for in-flight requests.
Double initializing handles.

I feel like this is already expected with most C APIs

I think a lot of these come from a misunderstanding of how libuv (and most C libraries) treats memory you give it. For uv_*_init, it can't assume anything about it. So it can't check if they're already initialized, etc. Once you have called uv_*_init, you should expect to only be able to free that memory once the callback is called (for requests), or the close callback is called (for handles).

Calling any API not from loop thread, except uv_async_send.

I think there's a mention of this somewhere...

@stale stale bot removed the stale label Sep 4, 2021
@vtjnash vtjnash closed this as completed Sep 5, 2021
@RealYukiSan
Copy link

This is okay, as long as you run the loop on the same stack or deeper until they're closed (used during tests, can't imagine very many uses of this in real code)

So, what's the advantage using heap over stack in this case? I am not too familliar with memory-related subject, please explain to me if you don't mind @Matheus28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants