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.

Sign up for GitHub

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

Jump to bottom

capi: hyper_waker_wake() accepts NULL pointer only to die a horrible death later on #2620

Closed
bagder opened this issue Aug 13, 2021 · 2 comments · Fixed by #2624
Closed

capi: hyper_waker_wake() accepts NULL pointer only to die a horrible death later on #2620

bagder opened this issue Aug 13, 2021 · 2 comments · Fixed by #2624
Labels
A-ffi Area: ffi (C API) C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@bagder
Copy link
Contributor

bagder commented Aug 13, 2021

Version
0.14.11 built from commit 684f2fa

Platform
Linux

Description

I accidentally called hyper_waker_wake() with a NULL pointer argument. That leads to a subsequent segfault deep inside hyper at a later point which makes it difficult to debug. (25 stack frames in from my C code no less).

I expected to see this happen: in a debug-version of hyper maybe an assert should trigger. Or the NULL should just be ignored. It should not lead to a crash much later (when the executor poll is called).
@bagder bagder added the C-bug Category: bug. Something is wrong. This is bad! label Aug 13, 2021
@seanmonstar
Copy link
Member

Heh, yea, kaboom indeed. Several of the functions that return a hyper_code do check for any passed NULLs and return a "bad argument" error. Other functions that don't expect to fail, I could add a debug assert. Silently ignoring may make things worse...

@seanmonstar seanmonstar added A-ffi Area: ffi (C API) E-easy Effort: easy. A task that would be a great starting point for a new contributor. labels Aug 14, 2021
@bagder
Copy link
Contributor Author

bagder commented Aug 14, 2021

Yes, I totally see how this was my fault. It just turned out rather hard to figure out exactly where the problem was since the crash happens so far away from my error.

@seanmonstar seanmonstar mentioned this issue Aug 18, 2021
Merged
seanmonstar added a commit that referenced this issue Aug 18, 2021
@seanmonstar
refactor(ffi): check pointer arguments for NULL (#2624)
3b26572 
This changes all the extern C functions in `hyper::ffi` to check passed
pointer arguments for being `NULL` before trying to use them. Before, we
would just assume the programmer had passed a good pointer, which could
result in segmentation faults. Now:

- In debug builds, it will assert they aren't null, and so if they are,
  a message identifying the argument name will be printed and then the
  process will crash.
- In release builds, it will still check for null, but if found, it will
  return early, with a return value indicating failure if the return type
  allows (such as returning NULL, or `HYPERE_INVALID_ARG`).

Closes #2620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ffi Area: ffi (C API) C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(ffi): check pointer arguments for NULL hyperium/hyper
2 participants
@seanmonstar @bagder