[libcxxabi][demangler] Fix a crash in the demangler

[erik.pilkington]

Summary

This patch fixes a bug in the demangler where a pack expansion template parameter substitution with more than one element in a lambda's parameter list resulted in either a misdemangle or a crash. I'll commit this change to ItaniumDemangle.cpp as well, as it has the same problem.

rdar://32098667

Thanks for taking a look,
Erik

[compnerd]

src/cxa_demangle.cpp

3036

I'm not sure how k1 can be < than k0. Isn't this effectively always going down the == path? If I'm just mistaken, then I think that this needs a comment.

3042–3051

I think that using a bit of algorithm here might be nice.

std::ostringstream oss(db.names[lambda_pos]);
std::copy_if(db.names[k0], db.names[k1], std::ostream_iterator<std::string>(oss, ",  "),
             [](const std::string &s) -> bool { return !s.empty(); });

[dexonsmith]

src/cxa_demangle.cpp

3042–3051

Introducing a dependency on std::ostream would be awkward. libc++abi.dylib cannot link against libc++.dylib, and it would bloat libc++abi.dylib to use custom traits to pull in a full copy.

[erik.pilkington]

src/cxa_demangle.cpp

3036

I agree that it should be impossible, but the previous version handled this case with the if (db.names.size() < 2) check, when really db.names.size() can only be 1 or greater. I think it makes more sense to be paranoid here, I trust this file about as far as I can throw it (And I can't throw it very far, its not a tangible object).

[compnerd]

src/cxa_demangle.cpp

3036

Hmm, how about being paranoid, and converting that to an assertion and then doing the == check? This code is already incredibly complicated, so adding additional things like this only makes it harder to understand whats going on.

3042–3051

Ugh, thats a good point. Very well. However, there doesnt seem to be any iterator invalidation here, so we can still just iterate over a slice here, which would be nicer.

[erik.pilkington]

In this new patch:

• Add the testcase from https://reviews.llvm.org/D33393
• Add an assert() that k0 <= k1
• Use std::for_each instead of the for loop

Thanks for taking a look,
Erik

[dexonsmith]

src/cxa_demangle.cpp

3036

There's no #include <cassert> in this file. Depending on the standard library you're building against, you made need that here.

[erik.pilkington]

src/cxa_demangle.cpp

3036

Sure, I'll commit this with that change.

[kcc]

oss-fuzz finds the assertion failure in this new code:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=1834

[erik.pilkington]

r303806 removes the assertion (instead just returning first). I though this should never happen, I'm looking into this testcase to see if there is another bug here.
Thanks,
Erik

[kcc]

I also encourage you to run the fuzzer on every change in this code.

[kcc]

Also, are you now maintaining this code?
I am trying to find someone who wants to be CC-ed to other demangler bugs automatically reported by oss-fuzz.

[erik.pilkington]

Also, are you now maintaining this code?
I am trying to find someone who wants to be CC-ed to other demangler bugs automatically reported by oss-fuzz.

I don’t think I’ll accept the title of maintainer, (I only have one commit in this file!) but I have some plans to work on it and would be interested in hearing about any bugs found in the fuzzer.
Can you add [e-mail] to your CC list?

[kcc]

Done (see https://github.com/google/oss-fuzz/blob/master/projects/llvm_libcxxabi/project.yaml)

src/cxa_demangle.cpp

                 db.names.pop_back();
                 return first;
             }
             first = t0 + 1;
           }
             break;
         case 'l':
           {
+            size_t lambda_pos = db.names.size();
             db.names.push_back(typename C::String("'lambda'("));
             const char* t0 = first+2;
             if (first[2] == 'v')
             {
                 db.names.back().first += ')';
                 ++t0;
             }
             else
             {
-                const char* t1 = parse_type(t0, last, db);
-                if (t1 == t0)
-                {
-                    if(!db.names.empty())
-                        db.names.pop_back();
-                    return first;
-                }
-                if (db.names.size() < 2)
-                    return first;
-                auto tmp = db.names.back().move_full();
-                db.names.pop_back();
-                db.names.back().first.append(tmp);
-                t0 = t1;
+                bool is_first_it = true;
                 while (true)
                 {
-                    t1 = parse_type(t0, last, db);
+                    size_t k0 = db.names.size();
+                    const char* t1 = parse_type(t0, last, db);
+                    size_t k1 = db.names.size();
                     if (t1 == t0)
                         break;
-                    if (db.names.size() < 2)
+                    if (k1 <= k0)
                         return first;
-                    tmp = db.names.back().move_full();
-                    db.names.pop_back();
-                    if (!tmp.empty())
-                    {
-                        db.names.back().first.append(", ");
-                        db.names.back().first.append(tmp);
+                    // If the call to parse_type above found a pack expansion
+                    // substitution, then multiple names could have been
+                    // inserted into the name table. Walk through the names,
+                    // appending each onto the lambda's parameter list.
+                    for (size_t k = k0; k < k1; ++k) {
+                        auto tmp = db.names[k].move_full();
+                        if (!tmp.empty())
+                        {
+                            if (!is_first_it)
+                                db.names[lambda_pos].first.append(", ");
+                            is_first_it = false;
+                            db.names[lambda_pos].first.append(tmp);
+                        }
                     }
+                    db.names.erase(db.names.begin() +
+                                       static_cast<long>(lambda_pos) + 1,
+                                   db.names.end());
                     t0 = t1;
                 }
-                if(db.names.empty())
+                if (is_first_it)
+                {
+                    if (!db.names.empty())
+                        db.names.pop_back();
+                    return first;
+                }
+                if (db.names.empty() || db.names.size() - 1 != lambda_pos)
                   return first;
                 db.names.back().first.append(")");
             }
             if (t0 == last || *t0 != 'E')
             {
               if (!db.names.empty())
                 db.names.pop_back();
               return first;

test/test_demangle.pass.cpp

     {"i", "int"},
 
     {"PKFvRiE", "void (*)(int&) const"},
     // FIXME(compnerd) pretty print this as void (*)(unsigned long &) volatile &&
     {"PVFvRmOE", "void (*)(unsigned long&)  volatile&&"},
     {"PFvRmOE", "void (*)(unsigned long&) &&"},
     {"_ZTW1x", "thread-local wrapper routine for x"},
     {"_ZTHN3fooE", "thread-local initialization routine for foo"},
+    {"_Z4algoIJiiiEEvZ1gEUlT_E_", "void algo<int, int, int>(g::'lambda'(int, int, int))"},
 };
 
 const unsigned N = sizeof(cases) / sizeof(cases[0]);
 
 struct FPLiteralCase {
     const char *mangled;
     // There are four possible demanglings of a given float.
     std::string expecting[4];