Page MenuHomePhabricator

ld128 demangle: allow space for 'L' suffix.
ClosedPublic

Authored by enh on Apr 10 2020, 5:42 PM.

Details

Summary

Caught by HWASAN on arm64 Android (which uses ld128 for long double). This
was running the existing fuzzer.

The specific minimized fuzz input to reproduce this is:

__cxa_demangle("1\006ILeeeEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE", 0, 0, 0);

Diff Detail

Event Timeline

enh created this revision.Apr 10 2020, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2020, 5:42 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Can you add your test to libcxxabi/test/test_demangle.pass.cpp? You can put it in invalid_cases if you expect it to fail to demangle.

libcxxabi/src/demangle/ItaniumDemangle.h
5206–5211

Do you know to get the actual upper bound of this? Is it actually 41? Its only used one place for a stack buffer so it doesn't really matter if its too large. It'd also be nice if we were more defensive in the call to snprintf, we should've been able to handle this more gracefully.

Is it possible to add a test to always trigger this bug?

ldionne requested changes to this revision.May 7 2020, 10:31 AM

Requesting changes so it shows up properly in my queue.

This revision now requires changes to proceed.May 7 2020, 10:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2020, 10:31 AM
enh marked an inline comment as done.Jun 11 2020, 1:25 PM

Is it possible to add a test to always trigger this bug?

only if you run with hwasan.

Can you add your test to libcxxabi/test/test_demangle.pass.cpp? You can put it in invalid_cases if you expect it to fail to demangle.

no, it's a valid demangle (for systems where long double is binary128), and the current code doesn't crash anywhere i know of --- it just overflows the buffer.

i'll send a new patch with 42 (and a comment explaining where that came from) when i've worked out how to get phabricator to do that...

libcxxabi/src/demangle/ItaniumDemangle.h
5206–5211

no, i think it's actually 42.

i think -0x1.ffffffffffffffffffffffffffffp+16383 is the longest string, which is 40, plus 1 for 'L', plus 1 for '\0' == 42.

the 0x1. and p+ are fixed, positives are a character shorter, and the 28 'f's == 28*4 bits == 112 bits, which is the number of bits explicitly stored in an IEEE binary128. (because there's always an explicit sign after p, the -16382 smallest exponent is the same length as the +16383 largest exponent.)

enh updated this revision to Diff 270224.Jun 11 2020, 1:44 PM

Use actual upper bound.

In D77924#2088349, @enh wrote:

Is it possible to add a test to always trigger this bug?

only if you run with hwasan.

Do the current tests fail under hwasan? If not, please add a test case that fails under hwasan, it's better than nothing.

enh updated this revision to Diff 273489.Jun 25 2020, 12:53 PM

Add test.

enh added a comment.Jun 25 2020, 12:55 PM
In D77924#2088349, @enh wrote:

Is it possible to add a test to always trigger this bug?

only if you run with hwasan.

Do the current tests fail under hwasan? If not, please add a test case that fails under hwasan, it's better than nothing.

no, the existing tests all pass. this was found by libFuzzer + hwasan.

test added.

erik.pilkington accepted this revision.Jun 25 2020, 1:25 PM

LGTM, thanks for figuring this out!

enh updated this revision to Diff 273546.Jun 25 2020, 4:23 PM

clang-format the new test (but leave the other 30kloc).

ldionne accepted this revision.Jul 30 2020, 6:36 AM
This revision is now accepted and ready to land.Jul 30 2020, 6:36 AM

@enh: Do you have commit rights? If not, Louis or I would be happy to commit this on your behalf.

It looks like the behavior here is host-dependent (we demangle the target's long double literals based on the host's long double). Is that correct? Should we try to avoid that? It seems like a bad idea to make llvm-cxxfilt behave differently on different targets.

It looks like the behavior here is host-dependent (we demangle the target's long double literals based on the host's long double). Is that correct? Should we try to avoid that? It seems like a bad idea to make llvm-cxxfilt behave differently on different targets.

This code predates my involvement in the demangler, and I agree that its unfortunate to have different output depending on the host, but I think this approach (deferring to the host's printf) is kinda reasonable. My understanding is that floating point literals are encoded using the bytes of their internal representation (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling.literal), and we don't know what the encoding is from the mangled name, all we can know is the builtin C++ type and the size of the encoding in bytes. We could potentially try to use that as a heuristic (e.g. 10 bytes probably means x87), but I'm not sure if that would always give the right answer (?), and it would mean that we'd have to write our own floating-point printers for other representations in libcxxabi, which sounds like a pain. WDYT?

(Regardless though, I think we should land this crash-fix.)

My understanding is that floating point literals are encoded using the bytes of their internal representation (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling.literal), and we don't know what the encoding is from the mangled name, all we can know is the builtin C++ type and the size of the encoding in bytes. We could potentially try to use that as a heuristic (e.g. 10 bytes probably means x87), but I'm not sure if that would always give the right answer (?)

We could probably decode 4, 8, and 16 bytes literals as IEEE floats, and 10 bytes as x86 long double, and be right 99% of the time. The only remotely realistic failure mode is that there's an alternative 16-byte float type on certain PowerPC variants.

Alternatively, we could skip decoding and dump the raw bytes; it's rare for a mangled name to contain a float literal in the first place, so most people probably won't notice.

and it would mean that we'd have to write our own floating-point printers for other representations in libcxxabi, which sounds like a pain. WDYT?

LLVM's APFloat has the necessary code. I guess you can't use that directly from libcxxabi, though?

(Regardless though, I think we should land this crash-fix.)

Yes. And maybe also change the code to use snprintf, so we never end up with a buffer overflow.

Yes. And maybe also change the code to use snprintf, so we never end up with a buffer overflow.

Err, I guess it is using snprintf, just not in a way that actually prevents the buffer overflow.

This revision was landed with ongoing or failed builds.Aug 18 2020, 4:14 PM
This revision was automatically updated to reflect the committed changes.
enh added a comment.Aug 18 2020, 4:27 PM

(sorry, didn't see these comments until after i'd git pushed. i've failed to set up gmail filters so i see the mail i need to see without drowning in thousands of mails i don't need to see :-( )

My understanding is that floating point literals are encoded using the bytes of their internal representation (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling.literal), and we don't know what the encoding is from the mangled name, all we can know is the builtin C++ type and the size of the encoding in bytes. We could potentially try to use that as a heuristic (e.g. 10 bytes probably means x87), but I'm not sure if that would always give the right answer (?)

We could probably decode 4, 8, and 16 bytes literals as IEEE floats, and 10 bytes as x86 long double, and be right 99% of the time. The only remotely realistic failure mode is that there's an alternative 16-byte float type on certain PowerPC variants.

yes, demangling an Android arm64 name on Linux x86 would get you a wrong answer. (though, as the other comment said, at least it won't crash now!)

your suggestion would cover everything i know of, but...

Alternatively, we could skip decoding and dump the raw bytes; it's rare for a mangled name to contain a float literal in the first place, so most people probably won't notice.

and it would mean that we'd have to write our own floating-point printers for other representations in libcxxabi, which sounds like a pain. WDYT?

LLVM's APFloat has the necessary code. I guess you can't use that directly from libcxxabi, though?

...that sounds like a problem. (though since there are two copies of the demangler, at least _one_ of them could switch!)

also --- does the demangler even know the arch it's demangling for? i didn't see that in the bits of the code i'd looked at at least.

(Regardless though, I think we should land this crash-fix.)

Yes. And maybe also change the code to use snprintf, so we never end up with a buffer overflow.

as you saw, it does use snprintf, but the buffer size was too small.

In D77924#2225045, @enh wrote:

also --- does the demangler even know the arch it's demangling for? i didn't see that in the bits of the code i'd looked at at least.

No, not at the moment. It's something we could add if it was useful. (I don't think we'd want to add it just for this.)

(Regardless though, I think we should land this crash-fix.)

Yes. And maybe also change the code to use snprintf, so we never end up with a buffer overflow.

as you saw, it does use snprintf, but the buffer size was too small.

It should be possible to use snprintf in a way that doesn't cause a buffer overflow even if the buffer is too small.

enh added a comment.Aug 20 2020, 9:49 AM

is it just me or is the stdout logging from just before that assertion missing?

t.tmp.exe: C:/buildbot/as-builder-2/clang-win-x-aarch64/llvm-project/libcxxabi/test/test_demangle.pass.cpp:30001: void testFPLiterals(): Assertion `false' failed.

any idea how i can see the actual failure details?

        if (std::find(e_beg, e_end, demang) == e_end)
        {
            std::cout << fpCase->mangled << " -> " << fpCase->expecting[0] << '\
n';
            std::cout << "Got instead: " << demang << '\n';
            assert(false);
            continue;
        }

I ran the test directly on our aarch64 board. The output is the following

ubuntu@jetson8:~$ ./t.tmp.exe
Testing 29715 symbols.
1ILeeeEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE -> <-0x1.cecececececececececececececep+11983
Got instead: <-0x1.cecececececececececececececep+11983L>
t.tmp.exe: C:/buildbot/temp/llvm-project/libcxxabi/test/test_demangle.pass.cpp:30001: void testFPLiterals(): Assertion `false' failed.
Aborted (core dumped)
enh added a comment.Aug 20 2020, 4:02 PM

I ran the test directly on our aarch64 board.

thank you! my aarch64 device is dead right now, and thanks to working from home i can't just kick it :-(

The output is the following

ubuntu@jetson8:~$ ./t.tmp.exe
Testing 29715 symbols.
1ILeeeEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE -> <-0x1.cecececececececececececececep+11983
Got instead: <-0x1.cecececececececececececececep+11983L>
t.tmp.exe: C:/buildbot/temp/llvm-project/libcxxabi/test/test_demangle.pass.cpp:30001: void testFPLiterals(): Assertion `false' failed.
Aborted (core dumped)

ah, crap, yeah, there's a bad copy and paste in the test (missing that trailing 'L').

fixed by https://reviews.llvm.org/D86321.