[SUGGESTION] Can cppfront detect/prevent unnamed RAII objects like `scoped_lock`?

[bluetarpmedia]

Can cppfront prevent bugs like this?

main: () -> int = {
    m: std::mutex = ();
    _ = std::scoped_lock(m);  // Oops
    return 0;
}

The user probably intended to write : _: std::scoped_lock = (m);

But instead that scoped_lock line lowers to static_cast<void>(std::scoped_lock(m)); and the temporary lock object destructs at the end of the statement.

See this example for a demonstration of the bug. Note that there are no Clang warnings about the temporary object destructing.

See also the same code compiled by MSVC. Note that the static_cast<void> suppresses the compiler warning.

Will your feature suggestion eliminate X% of security vulnerabilities of a given kind in current C++ code? CWE-667: Improper Locking can lead to race conditions and there are many examples of exploits listed on that page.

I haven't found any specific CVEs caused by unnamed C++ lock_guard (etc) objects but there are examples of bugs online, e.g. Fix unnamed lock in AESGCMGMAC_KeyFactory.cpp from eProsima Fast DDS.

Will your feature suggestion automate or eliminate X% of current C++ guidance literature? Yes, see Cpp Core Guidelines ES.84: Don’t try to declare a local variable with no name

[JohelEGP]

See the commit message for commit 41e54690f914ece7bc70e2ae695dd183ebf1fd53.

[bluetarpmedia]

See the commit message for commit 41e5469.

Do you mean this part?

• rationale: if that were allowed to keep the returned object alive, that syntax would be dangerously close to '_ = f();' to discard the returned object, which is exactly opposite in a fundamentally important way (lifetime!) and such importantly opposite meanings deserve more than a one-character typo distance
• but this shouldn't be any hardship because cases that use a type are still just as easy, e.g., lock_guard; before this commit _ := lock_guard(mut); worked, and after this commit that's disallowed but it's just as easy to write _: lock_guard = mut;

I agree it's easy to write the correct syntax _: std::scoped_lock = (m); but if a Cpp2 user writes the buggy discard version then not only does cppfront silently succeed, but the resulting C++ code has no warnings emitted from MSVC, Clang, GCC or clang-tidy. (MSVC and clang-tidy both produce warnings if the static_cast<void> is removed.)

[JohelEGP]

Yeah, cppfront can't prevent this.

[hsutter]

Thanks for pointing this out.

If this is a serious pain point, I think it's possible to prevent it. The question is the cost/benefit.

One option would be to limit or even disallow the function-style cast syntax. That syntax works because I never disallowed it, and it's been convenient in some cases. If we decided we wanted to limit or disallow it, we could for example make every free-function call syntax f(/*args*/) lower to something like an inline invoke-style cpp2::call(f /*,args*/), and then constrain that to reject the call if f:

• is a type name, if we want to reject all function-style constructor calls; or
• is a type name that satisfies a "ThisIsAnImportantType" concept. For example, standard mutexes are known to follow the .lock/.unlock/.try_lock shape, so that would be detectable via a constraint.

My bias would be to want to see some concrete data that it's a pain point before complicating the lowering for ordinary function calls. That said, I have to admit we already complicate the lowering of all member function calls, so maybe it's not that big a step... 😉

[bluetarpmedia]

My bias would be to want to see some concrete data that it's a pain point before complicating the lowering for ordinary function calls.

I also prefer an evidence/data-driven approach but I'm unsure how that could be obtained at the moment without wider use of Cpp2. A poll of current contributors right now is probably not a good sample!

If breaking changes are permitted for the foreseeable future then we can always delay a decision on this until there's enough usage and feedback available.

For me, the argument that I keep returning to in favour of fixing this now is that if a Cpp2 user writes this bug, then they won't get any warning or error either from cppfront or more importantly their C++ compiler, and the impact of race condition bugs are usually high. Obviously there are lots of bugs we can write that are not diagnosed at compile time but this one seems like it should be, especially because the equivalent C++ code is warned about (if only by MSVC and clang-tidy).

[Xeverous]

How about adding an attribute, similar to existing (and implicit in Cpp2) [[nodiscard]]? Such attribute could be applied to a type definition to warn about discarded constructions. This could also be applied to gsl::finally.

[SebastianTroy]

It is strange to me that constructors cannot be marked nodiscard, it feels natural, expecially for RAII types, this ought to be in the base language too

On 24 June 2024 09:54:11 Xeverous @.***> wrote:

How about adding an attribute, similar to existing (and implicit in Cpp2) [[nodiscard]]? Such attribute could be applied to a type definition to warn about discarded constructions. This could also be applied to gsl::finally.

— Reply to this email directly, view it on GitHubhttps://github.com/hsutter/cppfront/issues/973#issuecomment-2185962158, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALUZQJRZXHF2QX2S227XATZI7NDBAVCNFSM6AAAAABC66U7FGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBVHE3DEMJVHA. You are receiving this because you are subscribed to this thread.Message ID: @.***>