CastExpr conversion function decl is sometimes corrupt

[kimgr]

This is a problem that surfaced in IWYU recently.

I'm pretty sure it's difficult to provoke it in the compiler proper, since CastExpr::getConversionFunction is only called in a single place, for diagnostics. Tools built on Clang tooling and the recursive AST visitor can fall victim to it, however.

I have only been able to reproduce it on plain source code (not preprocessed), and only in conjunction with the fmt library (https://github.com/fmtlib/fmt). Attached is a minimal RAV tool and instructions to reproduce.

What makes me think this is somewhat critical is that CastExpr::getConversionFunction returns an AccessSpecDecl in this scenario, which breaks all sorts of AST and LLVM RTTI invariants. I suspect we're looking at garbage data for some reason. I have tried building ubsan+asan-instrumented LLVM/Clang, but without being able to track anything down.

Repro project: ravrepro.tar.gz.

Thankful for any ideas, let me know if I can add more information.

References:

• IWYU tracking bug: Bug: parsing /usr/lib/llvm-12/include/llvm/Support/Casting.h fails? include-what-you-use/include-what-you-use#951
• Potentially same/similar issue: AddressSanitizer use-after-poison error when optimisation is disabled #44317

[davrec72]

I believe the issue is with skipImplicitTemporary, called within CastExpr::getConversionFunction. The problematic SubExpr ends up as a ConstantExpr wrapping a CXXConstructExpr. ConstantExprs are implicit nodes added to help with constant evaluation, and so should be skipped here. I think adding the following at https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1910 should fix:

    // Skip ConstantExprs; they're implicit.
    if (auto *Constant = dyn_cast<ConstantExpr>(E))
      E = Constant->getSubExpr();

[kimgr]

@drec357 Thank you!

I can confirm that inlining those two functions into

static NamedDecl *
my_get_conversion_function(CastExpr *expr) {
  // Skip implicit temporary stuffsies
  const Expr *SubExpr = nullptr;
  for (const CastExpr *E = expr; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) {
    // Skip through reference binding to temporary.
    if (auto *Materialize = dyn_cast<MaterializeTemporaryExpr>(E->getSubExpr()))
      SubExpr = Materialize->getSubExpr();
    else if (auto *Binder = dyn_cast<CXXBindTemporaryExpr>(E->getSubExpr()))
      SubExpr = Binder->getSubExpr();
    else if (auto *Constant = dyn_cast<ConstantExpr>(E->getSubExpr()))
      SubExpr = Constant->getSubExpr();
    else
      SubExpr = E->getSubExpr();

    if (E->getCastKind() == CK_ConstructorConversion)
      return cast<CXXConstructExpr>(SubExpr)->getConstructor();

    if (E->getCastKind() == CK_UserDefinedConversion) {
      if (auto *MCE = dyn_cast<CXXMemberCallExpr>(SubExpr))
        return MCE->getMethodDecl();
    }
  }
  return nullptr;
}

makes both c++17 and c++20 behave as expected.

I'm trying to really understand why we get a zero declkind, though...? Is it because without assertions, the LLVM RTTI just blindly reinterprets the ConstantExpr as a CXXConstructExpr?

[davrec72]

On Jan 6, 2022, at 4:30 PM, Kim Gräsman ***@***.***> wrote: @drec357 <https://github.com/drec357> Thank you! I can confirm that inlining those two functions into static NamedDecl * my_get_conversion_function(CastExpr *expr) { // Skip implicit temporary stuffsies const Expr *SubExpr = nullptr; for (const CastExpr *E = expr; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) { // Skip through reference binding to temporary. if (auto *Materialize = dyn_cast<MaterializeTemporaryExpr>(E->getSubExpr())) SubExpr = Materialize->getSubExpr(); else if (auto *Binder = dyn_cast<CXXBindTemporaryExpr>(E->getSubExpr())) SubExpr = Binder->getSubExpr(); else if (auto *Constant = dyn_cast<ConstantExpr>(E->getSubExpr())) SubExpr = Constant->getSubExpr(); else SubExpr = E->getSubExpr(); if (E->getCastKind() == CK_ConstructorConversion) return cast<CXXConstructExpr>(SubExpr)->getConstructor(); if (E->getCastKind() == CK_UserDefinedConversion) { if (auto *MCE = dyn_cast<CXXMemberCallExpr>(SubExpr)) return MCE->getMethodDecl(); } } return nullptr; } makes both c++17 and c++20 behave as expected. I'm trying to really understand why we get a zero declkind, though...? Is it because without assertions, the LLVM RTTI just blindly reinterprets the ConstantExpr as a CXXConstructExpr?

I believe without assertions the `cast<CXXConstructExpr>(SubExpr)` will always succeed and the program will access bad data until it goes off the rails. Incorrect DeclKinds, StmtClasses etc. are usually where this happens I suppose, since the chances random data matches the kind expected in a certain context are slim.

…

— Reply to this email directly, view it on GitHub <#53044 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB56JNUWNLMNQT7CBPK5M4TUUYCW5ANCNFSM5LMW4E2A>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[kimgr]

Right, makes sense. I was a bit surprised it got that far, but getConstructor really just returns a pointer, so with a little luck that u64 represents an address inside the ASTContext arena.

With that additional context, I've been able to reduce the problematic input to:

// nofmt.cc
struct X {
  consteval X(const char*) {
  }
};

void t() {
  X x = "foobar";
}

This can be provoked using

$ clang-14 -fsyntax-only -Xclang -ast-dump=json -std=c++20 nofmt.cc | grep -A10 "conversionFunc"
                      "conversionFunc": {
                        "id": "0x205e298",
                        "kind": "AccessSpecDecl"
                      },
                      "inner": [
                        {
                          "id": "0x205e2c8",
                          "kind": "ConstantExpr",
                          "range": {
                            "begin": {
                              "offset": 67,

The JSON AST dumper also calls getConversionFunction, and the smoking gun is the "kind": "AccessSpecDecl" line.

I'm not quite sure how to turn that into a lit test.

[kimgr]

I suppose a case could be made for this to be a RecursiveASTVisitor unittest, too.

[kimgr]

I have just uploaded a patch for this at https://reviews.llvm.org/D117391 (which depends on a clang-reformat in https://reviews.llvm.org/D117390). @drec357 I'll add you as reviewer, I hope you don't mind.

[kimgr]

@AaronBallman Thanks for helping land this! I'd like to reintegrate and double-check this with include-what-you-use built against llvm's mainline before closing. I'll report back here as soon as I've confirmed.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[kimgr]

Issue is indeed fixed as of 403d7d8. Is there a procedure for closing, or should I just hit the button?

[AaronBallman]

The procedure is to link to the git hash for the commit with the fix and close. I'll let you do the honors for this one given it was your fix (thanks again!). :-)

[kimgr]

Thanks, closing!