Unsafe buffer usage: calls to [[clang::unsafe_buffer_usage]] constructors are not warned on

[danakj]

[[clang::unsafe_buffer_usage]] should be applied to any function which can cause OOB when given invalid inputs. However if a constructor is marked unsafe, calls to it do not cause a warning to be generated.

This prevents us from requiring annotations on dangerous constructor calls, such as those types like span.

https://godbolt.org/z/debqW3ahx

[danakj]

CC @haoNoQ @ziqingluo-90

[haoNoQ]

Yes looks like an omission, thanks a lot! Looks like we've unnecessarily restricted it to CallExpr so a few other C++ features may cause similar problems. We'll get to it!

[jkorous-apple]

This likely applies to operator() as well.

[jkorous-apple]

We might want to think about supporting the attribute on lambdas and function pointers although that'd be a separate topic entirely.

[danakj]

Another place where constructors appear is in field initializers. if all constructor calls are handled uniformly then the diagnostic shouldn't need to worry about it, but just in case, I thought I'd mention it because field initializers appear very different in the AST.

https://godbolt.org/z/Mz7r3WzqT

struct MySpan {
    [[clang::unsafe_buffer_usage]] explicit constexpr MySpan(int*, size_t) {}
};

struct S {
    S()
      : sp(nullptr, 5) {}  // No warning is generated.

    MySpan sp;
};

int main() {
    S();
}

[danakj]

Is this something that I should look into fixing? I don't want to waste time if someone else is making progress on it.

[ziqingluo-90]

Is this something that I should look into fixing? I don't want to waste time if someone else is making progress on it.

As far as I know, no one is working on fixing this? @haoNoQ @jkorous-apple

[danakj]

I have this working now. Calling operator() is a CallExpr so it already matches, but constructors are CXXConstructExpr which is not a CallExpr subclass, which is why it does not.

The design challenge here is that UnsafeBufferUsageReporter::handleUnsafeOperation() is hard-coded that a CXXConstructExpr should generate the warning for the std::span ctor. Its inputs don't allow it to easy determine that this is a warning on a different ctor, without reproducing all of the matching logic.

So I am going to propose that we split up UnsafeBufferUsageReporter::handleUnsafeOperation() into two:

• The first is largely unchanged, but removes the std::span ctor case. It will handle other CXXConstructExpr in the same way as CallExpr.
• The second would be a new UnsafeBufferUsageReporter::handleUnsafeOperationInContainer() which handles std::span ctor and could handle other future container warnings.

This is mostly straight forward except that then UnsafeBufferUsage.cpp has to decide which method to call. Either it has to dispatch through the Gadget, which would use virtual dispatch to choose the correct one, or it has to query the Gadget to determine if this is a WARNING_GADGET or a WARNING_CONTAINER_GADGET. I've got this working with the latter.

I tried the latter one first, but it's somewhat fragile, as you have to repeat the checks at all call sites. So I am going to try out the virtual dispatch.

[jkorous-apple]

I'd like @haoNoQ to comment on the proposed change.

[danakj]

The virtual one makes it easy to satisfy the TODO to remove getBaseStmt from the FixableGadget interface, which I will do in the same change then, so it seems like the better path forward between those two. I will send the PR shortly

[danakj]

PR: https://github.com/llvm/llvm-project/pull/91777

[danakj]

Constructor initializers will need a bit of a different approach as they are not Stmts. All of the matcher() machinery right now assumes everything is Stmt. I will look into this as another follow-up PR.

[danakj]

PR https://github.com/llvm/llvm-project/pull/91991 is on top of (includes the commits of) https://github.com/llvm/llvm-project/pull/91777 but has additional work to handle field and constructor initializers.

[danakj]

The PR has been sitting for a few days, who would be a good reviewer for it?

[danakj]

The PR has been sitting for a few days, who would be a good reviewer for it?

Oops, I see a reply is there, my bad!