This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by azat on Mar 17 2023, 6:18 AM.

Details

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.

Diff Detail

Event Timeline

azat created this revision.Mar 17 2023, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 6:18 AM
azat requested review of this revision.Mar 17 2023, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 6:18 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
azat updated this revision to Diff 506061.Mar 17 2023, 6:19 AM

Update commit message

azat added inline comments.Mar 17 2023, 6:20 AM
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 updated this revision to Diff 506147.Mar 17 2023, 11:06 AM

Fix test for 32-bit (check SIZE_WIDTH)

azat added a subscriber: ldionne.EditedMar 20 2023, 8:11 AM

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

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.

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 updated this revision to Diff 506602.Mar 20 2023, 8:25 AM

Fix review notes in the test

azat marked 2 inline comments as done.Mar 20 2023, 8:26 AM
azat added inline comments.
libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp
24

Sadly but it will allocate 2GiB of RAM

azat marked an inline comment as done.Mar 20 2023, 8:27 AM
azat marked an inline comment as done.
azat added a comment.Mar 26 2023, 11:36 PM

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

azat removed a reviewer: Restricted Project.Apr 3 2023, 3:54 AM

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

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 3 2023, 3:54 AM

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 added a comment.Apr 9 2023, 8:16 AM

@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 added inline comments.Apr 9 2023, 8:41 AM
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 updated this revision to Diff 512016.Apr 9 2023, 8:51 AM

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

azat marked an inline comment as done.Apr 9 2023, 8:52 AM
azat added inline comments.
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 added a comment.Apr 10 2023, 12:33 AM

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 updated this revision to Diff 512920.Apr 12 2023, 11:41 AM

Bump CI

azat added a comment.Apr 12 2023, 1:09 PM

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

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 updated this revision to Diff 513939.Apr 15 2023, 2:59 PM

Rebase on top of upstream/main

azat added a comment.Apr 17 2023, 1:46 AM

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?

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 updated this revision to Diff 514265.Apr 17 2023, 9:05 AM

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

azat added a comment.Apr 17 2023, 9:09 AM

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 added a comment.Apr 25 2023, 3:49 AM

@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

lxml.etree.XMLSyntaxError: internal error: Huge input lookup, line 161637, column 9
🚨 Error: The command exited with status 1
azat added a comment.May 6 2023, 4:41 AM

Friendly ping, can someone review this?

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 added inline comments.May 7 2023, 11:10 AM
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 added inline comments.May 7 2023, 11:37 AM
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 updated this revision to Diff 520206.May 7 2023, 12:19 PM

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

azat marked 3 inline comments as done.May 7 2023, 12:22 PM
azat added inline comments.
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 marked 2 inline comments as done.May 7 2023, 12:29 PM
azat updated this revision to Diff 520263.May 7 2023, 11:42 PM

Fix default value for sizeof_void_p parameter

azat added a comment.May 8 2023, 2:17 AM

Great, tests have been passed!

azat added a comment.EditedMay 8 2023, 2:21 AM

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 added a comment.May 10 2023, 8:48 AM

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

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 added inline comments.May 10 2023, 10:03 AM
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 added inline comments.May 10 2023, 10:06 AM
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 added inline comments.May 10 2023, 10:10 AM
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 added inline comments.May 10 2023, 10:30 AM
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 added inline comments.May 10 2023, 10:44 AM
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 updated this revision to Diff 521038.May 10 2023, 10:51 AM

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

azat marked 3 inline comments as done.May 10 2023, 10:53 AM
azat added inline comments.
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 accepted this revision.May 10 2023, 12:44 PM

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

This revision is now accepted and ready to land.May 10 2023, 12:44 PM

Thanks for fixing this BTW.

azat updated this revision to Diff 521252.May 11 2023, 4:17 AM

Fix UNSUPPORTED usage (missing target=)

azat added a comment.May 11 2023, 1:24 PM

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

Mordante requested changes to this revision.May 13 2023, 3:17 AM

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.

This revision now requires changes to proceed.May 13 2023, 3:17 AM
ldionne added inline comments.May 16 2023, 8:54 AM
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 accepted this revision.May 16 2023, 11:25 AM

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

This revision is now accepted and ready to land.May 16 2023, 11:25 AM
azat added a comment.May 17 2023, 2:10 AM

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

This revision was automatically updated to reflect the committed changes.
azat added inline comments.May 17 2023, 12:45 PM
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 added inline comments.May 18 2023, 9:10 AM
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 marked an inline comment as done.May 18 2023, 10:37 AM
azat added inline comments.
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 added inline comments.May 18 2023, 10:38 AM
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.

azat marked an inline comment as done.May 18 2023, 10:38 AM
azat marked an inline comment as done.

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 added a comment.May 18 2023, 12:55 PM

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 added a subscriber: mgorny.May 20 2023, 1:08 PM

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.

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 added a comment.May 20 2023, 1:30 PM

@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

@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.

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

azat added a comment.May 20 2023, 2:00 PM

@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

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

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 added a comment.May 20 2023, 11:28 PM

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 added a comment.May 20 2023, 11:32 PM

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.