Fix various memory issues and enable ASAN for the test suite #380

D4N · 2018-07-09T03:57:40Z

Thanks to the amazing work by @piponazo in #367 I was able to track down the last buffer overflows and memory leaks that were present which the test suite would pick up.
I have fix all issues that I could find and enabled address sanitizer on travis, so that all commits causing memory issues will immediately break the build. I did not enable undefined behavior sanitizer yet, as it still picks up some conversion issues.

Summary

Add DataBuf::free(): new api function that works just like release(), but does not return anything and frees the internal buffer. release() was missused in a few places (the return value can be ignored, causing a memory leak), thus I have marked it with a new macro EXV_WARN_UNUSED_RESULT
Reorder some memory accesses, so that a dereference happens always after the range check.
Add a custom compare function to pngimage.cpp, where a bunch of memcmp calls would result in buffer overflows.
Add padding bytes to an array, as it was too short

piponazo

Thanks Dan for your contributions. Please take a look to my comments.

piponazo · 2018-07-09T19:11:22Z

src/iptc.cpp

@@ -354,7 +354,10 @@ namespace Exiv2 {
 		while  ( i < size-3 && bytes[i] != 0x1c ) i++;
 		depth++;
 		out << Internal::indent(depth) << "Record | DataSet | Name                     | Length | Data" << std::endl;
-		while ( bytes[i] == 0x1c && i < size-3 ) {
+		while ( i < size-3 ) {


The indentation now looks weird in this function. The original code was using tabs and now you started to use spaces. Would you mind to convert all these tabs in spaces?

I'll change that.

I have clang-formatted the whole function in a separate commit.

piponazo · 2018-07-09T19:20:22Z

src/pngimage.cpp

@@ -158,9 +158,12 @@ namespace Exiv2 {
        long        count=0;
        const byte* p = bytes ;
        // header is \nsomething\n number\n hex
-        while ( count < 3 )
-            if ( *p++ == '\n' )
+        while ((count < 3) && (p - bytes < length)) {


oh god. This code is scary. Were you able to make this code fail before trying to fix it?

I could not understand what you are trying to do here (neither what the original code is doing).

Yep, it creates a heap buffer overflow with asan by simply running the test suite.

I'll try to comment it better. The first for loop is actually only responsible for incrementing the pointer p over three \n (but it wasn't subtracting that from the length, so the second loop was using the original length)... yeah, not the easiest code to understand.

I have added some comments explaining that part of the code.

piponazo · 2018-07-09T19:26:07Z

src/pngimage.cpp

+{
+    inline bool compare(const char* str, const Exiv2::DataBuf& buf, size_t length)
+    {
+        assert(strlen(str) <= length);


This check would only happen in Debug mode (which is fine for me). Is that what you had in mind?

Yes, the function is used only in contexts where length and str are known at compile time, so that is fine.

I have added a comment into the code explaining that.

piponazo · 2018-07-09T19:33:46Z

ci/run.sh

@@ -15,6 +15,9 @@ fi

 mkdir build && cd build
 conan install .. --build missing --profile release
+export CFLAGS='-fsanitize=address'


[minor] I think it would it make more sense to set these flags in the CMake invocation. Basically you would need to add to CMAKE_OPTIONS the following:

-DCMAKE_CXX_FLAGS="-fsanitize=address" -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" -DCMAKE_MODULE_LINKER_FLAGS="-fsanitize=address"

In that way, we would not be modifying any environment variable.

Oh, didn't know that was possible. I usually use CXXFLAGS, CFLAGS and LDFLAGS since nearly every build systems picks them up.

DataBuf::release() easily cause memory leaks, when the return value is ignored. free() provides the desired behavior, when the internal data should just be deleted and not used further.

The buffer array is not deleted, when an exception is thrown (happens for nBytes< 0). => use std::vector<char> instead

The loop condition will perform a range check correctly, but it will always dereference bytes[i], even if i is too large and fails the second check. => move the bytes[i] == 0x1c check into a if, after the range check was successfull

The pointer p is advanced in the while loop to step over three '\n'. However, its length is never reduced accordingly. => the length check in the following for loop is invalid, as it permits overreading by the number of characters that p was advanced by.

memcmp() compares the read data from key with the provided string, but when key.pData_ is shorter than the provided length, then memcmp can read beyond the bounds of key.pData_ => add custom compare function, which ensures that we never read more than key.size_

in the following call: getHeaderOffset (payload.pData_, payload.size_, (byte*)&exifLongHeader, 6); getHeaderOffset would read 6 bytes from exifLongHeader, reading beyond the bounds of the array => add 2 padding bytes to prevent overreads

EXV_WARN_UNUSED_RESULT is a conditional macro that expands to either __attribute__((warn_unused_result)) on gcc & clang or to _Check_return for MSVC => Compiler warns if the return value is ignored

D4N requested review from piponazo and clanmills July 9, 2018 03:57

piponazo reviewed Jul 9, 2018

View reviewed changes

D4N added 10 commits July 28, 2018 23:50

[types] Add DataBuf::free()

1b74db8

DataBuf::release() easily cause memory leaks, when the return value is ignored. free() provides the desired behavior, when the internal data should just be deleted and not used further.

[DataBuf] Replace wrong usage of release() with free()

607b191

Remove memory leak in SshIo::SshImpl::getDataByRange

39399ac

The buffer array is not deleted, when an exception is thrown (happens for nBytes< 0). => use std::vector<char> instead

[IptcData::printStructure] Remove buffer overrun

b2c3b61

The loop condition will perform a range check correctly, but it will always dereference bytes[i], even if i is too large and fails the second check. => move the bytes[i] == 0x1c check into a if, after the range check was successfull

[IptcData::printStructure] clang-format function

67dc3e6

Add EXV_WARN_UNUSED_RESULT macro & add it to DataBuf::release()

b12a868

EXV_WARN_UNUSED_RESULT is a conditional macro that expands to either __attribute__((warn_unused_result)) on gcc & clang or to _Check_return for MSVC => Compiler warns if the return value is ignored

[travis] Enable ASAN for the test suite

664e93c

D4N force-pushed the misc_issues_fix branch from 0092bde to 664e93c Compare July 28, 2018 22:20

piponazo approved these changes Jul 29, 2018

View reviewed changes

D4N merged commit 24ef91f into Exiv2:master Jul 29, 2018

D4N deleted the misc_issues_fix branch July 29, 2018 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various memory issues and enable ASAN for the test suite #380

Fix various memory issues and enable ASAN for the test suite #380

D4N commented Jul 9, 2018

piponazo left a comment

piponazo Jul 9, 2018

D4N Jul 9, 2018

D4N Jul 28, 2018

piponazo Jul 9, 2018

D4N Jul 9, 2018

D4N Jul 28, 2018

piponazo Jul 9, 2018

D4N Jul 9, 2018

D4N Jul 28, 2018

piponazo Jul 9, 2018 •

edited

Loading

D4N Jul 9, 2018

Fix various memory issues and enable ASAN for the test suite #380

Fix various memory issues and enable ASAN for the test suite #380

Conversation

D4N commented Jul 9, 2018

Summary

piponazo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo Jul 9, 2018 •

edited

Loading