Specific structure name can cause shadowing of globally defined lambda, causing "struct::operator()" to be called instead of "lambda::operator()"

[jjkazana]

Defining structure with naming convention used by the compiler for lambda expressions ("$_<num>") causes compiler to use structure's "operator()" instead of the one defined for a global lambda(s).

In the following code:

struct $_0
{
    int c;
    void operator()() const {std::cout << __func__ << " " << c <<" from struct\n";}
};

auto lambda = [](){std::cout << __func__ << " from lambda\n";};

int main()
{
    lambda();
}

Call to "lambda()" invokes "$_0::operator()" instead of the expected lambda:

If the "$_0::operator()" makes use of any memory, then calls to lambdas result in memory access operations. "-fsanitize=address" allows for detection of them: godbolt example

[llvmbot]

@llvm/issue-subscribers-c-1

Author: Jacek Kazana (jjkazana)

Defining structure with naming convention used by the compiler for lambda expressions ("$_\<num\>") causes compiler to use structure's "operator()" instead of the one defined for a global lambda(s).

In the following code:

struct $_0
{
    int c;
    void operator()() const {std::cout << __func__ << " " << c <<" from struct\n";}
};

auto lambda = [](){std::cout << __func__ << " from lambda\n";};

int main()
{
    lambda();
}

Call to "lambda()" invokes "$_0::operator()" instead of the expected lambda:

If the "$_0::operator()" makes use of any memory, then calls to lambdas result in memory access operations. "-fsanitize=address" allows for detection of them: godbolt example

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Jacek Kazana (jjkazana)

Defining structure with naming convention used by the compiler for lambda expressions ("$_\<num\>") causes compiler to use structure's "operator()" instead of the one defined for a global lambda(s).

In the following code:

struct $_0
{
    int c;
    void operator()() const {std::cout << __func__ << " " << c <<" from struct\n";}
};

auto lambda = [](){std::cout << __func__ << " from lambda\n";};

int main()
{
    lambda();
}

Call to "lambda()" invokes "$_0::operator()" instead of the expected lambda:

If the "$_0::operator()" makes use of any memory, then calls to lambdas result in memory access operations. "-fsanitize=address" allows for detection of them: godbolt example

[AaronBallman]

Agreed that this happens, but it sure feels like UB due to using the nonstandard identifier as the name of the structure.

[jjkazana]

TBH I found it by looking for a way to purposefully break or alter expected lambda behaviour.

Main issue I see with it is that you can technically hijack code execution without ever having to call that struct explicitly in a codebase, anywhere. It just have to exists in some header, and only warning you get is one about using a '$' sign in a name.

[erichkeane]

ooof... this is troublesome. Note this is not limited to just operator(), this could be ANYTHING we generate for lambdas that could be highjacked, including default, copy, and move construction, move/copy assignment, the __invoke function, destructor, and function-pointer conversion operators (of which we then have others based on calling convention).

We have basically 2 options here:

1- Break ABI and change the generated name of our lambdas (and anything else we generate with a '$' in the name). We could change to __$_<Num> instead which is a reserved name in the C languages (note the leading double underscore). At that point we should ALSO just hard-error on any attempt to name a struct (or perhaps a variable? Or perhaps ANY identifier?) with a name of that pattern.

2- Names with $ are implementation defined. They are pretty common in generated code though, so changing our behavior on them is probably out of the question. We could just change the 'implementation defined' behavior in this case and just hard-error on any attempt to declare a struct (or variable, or identifer, etc?) with a name that fits the troublesome pattern of $_<NUMBERS>. This is perhaps going to break code for many, as this is perhaps a common pattern. But the suggestion is that $_0 would be ill-formed, and $_0a would be fine (since it isn't just $_<NUMBERS>).

@zygoloid for his opinion. This is a tough one that I find myself saying "I need an adult" on.

[AaronBallman]

Personally, I think we should take the ABI break in this case, but I don't think we need the hard error on attempts to name a struct with that pattern. Once we're using reserved identifiers, the user deciding to play shenanigans with their identifiers gets what they get because they hit UB when using the reserved identifier. I can live with "play silly games, win silly prizes" in that case, but it's less palatable when we're technically using an identifier that is not reserved.

[erichkeane]

Personally, I think we should take the ABI break in this case, but I don't think we need the hard error on attempts to name a struct with that pattern. Once we're using reserved identifiers, the user deciding to play shenanigans with their identifiers gets what they get because they hit UB when using the reserved identifier. I can live with "play silly games, win silly prizes" in that case, but it's less palatable when we're technically using an identifier that is not reserved.

I think the hard error is DEFINITELY needed in either case. The submitter alluded to how this is actually a pretty insidious attack vector (though they have to modify a header file). Using reserved names is historically a fairly innocuous 'UB', this is a pretty horrifying 'UB', so the diagnostic is incredibly necessary.

[shafik]

This feels like it could be abused through third party headers and then makes taking over lightly maintained packages a more attractive target.

I would be okay w/ some sort of hard error.

[hubert-reinterpretcast]

1- Break ABI and change the generated name of our lambdas

I don't think this is a "real" ABI break. The Itanium ABI specifies a different mangling (used by Clang) for closure types that may be defined in more than one translation unit. The closure types that use the mangling affected by the current problem do not cross translation unit boundaries.

Indeed, we don't need a new mangling. We can just use the Itanium ABI mangling consistently.

[erichkeane]

1- Break ABI and change the generated name of our lambdas

I don't think this is a "real" ABI break. The Itanium ABI specifies a different mangling (used by Clang) for closure types that may be defined in more than one translation unit. The closure types that use the mangling affected by the current problem do not cross translation unit boundaries.

Indeed, we don't need a new mangling. We can just use the Itanium ABI mangling consistently.

Ah, interesting. That might actually be a pretty trivial fix to this. it might not actually have this problem at all, at the expense of slightly longer names.