Fix problems detected by "-fsanitize=address" - #358
Merged
Conversation
Contributor
Author
|
.. updated with a version that tries to work around non-c99 compliance in your windows toolchain |
Contributor
Author
|
I'm no longer confident that these fixes account for the test failures, since #359 affected my testing methodology. However, I believe the fixes are still right. |
mpictor
reviewed
Aug 19, 2017
|
|
||
| static int ERROR_vprintf( const char *format, va_list ap ) { | ||
| int result = snprintf( ERROR_string, ERROR_string_end - ERROR_string, format, ap ); | ||
| if(result < 0) ERROR_string = ERROR_string_end; |
Member
There was a problem hiding this comment.
Please add braces to match our coding style guidelines -
if {
/*...*/
} else if {
/*...*/
} else {
/*...*/
}This is no security library, but I still don't want to goto fail ;)
mpictor
reviewed
Aug 19, 2017
| } | ||
|
|
||
| static void ERROR_nexterror() { | ||
| if( ERROR_string == ERROR_string_end ) return; |
Member
|
Thanks for your contributions, Jeff! I've added two comments to the code, of changes I'd like to see. |
Contributor
Author
|
@mpictor thanks for your comments. I've updated the branch as you suggest. |
Contributor
Author
|
whoops it's actually #360 I've pushed the revisions for. Revisions for this PR are coming soon. |
On Debian Stretch, when configuring stepcode like so:
ASAN_OPTIONS="detect_leaks=false" CXX="clang++" CXXFLAGS="-fsanitize=address" cmake ..
a fatal error would be detected:
==29661==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x62100001dca0 at pc 0x0000004435e3 bp 0x7ffed6d9cae0 sp 0x7ffed6d9c290
READ of size 4001 at 0x62100001dca0 thread T0
#0 0x4435e2 in __interceptor_strlen.part.45 (/home/jepler/src/stepcode/build/bin/schema_scanner+0x4435e2)
#1 0x501d7b in ERRORreport_with_symbol /home/jepler/src/stepcode/src/express/error.c:413
0x62100001dca0 is located 0 bytes to the right of 4000-byte region
[0x62100001cd00,0x62100001dca0)
allocated by thread T0 here:
#0 0x4c3ae8 in __interceptor_malloc (/home/jepler/src/stepcode/build/bin/schema_scanner+0x4c3ae8)
#1 0x5011fc in ERRORinitialize /home/jepler/src/stepcode/src/express/error.c:129
Operations on ERROR_string were unsafe, because they did not guard
against accesses beyond the end of the allocatd region.
This patch ensures that all accesses via *printf functions do respect
the end of the buffer; and encapsulates the routine for pointing
ERROR_string at the space for the next error text to start, if space is
available.
Finally, because it was found with search and replace, a stray manipulation
of ERROR_string within the print-to-file branch of the code is removed.
This stray line would have had the effect of moving ERROR_string one byte
further along at every warning-to-file, which could also have been a
cause of the problem here.
The idiom
char c = ...;
_userMsg.append( &c );
is not correct C++, because it treats the address of 'c' as a NUL-
terminated C string. However, this is not guaranteed.
When building and testing on Debian Stretch with AddressSanitizer:
ASAN_OPTIONS="detect_leaks=false" CXX="clang++" CC=clang CXXFLAGS="-fsanitize=address" LDFLAGS="-fsanitize=address" cmake .. -DSC_ENABLE_TESTING=ON -DSC_BUILD_SCHEMAS="ifc2x3;ap214e3;ap209"
ASAN_OPTIONS="detect_leaks=false" make
ASAN_OPTIONS="detect_leaks=false" ctest . --output-on-failure
an error like the following is encountered:
==15739==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffeb2ca7621 at pc 0x00000043c943 bp 0x7ffeb2ca75d0 sp 0x7ffeb2ca6d80
READ of size 33 at 0x7ffeb2ca7621 thread T0
#0 0x43c942 in __interceptor_strlen.part.45 (/home/jepler/src/stepcode/build/bin/lazy_sdai_ap214e3+0x43c942)
#1 0x7fb9056e6143 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::append(char const*) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0x11f143)
#2 0x7fb905b677c3 in ErrorDescriptor::AppendToDetailMsg(char) /home/jepler/src/stepcode/src/clutils/errordesc.cc:150:5
Address 0x7ffeb2ca7621 is located in stack of thread T0 at offset 33 in frame
#0 0x7fb905b676af in ErrorDescriptor::AppendToDetailMsg(char) /home/jepler/src/stepcode/src/clutils/errordesc.cc:149
This frame has 1 object(s):
[32, 33) '' <== Memory access at offset 33 overflows this variable
A similar problem with AppendToUserMsg is found by inspection.
After this change, all 200 tests pass under the AddressSanitizer
configuration
Contributor
Author
|
OK now this PR is updated |
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
2 participants
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.
It was mentioned on #357 that the appveryor build has never worked. This made me interested in the testsuite, though of course I tested on Linux where the lighting is good. Specifically, I used clang++ 3.8.1 on Debian Stretch in "AddressSanitizer" mode. This mode can find many accesses-past-end-of-buffer. These patches fix the non-leak errors encountered when building as shown:
Before the change,
though unfortunately the generate_cpp_sdai_ap214e3 test which failed on windows did not fail under AddressSanitizer, so there's no reason to think I fixed that.