[PATCH] Libcxxabi Demangler PR32890

[marcel]

Summary

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

[dexonsmith]

src/cxa_demangle.cpp

3078–3079

Should this be >= 7?

[marcel]

src/cxa_demangle.cpp

3078–3079

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

[marcel]

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

[erik.pilkington]

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.

[marcel]

In D33393#761679, @erik.pilkington wrote:

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?

[erik.pilkington]

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]

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?

[erik.pilkington]

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]

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

CREDITS.TXT

 E: [e-mail]
 D: Minor patches
 
+N: Marcel Boehme
+E: [e-mail]
+D: Minor patches
+
 N: Logan Chien
 E: [e-mail]
 D: ARM EHABI Unwind & Exception Handling

src/cxa_demangle.cpp

                 const char* t1 = t0 + 1;
                 while (t1 != last && std::isdigit(*t1))
                     ++t1;
-                db.names.back().first.insert(db.names.back().first.begin()+7, t0, t1);
+                if (db.names.back().first.length() > 7)
+                  db.names.back().first.insert(db.names.back().first.begin() + 7, t0, t1);
                 t0 = t1;
             }
             if (t0 == last || *t0 != '_')

test/test_demangle.pass.cpp

     "\x44\x74\x70\x74\x71\x75\x32\x43\x41\x38\x65\x6E\x9B\x72\x4D\xC1\x43\x41\x72\x4D\x6E\x77\x38\x9A\x8E\x44\x6F\x64\x6C\x53\xF9\x5F\x70\x74\x70\x69\x45\x38\xD3\x73\x9E\x2A\x37",
     "\x46\x44\x74\x70\x74\x71\x75\x32\x43\x41\x72\x4D\x6E\x65\x34\x9F\xC1\x43\x41\x72\x4D\x6E\x77\x38\x9A\x8E\x44\x6F\x64\x6C\x53\xF9\x5F\x70\x74\x70\x69\x45\x34\xD3\x73\x9E\x2A\x37\x72\x33\x8E\x3A\x29\x8E\x44\x35",
        "_ZcvCiIJEEDvT__FFFFT_vT_v",
+    "Z1JIJ1_T_EE3o00EUlT_E0",
 };
 
 const unsigned NI = sizeof(invalid_cases) / sizeof(invalid_cases[0]);