Page MenuHomePhabricator

ld128 demangle: allow space for 'L' suffix.
Needs ReviewPublic

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

Details

Reviewers
eugenis
srhines
ldionne
erik.pilkington
Group Reviewers
Restricted Project
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
5177

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
5177

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.Thu, Jun 25, 12:53 PM

Add test.

enh added a comment.Thu, Jun 25, 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.Thu, Jun 25, 1:25 PM

LGTM, thanks for figuring this out!

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

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