Skip to content

Conversation

joelposti
Copy link

As described in the issue #460, isahc::agent::AgentContext::register returns a std::io::Result::Err, if isahc::agent::selector::is_bad_socket_error returns false. isahc::agent::AgentContext::poll then calls the std::io::Result::unwrap method on the Err variant. This causes a panic and that in turn causes a segmentation fault. I think there two bigger issues here:

  1. panic handling should be improved so that a panic does not cause a segmentation fault,
  2. and isahc::agent::AgentContext::poll should not call unwrap.

This pull request is not a fix for either of those issues. Instead, it is a proposal for a minimal change that fixes one case where issues 1 and 2 manifest.

I am unsure, if this is a good fix. I am not familiar with the polling backends that polling uses. I do not know what it means for poller.add and/or poller.modify to return a std::io::Result::Err variant whose std::io::Error::kind method returns a std::io::ErrorKind::PermissionDenied variant. But the panic and the subsequent segmentation fault are prevented, if is_bad_socket_error returns true in that case.

Possible ramifications of returning true on std::io::ErrorKind::PermissionDenied are unknown to me. What if poller.add or poller.modify constantly return PermissionDenied due to insufficient user privileges? Is it possible for that to cause an infinite loop of trying to call poller.add or poller.modify and them returing PermissionDenied over and over again?

The segfault integration test and the associated run-segfault-integration-test-repeatedly.sh script, available in the pull request #461, can be used to test that the fix introduced in this pull request prevents a panic and the subsequent segmentation fault.

…ad_socket_error` return `true` on `std::io::ErrorKind::PermissionDenied`. `poller.add` and/or `poller.modify` return `std::io::ErrorKind::PermissionDenied` in certain conditions.
@sagebind
Copy link
Owner

Thanks for looking at this! I can't review this immediately, but should have time to in the coming week. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants