[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)

Dana Jansens via cfe-commits cfe-commits at lists.llvm.org
Tue May 14 10:12:30 PDT 2024


danakj wrote:

> Hi! Thank you for digging into this! Sorry for the delay.
> 
> > The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching against the std::span(ptr, size) constructor because that is handled by SpanTwoParamConstructorGadget and we never want two gadgets to match the same thing (and this is guarded by asserts).
> 
> Hmm at a glance I'm not sure this should really be illegal. It's a big problem when fixable gadgets overlap, because this means that they'll try to fix the same code in two incompatible ways. I don't immediately see why this would be a problem for warning gadgets that don't try to fix anything. This just means that the code is non-compliant for two different reasons, which is theoretically fine. I also tried to reproduce your assert and I didn't hit it; which one are you running into?

https://github.com/llvm/llvm-project/blob/2ff43ce87e66d9324370e35ea6743ef57400c76e/clang/lib/Analysis/UnsafeBufferUsage.cpp#L1373-L1374

These assert that exactly one gadget matched. I think it's kinda worthwhile keeping the warnings independent too, so I don't see this as a bad thing. If we were to mark the span ctor as `[[clang::unsafe_buffer_usage]]` for instance, it the span-specific warning gives a better output still, and generating two warnings for the same piece of code is noisy.

> > To handle this we allow the gadget to control if the warning is general (it calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it calls handleUnsafeOperationInContainer()).
> 
> Ooo I love this.
> 
> My initial thinking was, just make the base Gadget class "public" (move it into `UnsafeBufferUsage.h` so that the handler could see it) and make the handler switch over the gadget's `getKind()` to produce specialized messages for each gadget. This would help us unscrew the rest of the `Reporter` code so that it also didn't have to guess by statement kind.
> 
> Your design can grow into that too - make a separate handler method for each gadget kind (or group of kinds), and it preserves even more encapsulation. Additionally it throws away `getBaseStmt()` in favor of a more "integrated" solution. I'm a big fan, let's keep this.

:+1: 

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


More information about the cfe-commits mailing list