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

Avoids buffer overflow in xmlrpcpp if fd > 1024 #1113

Closed
wants to merge 1 commit into from

Conversation

kmactavish
Copy link
Contributor

@kmactavish kmactavish commented Jul 28, 2017

A temporary fix to avoid buffer overflow in XmlRpcDispatch, since select() only works for file descriptors < FD_SETSIZE (1024). If fd >= FD_SETSIZE, there is buffer overflow, memory corruption, and all-round undefined behaviour. The real solution is to switch to poll as in #833, but since that PR was reverted in #981, this avoids catastrophic failure until that can be worked out.

It throws an exception since:

  • this behaviour is no less intrusive than a buffer overflow
  • returning an XmlRpcError was handled silently, and just looked like ros comm not being connected
  • this is a temporary solution to fail in a more predictable way.

@kmactavish
Copy link
Contributor Author

kmactavish commented Jul 28, 2017

The build of 582984f failed due to a test added by #1014. Even with this entire change commented out.

08:55:05 [test_roscpp.rosunit-timer_callbacks/multipleSteadyTimeCallbacks][FAILURE]------
08:55:05 /tmp/catkin_workspace/src/ros_comm/test/test_roscpp/test/src/timer_callbacks.cpp:220
08:55:05 Failed

A temporary fix to avoid buffer overflow in XmlRpcDispatch, since select() only works for file descriptors < FD_SETSIZE (1024). The real solution is to switch to poll as in ros#833, but since that PR was reverted, this avoids catastrophic failure. It throws instead of being silent, since (a) this behaviour is no less intrusive than a buffer overflow, and (b) returning an error was handled silently, and just looked like ros comm not being connected.
@kmactavish
Copy link
Contributor Author

This was superseded by #1133 .

@kmactavish kmactavish closed this Aug 14, 2017
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.

None yet

1 participant