This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix a bug in strstreambuf::overflow
ClosedPublic

Authored by ahatanak on May 17 2016, 12:20 PM.

Details

Summary

The end pointer should point to one past the end of the newly allocated buffer.

Without this fix, asan reports an error when the following code is compiled and executed:

$ cat test.cpp

std::string stringOfLength(const size_t length) {
  std::string s("");

  std::string characters("abcdefghijklmnopqrstuvwxyz0123456789+-*/");
  for (size_t i = 0; i < length; ++i) {
    s += characters[i % characters.size()];
  }

  return s;
}

int main(int, char const **argv) {
  std::ostrstream oss;

  oss << stringOfLength(atoi(argv[1])) << std::ends;
  std::cout << oss.str();
  oss.freeze(false);

  return 0;
}

$ clang++ -fsanitize=address test.cpp && ./a.out 4096

18970==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62100001dd00 at pc 0x00010277c45d bp 0x7fff5d4ce6e0 sp 0x7fff5d4cdea0

READ of size 4097 at 0x62100001dd00 thread T0

#0 0x10277c45c in wrap_strlen (libclang_rt.asan_osx_dynamic.dylib+0x4345c)
#1 0x102733954 in std::__1::char_traits<char>::length(char const*) (a.out+0x100002954)
#2 0x10273390b in std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::operator<<<std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*) (a.out+0x10000290b)
#3 0x102733346 in main (a.out+0x100002346)
#4 0x7fff901905ac in start (libdyld.dylib+0x35ac)
#5 0x1  (<unknown module>)

0x62100001dd00 is located 0 bytes to the right of 4096-byte region [0x62100001cd00,0x62100001dd00)
allocated by thread T0 here:

#0 0x10278d42b in wrap__Znam (libclang_rt.asan_osx_dynamic.dylib+0x5442b)
#1 0x7fff9bdc9fa1 in std::__1::strstreambuf::overflow(int) (libc++.1.dylib+0x44fa1)
#2 0x7fff9bd901cc in std::__1::basic_streambuf<char, std::__1::char_traits<char> >::xsputn(char const*, long) (libc++.1.dylib+0xb1cc)
#3 0x10273547c in std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> > std::__1::__pad_and_output<char, std::__1::char_traits<char> >(std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> >, char const*, char const*, char const*, std::__1::ios_base&, char) (a.out+0x10000447c)
#4 0x102734312 in std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::__put_character_sequence<char, std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*, unsigned long) (a.out+0x100003312)
#5 0x10273389d in std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::operator<<<char, std::__1::char_traits<char>, std::__1::allocator<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) (a.out+0x10000289d)
#6 0x1027332c4 in main (a.out+0x1000022c4)
#7 0x7fff901905ac in start (libdyld.dylib+0x35ac)
#8 0x1  (<unknown module>)

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 57507.May 17 2016, 12:20 PM
ahatanak retitled this revision from to [libcxx] Fix a bug in strstreambuf::overflow.
ahatanak updated this object.
ahatanak added a subscriber: cfe-commits.
bcraig added a subscriber: bcraig.May 24 2016, 11:10 AM

I don't believe this is a libcxx bug, but it is a bug in the test code. oss.str(); isn't required to return a null terminated string. std::cout << (char *) requires a null terminated string though.

My understanding is that typically std::ends is explicitly appended to null-terminate the stream buffer. The test case in the example does that.

http://en.cppreference.com/w/cpp/io/ostrstream/str

My understanding is that typically std::ends is explicitly appended to null-terminate the stream buffer. The test case in the example does that.

http://en.cppreference.com/w/cpp/io/ostrstream/str

Doh! You're right.

I'm still confused though, so please entertain my blind fumblings.

ASAN is complaining about an excessively large read. If the problem was in overflow, I would expect ASAN to complain about an out-of-bounds write instead.

ASAN is complaining about an excessively large read. If the problem was in overflow, I would expect ASAN to complain about an out-of-bounds write instead.

According to the example shown in the link below, ASAN is complaining about an out-of-bounds read:

https://github.com/google/sanitizers/wiki/AddressSanitizerExampleHeapOutOfBounds

This happens because epptr is not set correctly in strstreambuf::overflow. This causes the null terminator to be written to the wrong location, which causes strlen to keep reading the characters past the end of the allocated block.

strstreambuf::overflow is allocating a memory block that is sufficiently large, so ASAN doesn't complain about an out-of-bounds write.

So I definitely see a problem, and I think your fix is a reasonable solution to that problem. I'm still unclear on how that caused the ASAN error though. Here's my understanding of the current problem...

The initial buffer for strstreambuf is often 4096 ( _strstream> line 184, __default_alsize = 4096;).
So we have a 4096 character string, and push that into strstreambuf. There shouldn't be any calls to overflow because of that.
Now, we push in an ends(). This causes a call to overflow(). overflow() allocates a new buffer of twice the size, then saves off the relevant put and get range sizes for the old buffer. The old capacity ranges (einp and eout) should add up to 4096. We then set the new range sizes using the old range sizes. THIS SEEMS WRONG. We have an 8K buffer, but we are only setting it with 4K sizes. We then write ends() into the no-mans-land past the end of epptr() (well, exactly at epptr), and then advance pptr() past epptr().

I can see this causing a lot of trouble, as some code compares pptr() == epptr(), and we've broken that.

However, I would expect ASAN to require an extra call to overflow() before things explode. strlen should still find that end() that we placed. Maybe there's an extra overflow() that I didn't see though.

I would like to see a new test added to exercise this use case. The test should probably live somewhere under test/std/depr/depr.str.strstreams/depr.strstreambuf. Your example code is going to be pretty close to what you would want to put there (maybe a string.resize instead of the stringOfLength() function you have). I believe you can run the tests using address sanitizer by setting LLVM_USE_SANITIZER="Address" to your cmake line. "make check-libcxx" will then compile the tests (and libcxx?) with ASAN.

src/strstream.cpp
174 ↗(On Diff #57507)

This is now unused.

ahatanak updated this revision to Diff 58740.May 26 2016, 6:30 PM

Remove unused variable and add test case.

ahatanak marked an inline comment as done.May 26 2016, 6:42 PM

I spent some time debugging the code and here is what I found.

The initial buffer size is 0 when strstreambuf is constructed and all six pointers are null initially. When the first character is pushed, strstreambuf::overflow allocates 4096B (which is the value of __default_alsize) and initializes the six pointers to the address of "buf". Then it bumps the current put character pointer (pptr). The other five pointers don't change. strstreambuf::overflow gets called 4095 more times and pptr is incremented every time. No new memory blocks are allocated while this happens because pptr() != epptr() after the first character is pushed.

It seems that std::ends does get written to the right location (meaning buf+4096). I'm thinking ASAN doesn't catch this as an out-of-bound write because I'm using libc++ that is not instrumented. It catches the out-of-bound read because the call to strlen is intercepted (wrap_strlen) is called.

mclow.lists edited edge metadata.May 26 2016, 9:05 PM

Let's file a bug on this too - http://llvm.org/bugs (make it easier to find in the future)

LGTM. You may want to wait on Marshall's approval though. Thanks for being patient with me, and thanks for the patch!

Thanks!

Marshall, I've filed a PR for this bug:
https://llvm.org/bugs/show_bug.cgi?id=27915

The fix looks fairly obvious, but you haven't added a testcase. Would you please do so?

I agree with Ben that this looks good.

Marshall and Eric, do you want Akira to hold off, or are you happy deferring to Ben and me?

mclow.lists accepted this revision.Jun 29 2016, 5:56 AM
mclow.lists edited edge metadata.

This looks fine to me.

This revision is now accepted and ready to land.Jun 29 2016, 5:56 AM
This revision was automatically updated to reflect the committed changes.