Fix for buffer overflow on read failure#11
Merged
Conversation
Owner
|
Thank a lot for your PR and throurough explanation. I'll look into it after the weekend. |
eelcocramer
added a commit
that referenced
this pull request
Sep 2, 2013
Fix for buffer overflow on read failure
Owner
|
Pulled. Good find & fix :-) Thanks for the PR. I will publish a new version to npm later today. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On linux, whenever a connection is already established and the read operation on the file descriptor is unsuccessful (e.g. the connection was reset or other), the application crashes with a segmentation fault (e.g. Program received signal SIGSEGV, Segmentation fault). Debugging a little bit more through gdb, I discovered the following...
Problem:
In function BTSerialPortBinding::EIO_Read for src/linux/BTSerialPortBinding.cc:163, we assign the number of bytes read by the read(...) to baton->size. However, upon the operation failing the bytes read return is -1. A couple of lines below in the same function, src/linux/BTSerialPortBinding.cc:169, we copy the data read and stored in buf into baton->result. However, in the case the read operation failed baton->size == -1. This causes memcpy to be invoked with a -1 as its count value. The count parameter is actually defined as a size_t while baton->size happens to be an int. Passing the -1 (int) into memcpy causes the count (size_t is actually unsigned) value to wrap around and become a really large value (integer overflow). In turn, when the copy function refers to an index outside either buffers we overflow because we are referring to a memory location larger than 1024 (which is the max size for either).
Solution:
Luckily the solution is simple. We just need to add a check to determine if the size is non-negative. If it is not, then we proceed to copy. Otherwise, we don't. I tried this fix with the application that I used to uncover this issues and confirmed that this works as expected.