[libcxx] Fix crash in std::stringstream with payload >= INT_MAX

[azat]

Summary

stringstream does works for payload > INT_MAX, however
stringstream::gcount() can break the internal field (__nout_) and this
breaks the stringstream itself, and so the program will crash.

Fix this, by using __pbump(streamsize) over pbump(int)

Note, libstdc++ does not have this bug.

[azat]

Update commit message

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

24

I'm not sure that the test that allocates 2GiB of RAM is good for your CI, so please let me know.

[azat]

Fix test for 32-bit (check SIZE_WIDTH)

[azat]

Test failures looks unrelated, can someone take a look at the patch, @ldionne you can take a look ?
Thanks in advance!

[philnik]

I have no idea how the stream stuff is implemented, so I can't say whether the implementation changes look good. I've got a few nits in the test.

In D146294#4206359, @azat wrote:

Test failures looks unrelated, can someone take a look at the patch, @ldionne you can take a look ?
Thanks in advance!

Yes, the failures look unrelated.

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

11–12

19

This seems unused.

24

This should be fine. It's probably not actually allocated anyways.

[azat]

Fix review notes in the test

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

24

Sadly but it will allocate 2GiB of RAM

[azat]

Just a friendly reminder, can someone please take a look?

[azat]

@ldionne maybe you could take a look? (Looks like you've reviewed few patches to sstream in the past) Thanks in advance!

[Mordante]

Do you have a way to reproduce the original bug? I'm a bit curious how this fixes it.

libcxx/include/sstream

475

This type is most likely std::size_t, and __pbump takes a streamsize which is signed. Why is this overflow save?

[azat]

@Mordante

Do you have a way to reproduce the original bug?

Yes sure, you can look at the test, without this patch it will SIGSEGV or something

I'm a bit curious how this fixes it.

The problem was that gcount() reinitialize some internal fields, but it works only for INT_MAX not more, since it uses pbump(int) over __pbump(streamsize), which leaves the __nout_ in invalid state (overflowed int).

[azat]

libcxx/include/sstream

475

Yeah, you are right, so it seems that both places needs this while loop, or maybe I could introduce another helper instead of using this loop? (asking because I'm not sure about adding new symbols to the libc++, since this can change ABI, though it is protected)

[azat]

Remove __pbump() usage in basic_stringbuf<>::str() as catched by @Mordante

[azat]

libcxx/include/sstream

475

Actually only this hunk is wrong, so I simply remove this change from the patch, to keep this bug fix as simple as possible.

[azat]

It looks like CI is broken?

+ tee /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-libcxx-precommit-ci-group-0grb-1/llvm-project/libcxx-ci/build/check-format/clang-format.patch
error: cannot find executable "clang-format"
🚨 Error: The command exited with status 2

CMake Error at CMakeLists.txt:10 (project):
  No CMAKE_C_COMPILER could be found.

[azat]

Bump CI

[azat]

Any clue on how to fix CI? Thanks in advance!

[Mordante]

In D146294#4262768, @azat wrote:

Any clue on how to fix CI? Thanks in advance!

Can you try to rebase your changes to the HEAD of main, I'm not sure whether that fixes it, but looking at the messages it might.

[azat]

Rebase on top of upstream/main

[azat]

Can you try to rebase your changes to the HEAD of main, I'm not sure whether that fixes it, but looking at the messages it might.

@Mordante yep, seems that this fixed the issue, however the CI seems to stuck for some builds, any clue?

[Mordante]

In D146294#4273210, @azat wrote:

Can you try to rebase your changes to the HEAD of main, I'm not sure whether that fixes it, but looking at the messages it might.

@Mordante yep, seems that this fixed the issue, however the CI seems to stuck for some builds, any clue?

It seems the AIX builders went offline. Can you rebase your patch to trigger a new build. (Note that the sanitizer build failures are not due to your patch.)

[azat]

Rebase on top of upstream/main (to submit one more CI build)

[azat]

It seems the AIX builders went offline. Can you rebase your patch to trigger a new build. (Note that the sanitizer build failures are not due to your patch.)

@Mordante ok, done, thanks!

BTW how does the patch itself looks to you?

[azat]

@Mordante kind ping, can you please take a look at the patch itself (or maybe you can suggest someone who can review the patch)?

About CI- it still fails, though it does not looks related

• premerge checks

lxml.etree.XMLSyntaxError: internal error: Huge input lookup, line 161637, column 9
🚨 Error: The command exited with status 1

[azat]

Friendly ping, can someone review this?

[Mordante]

In D146294#4324132, @azat wrote:

Friendly ping, can someone review this?

Sorry I missed the updates to this patch.

I'm happy with the code, I'm only a bit concerned with the test.

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Why is this needed? Can't you just compare that an int is 32-bit?

I ideally I would rather make the test unsupported on platform that are not used. Otherwise this test gives a false sense of doing something.

Maybe we should add a feature in libcxx/utils/libcxx/test/features.py Something like can-test-gcount Then we only enable it on 64-bit platforms where int is a 32-bit value. WDYT?

24

Why this loop? Wouldn't it be easier to:

• create a string with INT_MAX - 1 elements
• write the string and validate (INT_MAX - 1)
• write 1 character and validate (INT_MAX)
• write 1 character and validate (INT_MAX + 1)

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Yeah, I did not like it either, but that was simpler.

I don't think that introducing special flag only for one test will be a good choice however.
What do you think about something like this instead:

// UNSUPPORTED: target={{.*}}32{{.*}}

But this will not work for some arches (not sure that they are added to CI), i.e. x86.

So instead I could add a separate parameter libcxx/utils/libcxx/test/params.py something like size_t_bytes, although I'm not sure is there an easy way to extract this in python, maybe you have an idea?

[Mordante]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

The tests are also used outside libc++ so even when we don't test x86, other might test that platform.

I don't mind adding a feature for a few test test. We already have precedence for them. For example
1 user host-has-gdb-with-python
2 users for glibc-old-ru_RU-decimal-point
2 users for win32-broken-printf-g-precision

I'm not well enough versed in Python to know whether that's even possible. Even if it is I'm not sure whether Python uses the same widths as C++. I can imagine Python always using 64-bit integrals.

I'm not against your suggestion as long as we can do it reliable.

[azat]

Update the test (add new parameters and simplify the logic)

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

I've found more or less simple, added two new parameters - libcxx-32-bits and libcxx-64-bits, and used UNSUPPORTED: libcxx-32-bits in the test, and llvm-lit successfully skips it if libcxx-32-bits is set. What do you think about this?

24

Nice catch, I've simplified it

[azat]

Fix default value for sizeof_void_p parameter

[azat]

Great, tests have been passed!

[azat]

And seems that parameter works correctly:

I've checked [MinGW (DLL, i686)](https://buildkite.com/organizations/llvm-project/pipelines/libcxx-ci/builds/23603/jobs/0187fa1b-cd4a-4096-a69f-c2e7832e2e04/raw_log):

********************
Unsupported Tests (313):
...
  llvm-libc++-mingw.cfg.in :: std/input.output/string.streams/stringstream.members/gcount.pass.cpp

And [C++20 libcxx/utils/ci/run-buildbot generic-cxx20](https://buildkite.com/organizations/llvm-project/pipelines/libcxx-ci/builds/23603/jobs/0187fa35-d3c2-4b35-95ff-cf5de6ac1cde/raw_log)

... g.in :: std/input.output/string.streams/stringstream.members/gcount.pass.cpp
 43% [=========================----------------------------------(B](B ETA: 00:06:02

[azat]

@Mordante can you take a look one more time? Thanks in advance.

[ldionne]

This seems like a missed case in https://github.com/llvm/llvm-project/commit/d90758e2efff62e5207215d1a20672229ed26f2b, so the fix LGTM but I also have comments about the test.

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Sorry for not chiming in earlier, but IMO there's a lot of added complexity by trying to introduce a new lit feature here. For example, we should not have to change cmake-bridge.cfg.in when adding most new tests.

Is there a reason why we don't simply mark the test as UNSUPPORTED on the targets where it fails? There should be a limited number of those.

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

The reason is simply: I'm not sure that I don't know all 32 bit targets that you have in CI

If someone can give me them, or a link where I can get them, I will replace this new feature with just a list instead.
I thought about using *32* but AFAIR it does not cover all the targets.

[ldionne]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Try removing UNSUPPORTED: libcxx-32-bits, then re-upload the patch and watch the CI failures. Then you can add targets to the failure list per the CI failures.

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Also, I don't know that is the typical use case of the llvm/clang developer, but I guess this feature

20

But AFAIR not all tests will be run for this patch, more various configurations will be run for the main branch? (I remember this from some previous patch, for which CI did not shows the problem, but them CI from main did show)

[ldionne]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

All the configurations we support should be tested in the pre-commit CI. If this is checked in with our CI green and someone complains about another bot somewhere being broken, that's another discussion we need to have with them, but it shouldn't impact your ability to land this. At least that's the way we try to have things, otherwise it's really impossible to know when it's safe to land something.

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Ok, will replace this new parameter with simply UNSUPPORTED

All the configurations we support should be tested in the pre-commit CI. If this is checked in with our CI green and someone complains about another bot somewhere being broken, that's another discussion we need to have with them, but it shouldn't impact your ability to land this. At least that's the way we try to have things, otherwise it's really impossible to know when it's safe to land something.

Here is one example - https://reviews.llvm.org/D133841#3789855

[azat]

Remove additional parameter for test, and simply add all 32 bit arches that CI has as UNSUPPORTED

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Is there a reason why we don't simply mark the test as UNSUPPORTED on the targets where it fails? There should be a limited number of those.

Ok, done (got all failures from one of the previous CI attempts - https://reviews.llvm.org/harbormaster/unit/220047/)

[ldionne]

This LGTM once the CI is green! The CI run is https://buildkite.com/llvm-project/libcxx-ci/builds/23765.

[ldionne]

Thanks for fixing this BTW.

[azat]

Fix UNSUPPORTED usage (missing target=)

[azat]

There was a bug a test, it had been fixed, and now CI is green.

[Mordante]

For now I mark this as request changes since I would like to discuss the approach taken for disabling the tests.

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Sorry for not chiming in earlier, but IMO there's a lot of added complexity by trying to introduce a new lit feature here. For example, we should not have to change cmake-bridge.cfg.in when adding most new tests.

Is there a reason why we don't simply mark the test as UNSUPPORTED on the targets where it fails? There should be a limited number of those.

@ldionne what's wrong with adding a lit feature for 32-bit? I agree it should no be part of cmake-bridge.cfg.in. A simple compile test with a static_asssert would work. As pointed out we have precedence to add features used in a few tests.

[ldionne]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

what's wrong with adding a lit feature for 32-bit?

Nothing, actually. It's just that my preference would be to use the target triple since we have that mechanism. We used to have a 32 bit Lit configuration (and an associated CI bot), and it was removed in favour of testing specific target triples. The precedent I was going for here was to do roughly what we did in libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp, except using Lit features instead of #ifdef.

As pointed out we have precedence to add features used in a few tests.

That's true, however those are often things that we literally can't test without running some significant runtime code (e.g. win32-broken-printf-g-precision).

TLDR I think a feature would be fine too and it's OK to add new Lit features (it's meant for that!), but in this case it would be used in a single test and using the target triple does as good a job, so that was the motivation behind my comment.

[Mordante]

I discussed this with @ldionne and he posted a summary. So let's keep the patch as is.

[azat]

@ldionne can you land this revision for me please? (I don't have write access)

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

All the configurations we support should be tested in the pre-commit CI

@ldionne apparently it is not true, I've just received an email that the test fails on llvm-clang-win-x-armv7l, I can submit one more UNSUPPORTED: *armv7l* to fix this, or a more generic fix.

[Mordante]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

All the configurations we support should be tested in the pre-commit CI

@ldionne apparently it is not true, I've just received an email that the test fails on llvm-clang-win-x-armv7l, I can submit one more UNSUPPORTED: *armv7l* to fix this, or a more generic fix.

I think explicitly excluding this bot would work fine. Are there other bots that fail?

I can land a patch for you, no need to make a review.

[azat]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

I think explicitly excluding this bot would work fine. Are there other bots that fail?

I haven't received anymore so far.

I can land a patch for you, no need to make a review.

https://reviews.llvm.org/D150886

[ldionne]

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

20

Yeah, I agree with @Mordante.

FWIW I'm trying to track down who owns this bot because we should have an equivalent in the pre-commit CI. I don't want to be rude to them, but formally we would consider their configuration as unsupported right now. Let's fix this to unbreak their CI but let's also figure out how we can get their tests in pre-commit.

[vvereschaka]

Hi @azat , @ldionne ,

the llvm-clang-win-x-armv7l builder runs the libc++ tests on the NV Jetson tk1 ARM board. This board has a limited memory: KiB Mem : 1788136 total, 90716 free, 148584 used, 1548836 buff/cache
In case MAX_INT is >2G for that target we should get an out-of-memory exception there or something similar.
I'll start looking, but most likely a reason of the test failures is a limited memory on the boards.

[azat]

In D146294#4354112, @vvereschaka wrote:

Hi @azat , @ldionne ,

the llvm-clang-win-x-armv7l builder runs the libc++ tests on the NV Jetson tk1 ARM board. This board has a limited memory: KiB Mem : 1788136 total, 90716 free, 148584 used, 1548836 buff/cache
In case MAX_INT is >2G for that target we should get an out-of-memory exception there or something similar.
I'll start looking, but most likely a reason of the test failures is a limited memory on the boards.

Hi @vvereschaka,

Thanks for talking a look!
As far as I can see armv7l is 32-bit, then the test will fail anyway, since it tellp returns signed int.

[mgorny]

This test fails on 32-bit x86 too. It's really not acceptable to add a test case that's broken on all 32-bit targets, mark it unsupported on a choice of arbitrary four and have it fail everywhere else. Please fix it to be excluded correctly on all 32-bit arches, or at least a reasonable subset of them.

[Mordante]

In D146294#4358801, @mgorny wrote:

This test fails on 32-bit x86 too. It's really not acceptable to add a test case that's broken on all 32-bit targets, mark it unsupported on a choice of arbitrary four and have it fail everywhere else. Please fix it to be excluded correctly on all 32-bit arches, or at least a reasonable subset of them.

Thanks for the feedback! @ldionne I really feel we should implement my original suggestion where we test whether the platform under test is a 32-bit platform and disable the test based on that result. Based on the feedback, we have more 32-bit users than we test in our CI, using a generic 32-bit test would fix this for all these platforms. WDYT?

[azat]

In D146294#4358805, @Mordante wrote:

@ldionne I really feel we should implement my original suggestion where we test whether the platform under test is a 32-bit platform and disable the test based on that result

FWIW here is my initial patch for this - https://github.com/azat-archive/llvm-project/commit/56ab528e03fc9cba25d4f9944f38f159ec162f88

[Mordante]

In D146294#4358807, @azat wrote:

In D146294#4358805, @Mordante wrote:

@ldionne I really feel we should implement my original suggestion where we test whether the platform under test is a 32-bit platform and disable the test based on that result

FWIW here is my initial patch for this - https://github.com/azat-archive/llvm-project/commit/56ab528e03fc9cba25d4f9944f38f159ec162f88

I talked with @ldionne about making changes to the bridge before (unrelated to this patch) and he convinced me that's not a great idea. There are good reasons to have as little in bridge as possible. However I think we can fix this by changing libcxx/utils/libcxx/test/features.py.

[vvereschaka]

@azat would you fix the test also for armv7-unknown-linux-gnueabihf target?

[azat]

In D146294#4358844, @vvereschaka wrote:

@azat would you fix the test also for armv7-unknown-linux-gnueabihf target?

@vvereschaka it had been fixed already, in https://reviews.llvm.org/D150886

[vvereschaka]

it was fixed for armv7l, but these changes do not work for armv7-* targets.

[mgorny]

Other options could be:

• using regexps to grab wider range of targets (e.g. i.86-.*, arm.*, powerpc-.* and so on),
• doing a if (sizeof(void*) < 8) return 0 or alike in the test.

Note that I haven't tested either.

[azat]

In D146294#4358869, @vvereschaka wrote:

it was fixed for armv7l, but these changes do not work for armv7-* targets.

Oh, I see, anyway, it seems that there are more targets on which the test could fail, let's wait for a more general solution here.

[azat]

In D146294#4358993, @mgorny wrote:

Other options could be:

• using regexps to grab wider range of targets (e.g. i.86-.*, arm.*, powerpc-.* and so on),

It doesn't sounds generic to me.

• doing a if (sizeof(void*) < 8) return 0 or alike in the test.

I had sizeof before, but there was concerns that it is unclear when the test runs and when it is not.

[azat]

Ok, @ldionne @Mordante, what do you think about https://reviews.llvm.org/D151046 ?

libcxx/include/sstream

basic_stringbuf<_CharT, _Traits, _Allocator>::str(const string_type& __s)
     {
         __hm_ = const_cast<char_type*>(__str_.data()) + __str_.size();
         this->setg(const_cast<char_type*>(__str_.data()),
                    const_cast<char_type*>(__str_.data()),
                    __hm_);
     }
     if (__mode_ & ios_base::out)
     {
         typename string_type::size_type __sz = __str_.size();
         __hm_ = const_cast<char_type*>(__str_.data()) + __sz;
         __str_.resize(__str_.capacity());
         this->setp(const_cast<char_type*>(__str_.data()),
                    const_cast<char_type*>(__str_.data()) + __str_.size());
         if (__mode_ & (ios_base::app | ios_base::ate))
         {
-            while (__sz > INT_MAX)
-            {
-                this->pbump(INT_MAX);
-                __sz -= INT_MAX;
-            }
-            if (__sz > 0)
-                this->pbump(__sz);
+            this->__pbump(__sz);
         }
     }
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 typename basic_stringbuf<_CharT, _Traits, _Allocator>::int_type
 basic_stringbuf<_CharT, _Traits, _Allocator>::underflow()
 {
if (__noff != 0)
         if ((__wch & ios_base::out) && this->pptr() == nullptr)
             return pos_type(-1);
     }
     if (__wch & ios_base::in)
         this->setg(this->eback(), this->eback() + __noff, __hm_);
     if (__wch & ios_base::out)
     {
         this->setp(this->pbase(), this->epptr());
-        this->pbump(__noff);
+        this->__pbump(__noff);
     }
     return pos_type(__noff);
 }
 
 // Class template basic_istringstream [istringstream]
 
 template <class _CharT, class _Traits, class _Allocator>
 class _LIBCPP_TEMPLATE_VIS basic_istringstream

libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <sstream>
+
+// Test that tellp() does not breaks the stringstream after INT_MAX, due to use
+// of pbump() that accept int.
+
+#include <string>
+#include <sstream>
+#include <cassert>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+  std::stringstream ss;
+  std::string payload(1 << 20, 'A');
+
+  for (size_t i = 0; i < (2ULL << 30) - payload.size(); i += payload.size()) {
+    assert(ss.tellp() != -1);
+    ss.write(payload.data(), payload.size());
+  }
+
+  assert(ss.tellp() != -1);
+  ss.write(payload.data(), payload.size());
+
+  assert(ss.tellp() != -1);
+  ss.write(payload.data(), payload.size());
+
+  return 0;
+}