steakhal added a comment.
Aside from that `Returned pointer value points outside the original object with
size of 10 'int' objects` reads somewhat unnatural I have no objections.
A couple of nits here and there but overall I'm pleased to see more verbose bug
reports.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp:83-84
- // FIXME: It would be nice to eventually make this diagnostic more clear,
- // e.g., by referencing the original declaration or by saying *why* this
- // reference is outside the range.
+ auto ConcreteElementCount = ElementCount.getAs<nonloc::ConcreteInt>();
+ auto ConcreteIdx = Idx.getAs<nonloc::ConcreteInt>();
+
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp:102
// Generate a report for this bug.
- auto report =
- std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
-
+ auto report = std::make_unique<PathSensitiveBugReport>(*BT, SBuf, N);
report->addRange(RetE->getSourceRange());
----------------
================
Comment at: clang/test/Analysis/return-ptr-range.cpp:1
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange
-verify %s
+// RUN1: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange
-verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange
-analyzer-output text -verify=notes %s
----------------
Is `RUN1` intentional? If so, what does it do?
================
Comment at: clang/test/Analysis/return-ptr-range.cpp:2
+// RUN1: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange
-verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange
-analyzer-output text -verify=notes %s
----------------
You can specify multiple match prefixes.
If you were passing `-verify=expected,notes`, you would not have to duplicate
each warning.
================
Comment at: clang/test/Analysis/return-ptr-range.cpp:8-10
+int arr1[10]; // notes-note{{Memory object declared here}}
+int arr2[10]; // notes-note{{Memory object declared here}}
+int arr3[10]; // notes-note{{Memory object declared here}}
----------------
Only a single array should be enough.
You can specify the match count for the diagnostic verifier.
`// notes-note 3 {{Memory object...}}` Will succeed only if exactly 3 times
matched.
================
Comment at: clang/test/Analysis/return-ptr-range.cpp:11
+int arr3[10]; // notes-note{{Memory object declared here}}
int *ptr;
----------------
This is not used in every test case. I would recommend moving this to the
function parameter where it's actually being used.
================
Comment at: clang/test/Analysis/return-ptr-range.cpp:19-20
+ ptr = arr1 + x; // notes-note{{Value assigned to 'ptr'}}
+ if (x != 20) // notes-note{{Assuming 'x' is equal to 20}}
+ // notes-note@-1{{Taking false branch}}
+ return arr1; // no-warning
----------------
This is probably more of a taste.
I would prefer fewer indentations.
The same applies everywhere.
================
Comment at: clang/test/Analysis/return-ptr-range.cpp:23
} while (0);
- return ptr; // expected-warning{{Returned pointer value points outside the
original object (potential buffer overflow)}}
+ return ptr; // expected-warning{{Returned pointer value points outside the
original object with size of 10 'int' objects (potential buffer overflow)
[alpha.security.ReturnPtrRange]}}
+ // notes-warning@-1{{Returned pointer value points outside the
original object with size of 10 'int' objects (potential buffer overflow)
[alpha.security.ReturnPtrRange]}}
----------------
I think it's fine to leave the common suffix `(potential ...` out of the
matched string. They give only a little value.
The same applies everywhere, except keeping a single one maybe.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107051/new/
https://reviews.llvm.org/D107051
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
