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);Details
Details
- Reviewers
eugenis srhines ldionne erik.pilkington - Group Reviewers
Restricted Project - Commits
- rGa7d0b7a786c7: ld128 demangle: allow space for 'L' suffix.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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. | |
Comment Actions
only if you run with hwasan.
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.) | |
Comment Actions
Do the current tests fail under hwasan? If not, please add a test case that fails under hwasan, it's better than nothing.
Comment Actions
@enh: Do you have commit rights? If not, Louis or I would be happy to commit this on your behalf.
Comment Actions
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.
Comment Actions
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.)
Comment Actions
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.
Comment Actions
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.
Comment Actions
(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 :-( )
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.
Comment Actions
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.
Comment Actions
@enh This is causing a test failure on the windows/aarch64 cross compile bot, please can you take a look:
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/2267
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/2267/steps/test-libc%2B%2Babi/logs/FAIL%3A%20libc%2B%2Babi%3A%3A%20test_demangle.pass.cpp
Comment Actions
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;
}Comment Actions
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)
Comment Actions
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.
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.