Page MenuHomePhabricator

[PATCH] Libcxxabi Demangler PR32890
AbandonedPublic

Authored by marcel on May 22 2017, 1:02 AM.

Details

Summary

Check whether the string (db.names.back().first) is longer than 7 characters before inserting another string at index 8.

Diff Detail

Event Timeline

marcel created this revision.May 22 2017, 1:02 AM
dexonsmith added a subscriber: dexonsmith.
dexonsmith added inline comments.
src/cxa_demangle.cpp
3078–3080

Should this be >= 7?

marcel added inline comments.May 22 2017, 11:13 PM
src/cxa_demangle.cpp
3078–3080

You are right. Thanks for spotting this. Updating the patch.

marcel updated this revision to Diff 99850.May 22 2017, 11:17 PM

Fixing off-by-one. Passes make check-cxxabi for LLVM in trunk on my machine (x86-64, Ubuntu 16.04).

Could you please add more context lines in any future patches? Makes it easier to review!

I think we fixed the same problem at the same time, I have https://reviews.llvm.org/D33368 that also fixes this! The reason that inserting into db.names.back() sometimes results in a insertion out of bounds is that parse_type() can sometimes append multiple names into db.names (ie for a reference to a pack expansion). This results in db.names.back() no longer being the text 'lambda, which makes inserting at +7 dangerous. I think we should handle this case by appending them one by one into the lambda's parameters, which is what my patch does. Do you agree that this is the right approach here?

src/cxa_demangle.cpp
3080

We shouldn't make progress here if something invalid happened, if the names.back().size() is less than 7 then we should just bail out and return first.

Do you agree that this is the right approach here?

Sure. As long as it fixes PR32890.
You could append my test case as a quick-check.

So, I'll go ahead and abandon this revision?

You could append my test case as a quick-check.

Sure, I added the test case here to my patch and it works.

So, I'll go ahead and abandon this revision?

Guess so, sorry for the confusion here.

kcc added a subscriber: kcc.May 23 2017, 11:35 AM

I don't know this code and can't properly comment, but having a constant like this ("7") sounds wrong.
Why not 6, or 8, or 42?

but having a constant like this ("7") sounds wrong. Why not 6, or 8, or 42?

7 is correct here because we inserting into a specific point into a string that was inserted into db.names just above this (but out of context from the diff). The only problem is that more stuff was inserted in the meantime.
This is horrible, but the demangler is littered with random offsets. My personal favorite is that const, volatile, and restrict aren't represented with an enum, but just with the literals 1, 2, and 4.

marcel abandoned this revision.May 23 2017, 5:34 PM

Closed. Patch https://reviews.llvm.org/D33368 addresses PR32890 more comprehensively.