This is an archive of the discontinued LLVM Phabricator instance.

[-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference
ClosedPublic

Authored by malavikasamak on Feb 2 2023, 11:42 AM.

Details

Summary

This patch introduces PointerDereferenceGadget, a FixableGadget that emits fixits to handle cases where a pointer that is identified as unsafe is dereferenced. The current implementation only handles cases where the strategy is to change the type of the raw pointer to std::span. The fixit for this strategy is to fetch the first element from the corresponding span instance.

For example for the code below, the PointerDereferenceGadget emits a fixit for S3 (S1, S2 are to be handled by other gadgets): 
S1: int *ptr = new int[10];
S2: int val1 = ptr[k];  // Unsafe operation
S3: int val2 =  *ptr;  => Fixit: int val2 = ptr[0];

Diff Detail

Event Timeline

malavikasamak created this revision.Feb 2 2023, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 11:42 AM

Changing the tests to new format

jkorous added inline comments.Feb 6 2023, 12:09 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
430

FWIW other Fixables don't need it to keep result.

Also, I am not even entirely sure how does lifetime of Result passed to the constructor look like - since it isn't a smart pointer I'd be worried about the possibility of getting a dangling reference.

436

Nit: I think we don't want to hardcode tags ("op") but instead we should just add another tag like BaseDeclRefExprTag.

800

Ultimately we need to also handle the case when getEndCharLoc returns nullopt.

ziqingluo-90 added inline comments.Feb 9 2023, 5:14 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
805

Trying to make the same pitch as here https://reviews.llvm.org/D142795 :
why don't we replace *DRE with DRE[0] in one fix-it step? The benefit is that we can minimize the use these error-prone locations made from getLocWithOffset.

Addressing the recent comments.

malavikasamak marked 3 inline comments as done.Feb 10 2023, 11:01 AM
jkorous added inline comments.Feb 13 2023, 10:37 AM
clang/lib/Analysis/UnsafeBufferUsage.cpp
430

Let's initialize Op to nullptr.

791

I don't think we should/need to do this - we already have the DeclRefExpr we need in BaseDeclRefExpr.

809

This unreachable is not in the if block that checks for strategy. This is likely incorrect for DeclRefExpr-s to non-VarDecl-s.

NoQ added inline comments.Feb 16 2023, 6:21 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
458–460

IIRC we wanted to move this method to WarningGadget right? So that we didn't need to implement it here? Or was there something that prevented us from doing that? In the latter case nullptr could be really bad.

792

This is another case where the check should probably be included in the matcher. There's no way we'll fix a non-variable, so it shouldn't be "fixable" (some discussion).

What are the other interesting cases that could be found here? I guess BindingDecl could be interesting. Not sure if lambda captures will show up here.

NoQ added inline comments.Feb 16 2023, 6:23 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
794

Clang-format might complain about this.

malavikasamak marked 5 inline comments as done.
malavikasamak added inline comments.
clang/lib/Analysis/UnsafeBufferUsage.cpp
458–460

Thank you. Fixed!

791

Good catch! Thank you!

809

Fixed!

NoQ added inline comments.Feb 21 2023, 1:48 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
791

Ok now if the Decl is not a VarDecl, the program will crash with one of the unreachables, because S.lookup(nullptr) won't find anything.

If we're sure it's always a VarDecl we should use cast<> (instead of dyn_cast<>) to crash immediately. Otherwise, we probably should check that it's a variable as part of the matcher.

Definitely worth a test.

malavikasamak added inline comments.
clang/lib/Analysis/UnsafeBufferUsage.cpp
791

Hmmm. I thought that this should always yield a VarDecl, but perhaps it's best to keep the check for it. Just in case.

malavikasamak retitled this revision from [-Wunsafe-buffer-usage][WIP] Add Fixable for simple pointer dereference to [-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference.Feb 22 2023, 2:21 PM
NoQ added inline comments.Feb 22 2023, 5:11 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
791

Yeah I think something along the lines of https://godbolt.org would have crashed it. Might be a good idea to add a test.

Another tiny nitpick, there appears to be a policy to favor early returns over nesting, this looks like a prime example of that.

malavikasamak added inline comments.Mar 2 2023, 11:21 AM
clang/lib/Analysis/UnsafeBufferUsage.cpp
805

TODO: I am waiting for us to establish a standard for fixit format before addressing this comment.

NoQ added inline comments.Mar 16 2023, 2:54 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
791

Uh-oh, just noticed that godbolt link is broken. Made a new one: https://godbolt.org/z/1ahTx1E37

malavikasamak marked 2 inline comments as done.

Addressing comment on VarDecl checks in matcher and early returns.

NoQ added a comment.Mar 21 2023, 2:27 PM

The early return appears to be missing in the final patch 🤔

NoQ accepted this revision.Mar 21 2023, 2:33 PM

Oh, wait, nvm, you did the matcher thing, nice! LGTM then!

clang/lib/Analysis/UnsafeBufferUsage.cpp
791

Now that this is a matcher, we no longer need the cast to be "dynamic".

This revision is now accepted and ready to land.Mar 21 2023, 2:33 PM
malavikasamak marked an inline comment as done.
This revision was landed with ongoing or failed builds.Mar 22 2023, 3:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 3:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript