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

Null pointer dereference in patched wepoll code #85

Closed
notgull opened this issue Feb 12, 2023 · 4 comments · Fixed by #88
Closed

Null pointer dereference in patched wepoll code #85

notgull opened this issue Feb 12, 2023 · 4 comments · Fixed by #88

Comments

@notgull
Copy link
Member

notgull commented Feb 12, 2023

See piscisaureus/wepoll#32. It looks like the patched wepoll code has a null pointer dereference for when the poller is notified.

@notgull
Copy link
Member Author

notgull commented Feb 12, 2023

From a cursory glance, this issue probably comes from using the distribution wepoll with the patched version that the polling crate uses. polling patches wepoll so that a null completion packet can be used to wake up an ongoing poll operation, which is necessary for futures to be able to run themselves. If the distribution wepoll is overriding the patched wepoll, this means that a null packet sent by polling will cause a segmentation fault.

I've opened this here because we're planning on moving away from wepoll anyhow, see #22 and #76 (although I'm currently working on an alternative PR that should be easier to debug).

@taiki-e
Copy link
Collaborator

taiki-e commented Feb 16, 2023

Um, did the distributor disable wepoll-ffi because of symbol conflicts with the original wepoll?

@notgull
Copy link
Member Author

notgull commented Feb 16, 2023

The issue comes down to this:

  • A program compiles in polling. polling brings in a modified version of wepoll that's programmed to handle null OVERLAPPED wake-up's.
  • The program also links libzmq statically. libzmq embeds the unpatched version of wepoll, without support for null OVERLAPPED wakeups.
  • There are two versions of wepoll being compiled into the program. The linker chooses the distribution wepoll, without null wakeups support.
  • During runtime, polling uses a null OVERLAPPED wakeup to notify wepoll. However, the current version of wepoll does not expect a null OVERLAPPED. It reads from the null pointer and causes a segfault.

A fix for this could be renaming all of the wepoll functions so that they are unique and won't conflict with distribution wepoll symbols. However, I'd rather spend my time and energy getting #88 working.

@taiki-e
Copy link
Collaborator

taiki-e commented Feb 16, 2023

Thanks for the explanation! I agree that removing the dependency on C is the best solution here.

There are two versions of wepoll being compiled into the program. The linker chooses the distribution wepoll, without null wakeups support.

It is unfortunate that the linker does not report an error in this case.

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

Successfully merging a pull request may close this issue.

2 participants