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

string buffer overflow #1222

Merged
merged 2 commits into from Jan 11, 2017

Conversation

@m-mcgowan
Copy link
Contributor

commented Dec 29, 2016

fixes bug in String(const char* str, int len) constructor when the string is longer than the specified length. Instead of extracting just the specified substring, it copies the full string to the smaller buffer, corrupting the heap beyond the buffer.


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation (N/A)
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Bugfixes

  • [PR #1222] Fixed bug in String(const char* str, int len) constructor when the string is longer than the specified length.
fixes bug in String(const char* str, int len) constructor when the st…
…ring is longer than the specified length. Instead of extracting just the specified substring, it copies the full string to the smaller buffer, corrupting the heap beyond the buffer.

@sergeuz sergeuz self-assigned this Dec 29, 2016

@sergeuz sergeuz added this to the 0.6.1 milestone Dec 29, 2016

@@ -217,7 +217,8 @@ String & String::copy(const char *cstr, unsigned int length)
return *this;
}
len = length;
strcpy(buffer, cstr);
memcpy(buffer, cstr, length);
buffer[len] = 0;

This comment has been minimized.

Copy link
@harrisonhjones

harrisonhjones Dec 29, 2016

Contributor

Might it not be clearer if it was buffer[len] = '\0';? My C/C++ knowledge is woefully behind y'all's so sorry if the suggestion is silly.

This comment has been minimized.

Copy link
@technobly

technobly Dec 30, 2016

Member

They are the same thing, but I agree most people see a null terminator and know what's going on without thinking about it. Sometimes the way to go though is to match the style used in the existing file instead of applying your own style here and there. In that light, most of the file uses = 0; and there are only two instances of = '\0';

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Dec 30, 2016

Author Contributor

The problem isn't null terminating the local buffer, but that it's copying too much data into the buffer. We can't null terminate the cstr at the given length since that's input data (and const).

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Dec 30, 2016

Author Contributor

Oh, I see what you mean, Using a char instead of an int. Sure we can do that.

@technobly

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

Nice catch Mat! Looks good and fixed and I'd love to see a test added to keep it that way.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

I was unsure how to test it when I found the bug (how to test corrupt heap?) but on reflection it's simple.

If we can pre-size the internal buffer to be larger than the input string (say 10 chars) and then provide a string that is 7 chars long, with a length of 5, then we verify that only 5 chars were copied to the buffer, and not 7. (And with the buffer being larger, there is no memory corruption so we don't corrupt the test runner.)

I'll add this test when I'm back from vacation (if someone doesn't beat me to it.)

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

Nice catch, Mat. I'll add the test, since, after all, I was the one who added that constructor to the String class. Couldn't imagine String::copy() was implemented in such a weird way though.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

Thank you for for stepping up - much appreciated! Yes, it's surprising that the provided String implementation makes a textbook error, but these things are easy to do, and just as easy to overlook in a review too. Case in point - it took me several hours to track down the bug, it was far from trivial even though the fix is. I'm glad we provide the String class so that the insidiousness of C strings is encapsulated and wrapped to save users from shooting themselves in the foot.

While you're working on this, could you check with the arduino String implementation to see if there are any bugfixes made since our code was forked. There might be other bombs lurking waiting to go off :-)

@technobly technobly added the bug label Jan 3, 2017

@sergeuz

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Hey @technobly, I've added API tests for the String class, as per your 0.6.1-rc.1 merging notes.

As for the unit test discussed above, at closer look I don't see how it can be implemented without changing String API, since the bug is in String::copy() method, which is not public, nor can be tested indirectly via other methods in this particular case.

Considering that current coverage of String class by unit tests is very limited anyway, maybe let's merge this PR as it is? It still would be good to review the implementation later, e.g. after releasing rc2, and, as Mat suggested, backport possible fixes from Arduino.

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Sounds good, I captured this in the merging notes for 0.6.1-rc.2

@technobly technobly merged commit aa58936 into develop Jan 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/string_heap_corruption branch Jan 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.