[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added a comment.

In D137379#4225000 , @manojgupta 
wrote:

> This is firing even in checked length codes, is that expected?

Yes, it is expected.  The unsafe buffer analysis is syntax-based.  The analysis 
warns operations that do not follow the buffer-safe programming model we are 
suggesting.  The programming model prohibits pointer arithmetic.  In the model, 
pointer arithmetic and buffer access can be done using hardened libc++ 
facilities such as `std::span`.

More information about the analysis and the programming model can be found at 
https://discourse.llvm.org/t/rfc-c-buffer-hardening/65734.

To suppress the warning, you can either turn the analysis off using 
`-Wno-unsafe-buffer-usage` or put code in a pair of opt-out pragmas `#pragma 
clang unsafe_buffer_usage begin` & `#pragma clang unsafe_buffer_usage end`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

This is firing even in checked length codes, is that expected?

example:
https://godbolt.org/z/Todje76ao

  std::optional result;
  bool ReadDevice(uint8_t* data, size_t len) {
  if (!result)
return false;
memset(data, 0, len);
if (len > 0) data[0] = (result.value() >> 8) & 0xFF;
if (len > 1) data[1] = result.value() & 0xFF;
return true;
  }

  :7:26: warning: 'data' is an unsafe pointer used for buffer access 
[-Wunsafe-buffer-usage]
  bool ReadDevice(uint8_t* data, size_t len) {
  ~^~~~
  :13:20: note: used in buffer access here
if (len > 1) data[1] = result.value() & 0xFF;
 ^~~~


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6d1d055fad50: [-Wunsafe-buffer-usage] Add warnings for 
unsafe buffer accesses by array… (authored by ziqingluo-90).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -6,3 +6,63 @@
   --p; // expected-warning{{unchecked operation on raw buffer in expression}}
   p--; // expected-warning{{unchecked operation on raw buffer in expression}}
 }
+
+void foo(...);   // let arguments of `foo` to hold testing expressions
+
+void * voidPtrCall(void);
+char * charPtrCall(void);
+
+void testArraySubscripts(int *p, int **pp) {
+  foo(p[0], // expected-warning{{unchecked operation on raw buffer in expression}}
+  pp[0][0], // expected-warning2{{unchecked operation on raw buffer in expression}}
+  0[0[pp]], // expected-warning2{{unchecked operation on raw buffer in expression}}
+  0[pp][0]  // expected-warning2{{unchecked operation on raw buffer in expression}}
+  );
+
+  if (p[3]) {   // expected-warning{{unchecked operation on raw buffer in expression}}
+void * q = p;
+
+foo(((int*)q)[10]); // expected-warning{{unchecked operation on raw buffer in expression}}
+  }
+
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unchecked operation on raw buffer in expression}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unchecked operation on raw buffer in expression}}
+  charPtrCall()[3], // expected-warning{{unchecked operation on raw buffer in expression}}
+  3[charPtrCall()]  // expected-warning{{unchecked operation on raw buffer in expression}}
+  );
+
+  int a[10], b[10][10];
+
+  // not to warn subscripts on arrays
+  foo(a[0], a[1],
+  0[a], 1[a],
+  b[3][4],
+  4[b][3],
+  4[3[b]]);
+}
+
+void testArraySubscriptsWithAuto(int *p, int **pp) {
+  int a[10];
+
+  auto ap1 = a;
+
+  foo(ap1[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+
+  auto ap2 = p;
+
+  foo(ap2[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+
+  auto ap3 = pp;
+
+  foo(pp[0][0]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+
+  auto ap4 = *pp;
+
+  foo(ap4[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+}
+
+void testUnevaluatedContext(int * p) {
+  //TODO: do not warn for unevaluated context
+  foo(sizeof(p[1]), // expected-warning{{unchecked operation on raw buffer in expression}}
+  sizeof(decltype(p[1])));  // expected-warning{{unchecked operation on raw buffer in expression}}
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -129,6 +129,32 @@
 
   const UnaryOperator *getBaseStmt() const override { return Op; }
 };
+
+/// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
+/// it doesn't have any bounds checks for the array.
+class ArraySubscriptGadget : public UnsafeGadget {
+  static constexpr const char *const ArraySubscrTag = "arraySubscr";
+  const ArraySubscriptExpr *ASE;
+
+public:
+  ArraySubscriptGadget(const MatchFinder::MatchResult )
+  : UnsafeGadget(Kind::ArraySubscript),
+ASE(Result.Nodes.getNodeAs(ArraySubscrTag)) {}
+
+  static bool classof(const Gadget *G) {
+return G->getKind() == Kind::ArraySubscript;
+  }
+
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(
+arraySubscriptExpr(hasBase(ignoringParenImpCasts(hasPointerType(
+.bind(ArraySubscrTag));
+  }
+
+  const ArraySubscriptExpr *getBaseStmt() const override { return ASE; }
+};
 } // namespace
 
 // Scan the function and return a list of gadgets found with provided kits.
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
===
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -27,6 +27,7 @@
 
 UNSAFE_GADGET(Increment)
 UNSAFE_GADGET(Decrement)
+UNSAFE_GADGET(ArraySubscript)
 
 #undef SAFE_GADGET
 #undef UNSAFE_GADGET
___
cfe-commits mailing list

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

LGTM! Let's land once I land the previous patches.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 480703.
ziqingluo-90 added a comment.

Addressing all the comments we have so far.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -6,3 +6,63 @@
   --p; // expected-warning{{unchecked operation on raw buffer in expression}}
   p--; // expected-warning{{unchecked operation on raw buffer in expression}}
 }
+
+void foo(...);   // let arguments of `foo` to hold testing expressions
+
+void * voidPtrCall(void);
+char * charPtrCall(void);
+
+void testArraySubscripts(int *p, int **pp) {
+  foo(p[0], // expected-warning{{unchecked operation on raw buffer in expression}}
+  pp[0][0], // expected-warning2{{unchecked operation on raw buffer in expression}}
+  0[0[pp]], // expected-warning2{{unchecked operation on raw buffer in expression}}
+  0[pp][0]  // expected-warning2{{unchecked operation on raw buffer in expression}}
+  );
+
+  if (p[3]) {   // expected-warning{{unchecked operation on raw buffer in expression}}
+void * q = p;
+
+foo(((int*)q)[10]); // expected-warning{{unchecked operation on raw buffer in expression}}
+  }
+
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unchecked operation on raw buffer in expression}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unchecked operation on raw buffer in expression}}
+  charPtrCall()[3], // expected-warning{{unchecked operation on raw buffer in expression}}
+  3[charPtrCall()]  // expected-warning{{unchecked operation on raw buffer in expression}}
+  );
+
+  int a[10], b[10][10];
+
+  // not to warn subscripts on arrays
+  foo(a[0], a[1],
+  0[a], 1[a],
+  b[3][4],
+  4[b][3],
+  4[3[b]]);
+}
+
+void testArraySubscriptsWithAuto(int *p, int **pp) {
+  int a[10];
+
+  auto ap1 = a;
+
+  foo(ap1[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+
+  auto ap2 = p;
+
+  foo(ap2[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+
+  auto ap3 = pp;
+
+  foo(pp[0][0]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+
+  auto ap4 = *pp;
+
+  foo(ap4[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+}
+
+void testUnevaluatedContext(int * p) {
+  //TODO: do not warn for unevaluated context
+  foo(sizeof(p[1]), // expected-warning{{unchecked operation on raw buffer in expression}}
+  sizeof(decltype(p[1])));  // expected-warning{{unchecked operation on raw buffer in expression}}
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -128,6 +128,32 @@
 
   const UnaryOperator *getBaseStmt() const override { return Op; }
 };
+
+/// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
+/// it doesn't have any bounds checks for the array.
+class ArraySubscriptGadget : public UnsafeGadget {
+  static constexpr const char *const ArraySubscrTag = "arraySubscr";
+  const ArraySubscriptExpr *ASE;
+
+public:
+  ArraySubscriptGadget(const MatchFinder::MatchResult )
+  : UnsafeGadget(Kind::ArraySubscript),
+ASE(Result.Nodes.getNodeAs(ArraySubscrTag)) {}
+
+  static bool classof(const Gadget *G) {
+return G->getKind() == Kind::ArraySubscript;
+  }
+
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(
+arraySubscriptExpr(hasBase(ignoringParenImpCasts(hasPointerType(
+.bind(ArraySubscrTag));
+  }
+
+  const ArraySubscriptExpr *getBaseStmt() const override { return ASE; }
+};
 } // namespace
 
 // Scan the function and return a list of gadgets found with provided kits.
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
===
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -27,6 +27,7 @@
 
 UNSAFE_GADGET(Increment)
 UNSAFE_GADGET(Decrement)
+UNSAFE_GADGET(ArraySubscript)
 
 #undef SAFE_GADGET
 #undef UNSAFE_GADGET
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:16
+void testArraySubscripts(int *p, int **pp) {
+  foo(p[0], // expected-warning{{unchecked operation on raw buffer 
in expression}}
+  pp[0][0], // expected-warning2{{unchecked operation on raw 
buffer in expression}}

aaron.ballman wrote:
> One test case I'd like to see is: `sizeof(p[0])` -- should code in an 
> unevaluated context be warned?
I think they should NOT be warned. We haven't addressed the issue of 
unevaluated context in our patches. I'm adding a test for code in unevaluated 
context so that we don't forget about it later.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:43
+}
+
+void testArraySubscriptsWithAuto(int *p, int **pp) {

aaron.ballman wrote:
> Can you also add tests for function declarations like:
> ```
> void foo(int not_really_an_array[10]) { ... }
> 
> template 
> void bar(int (_an_array)[N]) { ... }
> 
> // Using a dependent type but we know it's a pointer.
> template 
> void baz(Ty *ptr) { ... }
> 
> // Using a dependent type where we have no idea if it's a pointer.
> template 
> void quux(Ty ptr) { ... }
> ```
> 
Thanks for suggesting these test cases. They have been added in one of the 
following patches (https://reviews.llvm.org/D138318). That patch improves the 
matchers to recognize these cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:155-176
+class ArraySubscriptGadget : public UnsafeGadget {
+  const ArraySubscriptExpr *ASE;
+
+public:
+  ArraySubscriptGadget(const MatchFinder::MatchResult )
+  : UnsafeGadget(Kind::ArraySubscript),
+ASE(Result.Nodes.getNodeAs("arraySubscr")) {}

Carryover from the nearby patch - let's make a named constant for "arraySubscr"!




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:175
+
+  const Stmt *getBaseStmt() const override { return ASE; }
+};

Carryover from nearby patch - let's use covariant return types!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:168-169
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(

xazax.hun wrote:
> NoQ wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > As per some of the discussions, in the future the compiler might be 
> > > > able to recognize certain safe patterns, e.g., when there is a simple 
> > > > for loop with known bounds, or when both the index and the array size 
> > > > is statically known.
> > > > 
> > > > I think here we need to make a very important design decision: Do we 
> > > > want the gadgets to have the right "safety" category when it is created 
> > > > (e.g., we have to be able to decide if a gadget is safe or not using 
> > > > matchers), or do we want some mechanisms to be able to promote an 
> > > > unsafe gadget to be a safe one? (E.g., do we want to be able to prove 
> > > > some unsafe gadgets safe using dataflow analysis in a later pass?)
> > > (FWIW, this is a great question and I really appreciate you asking it!)
> > My original design implies that safe gadgets are a separate hierarchy, so 
> > there will be a new gadget class for index zero, and this gadget will be 
> > changed to skip index zero. But I don't immediately see why such early 
> > separation is strictly necessary, other than for a bit of extra type safety 
> > (extra virtual methods of the `UnsafeGadget` class don't make sense on safe 
> > gadgets). We *do* have time to make this distinction later, before we get 
> > to emitting warnings.
> > 
> > So maybe eventually we'll end up replacing `isSafe()` with a pure virtual 
> > method and deprecate `SafeGadget` and `UnsafeGadget` base classes, if we 
> > see it cause too much duplication or it turns out that the extra analysis 
> > necessary to establish safety can't be nicely implemented in ASTMatchers. 
> > In this case I'll admit that I over-engineered it a bit.
> I'd strongly prefer `isSafe()` method over inheritance. While adding safe and 
> unsafe gadgets for zero indices is not too bad, if we plan to expand this to 
> more cases, like known array size + compile time constant index, it will get 
> harder and harder to keep all the gadgets in sync. We can end up missing 
> cases or detecting a code snippet both as safe and unsafe. The `isSafe` 
> method would basically get rid of a whole class of problems, where safe and 
> unsafe versions of the gadgets are overlapping. That being said, I am not 
> insisting doing this change in this PR, it is OK doing the change later in a 
> follow-up.
Ok so we've had an interesting conversation about this, it's actually harder 
than that and we'll probably have a much more divergence between unsafe gadgets 
and safe gadgets covering these operations. The problem is that most unsafe 
gadgets don't allow fixing code anyway, you typically need more context. For 
example, `ptr[i]` may represent unsafe code and `ptr[0]` may represent safe 
code, but that doesn't make `ptr[0]` a safe gadget, because it doesn't make it 
//fixable//. In particular, `[0]` crashes when `span` is empty, whereas 
`ptr[0]` doesn't crash when `ptr` points to one-past-end of the array, so 
blinding replacing `ptr[0]` with `span[0]` will break some code.

Of course even simple `ptr`, regardless of context, is always fixable to 
`span.data()`. But that's a low-quality fix that ruins libc++ runtime checks. 
So we'll do our best to avoid such fixes unless the extra context makes them 
inevitable.

So I suspect that we'll allow safe and unsafe gadgets to //partially overlap// 
and exclude unsafe gadgets from fixit machine entirely (i.e. move the 
`getFixits()` method to the `SafeGadget` class).

So, like I said in https://reviews.llvm.org/D137348?id=472990#inline-1345337, 
@malavikasamak is looking into this more deeply.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:168-169
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(

NoQ wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > As per some of the discussions, in the future the compiler might be able 
> > > to recognize certain safe patterns, e.g., when there is a simple for loop 
> > > with known bounds, or when both the index and the array size is 
> > > statically known.
> > > 
> > > I think here we need to make a very important design decision: Do we want 
> > > the gadgets to have the right "safety" category when it is created (e.g., 
> > > we have to be able to decide if a gadget is safe or not using matchers), 
> > > or do we want some mechanisms to be able to promote an unsafe gadget to 
> > > be a safe one? (E.g., do we want to be able to prove some unsafe gadgets 
> > > safe using dataflow analysis in a later pass?)
> > (FWIW, this is a great question and I really appreciate you asking it!)
> My original design implies that safe gadgets are a separate hierarchy, so 
> there will be a new gadget class for index zero, and this gadget will be 
> changed to skip index zero. But I don't immediately see why such early 
> separation is strictly necessary, other than for a bit of extra type safety 
> (extra virtual methods of the `UnsafeGadget` class don't make sense on safe 
> gadgets). We *do* have time to make this distinction later, before we get to 
> emitting warnings.
> 
> So maybe eventually we'll end up replacing `isSafe()` with a pure virtual 
> method and deprecate `SafeGadget` and `UnsafeGadget` base classes, if we see 
> it cause too much duplication or it turns out that the extra analysis 
> necessary to establish safety can't be nicely implemented in ASTMatchers. In 
> this case I'll admit that I over-engineered it a bit.
I'd strongly prefer `isSafe()` method over inheritance. While adding safe and 
unsafe gadgets for zero indices is not too bad, if we plan to expand this to 
more cases, like known array size + compile time constant index, it will get 
harder and harder to keep all the gadgets in sync. We can end up missing cases 
or detecting a code snippet both as safe and unsafe. The `isSafe` method would 
basically get rid of a whole class of problems, where safe and unsafe versions 
of the gadgets are overlapping. That being said, I am not insisting doing this 
change in this PR, it is OK doing the change later in a follow-up.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

jkorous wrote:
> NoQ wrote:
> > ziqingluo-90 wrote:
> > > steakhal wrote:
> > > > NoQ wrote:
> > > > > ziqingluo-90 wrote:
> > > > > > steakhal wrote:
> > > > > > > I would expect this test file to grow quite a bit.
> > > > > > > As such, I think we should have more self-descriptive names for 
> > > > > > > these functions.
> > > > > > > 
> > > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > > Thanks for the comment.  I agree that they should have better names 
> > > > > > or at least explaining comments.
> > > > > > 
> > > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > > 
> > > > > > I make all testing expressions arguments of `foo` so that I do not 
> > > > > > have to create statements to use these expressions while avoiding 
> > > > > > irrelevant warnings.
> > > > > That's pretty cool but please note that when `foo()` is declared this 
> > > > > way, it becomes a "C-style variadic function" - a very exotic 
> > > > > construct that you don't normally see in code (the only practical 
> > > > > example is the `printf`/`scanf` family of functions). So it may be 
> > > > > good that we cover this exotic case from the start, but it may also 
> > > > > be really bad that we don't cover the *basic* case.
> > > > > 
> > > > > C++ offers a different way to declare variadic functions: //variadic 
> > > > > templates// 
> > > > > (https://en.cppreference.com/w/cpp/language/parameter_pack). These 
> > > > > are less valuable to test because they expand to AST that's very 
> > > > > similar to the basic case, but that also allows you to cover the 
> > > > > basic case better.
> > > > > 
> > > > > Or you can always make yourself happy with a few overloads that cover 
> > > > > all your needs, it's not like we're worried about code duplication in 
> > > > > tests:
> > > > > ```lang=c++
> > > > > void foo(int);
> > > > > void foo(int, int);
> > > > > void foo(int, int, int);
> > > > > void foo(int, int, int, int);
> > > > > void foo(int, int, int, int, int);
> > > > > void foo(int, int, int, int, int, int);
> > > > > ```
> > > > IMO its fine. I would probably call it `sink()` though. Ive used the 
> > > > same construct for the same reason in CSA tests with this name.
> > > I don't quite get what "basic case" refers to.  Could you explain it to 
> > > me a little more?
> > By "basic case" I mean the ordinary, non-variadic function call with 
> > predefined set of arguments in the prototype.
> @ziqingluo-90 If the only purpose of `foo()` is to suppress warnings about 
> unused values then you might also consider just suppressing those warnings 
> with relevant `-Wno...` flags.
I now see that using a C variadic function could bring some unexpected 
behaviors such as implicit casts from lvalue reference to rvalue.   There are 
following patches whose tests also use `foo`.  I plan to add an NFC patch later 
to remove all uses of `foo`, if this sounds ok. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

NoQ wrote:
> ziqingluo-90 wrote:
> > steakhal wrote:
> > > NoQ wrote:
> > > > ziqingluo-90 wrote:
> > > > > steakhal wrote:
> > > > > > I would expect this test file to grow quite a bit.
> > > > > > As such, I think we should have more self-descriptive names for 
> > > > > > these functions.
> > > > > > 
> > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > Thanks for the comment.  I agree that they should have better names 
> > > > > or at least explaining comments.
> > > > > 
> > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > 
> > > > > I make all testing expressions arguments of `foo` so that I do not 
> > > > > have to create statements to use these expressions while avoiding 
> > > > > irrelevant warnings.
> > > > That's pretty cool but please note that when `foo()` is declared this 
> > > > way, it becomes a "C-style variadic function" - a very exotic construct 
> > > > that you don't normally see in code (the only practical example is the 
> > > > `printf`/`scanf` family of functions). So it may be good that we cover 
> > > > this exotic case from the start, but it may also be really bad that we 
> > > > don't cover the *basic* case.
> > > > 
> > > > C++ offers a different way to declare variadic functions: //variadic 
> > > > templates// 
> > > > (https://en.cppreference.com/w/cpp/language/parameter_pack). These are 
> > > > less valuable to test because they expand to AST that's very similar to 
> > > > the basic case, but that also allows you to cover the basic case better.
> > > > 
> > > > Or you can always make yourself happy with a few overloads that cover 
> > > > all your needs, it's not like we're worried about code duplication in 
> > > > tests:
> > > > ```lang=c++
> > > > void foo(int);
> > > > void foo(int, int);
> > > > void foo(int, int, int);
> > > > void foo(int, int, int, int);
> > > > void foo(int, int, int, int, int);
> > > > void foo(int, int, int, int, int, int);
> > > > ```
> > > IMO its fine. I would probably call it `sink()` though. Ive used the same 
> > > construct for the same reason in CSA tests with this name.
> > I don't quite get what "basic case" refers to.  Could you explain it to me 
> > a little more?
> By "basic case" I mean the ordinary, non-variadic function call with 
> predefined set of arguments in the prototype.
@ziqingluo-90 If the only purpose of `foo()` is to suppress warnings about 
unused values then you might also consider just suppressing those warnings with 
relevant `-Wno...` flags.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

ziqingluo-90 wrote:
> steakhal wrote:
> > NoQ wrote:
> > > ziqingluo-90 wrote:
> > > > steakhal wrote:
> > > > > I would expect this test file to grow quite a bit.
> > > > > As such, I think we should have more self-descriptive names for these 
> > > > > functions.
> > > > > 
> > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > Thanks for the comment.  I agree that they should have better names or 
> > > > at least explaining comments.
> > > > 
> > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > 
> > > > I make all testing expressions arguments of `foo` so that I do not have 
> > > > to create statements to use these expressions while avoiding irrelevant 
> > > > warnings.
> > > That's pretty cool but please note that when `foo()` is declared this 
> > > way, it becomes a "C-style variadic function" - a very exotic construct 
> > > that you don't normally see in code (the only practical example is the 
> > > `printf`/`scanf` family of functions). So it may be good that we cover 
> > > this exotic case from the start, but it may also be really bad that we 
> > > don't cover the *basic* case.
> > > 
> > > C++ offers a different way to declare variadic functions: //variadic 
> > > templates// (https://en.cppreference.com/w/cpp/language/parameter_pack). 
> > > These are less valuable to test because they expand to AST that's very 
> > > similar to the basic case, but that also allows you to cover the basic 
> > > case better.
> > > 
> > > Or you can always make yourself happy with a few overloads that cover all 
> > > your needs, it's not like we're worried about code duplication in tests:
> > > ```lang=c++
> > > void foo(int);
> > > void foo(int, int);
> > > void foo(int, int, int);
> > > void foo(int, int, int, int);
> > > void foo(int, int, int, int, int);
> > > void foo(int, int, int, int, int, int);
> > > ```
> > IMO its fine. I would probably call it `sink()` though. Ive used the same 
> > construct for the same reason in CSA tests with this name.
> I don't quite get what "basic case" refers to.  Could you explain it to me a 
> little more?
By "basic case" I mean the ordinary, non-variadic function call with predefined 
set of arguments in the prototype.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

steakhal wrote:
> NoQ wrote:
> > ziqingluo-90 wrote:
> > > steakhal wrote:
> > > > I would expect this test file to grow quite a bit.
> > > > As such, I think we should have more self-descriptive names for these 
> > > > functions.
> > > > 
> > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > Thanks for the comment.  I agree that they should have better names or at 
> > > least explaining comments.
> > > 
> > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > 
> > > I make all testing expressions arguments of `foo` so that I do not have 
> > > to create statements to use these expressions while avoiding irrelevant 
> > > warnings.
> > That's pretty cool but please note that when `foo()` is declared this way, 
> > it becomes a "C-style variadic function" - a very exotic construct that you 
> > don't normally see in code (the only practical example is the 
> > `printf`/`scanf` family of functions). So it may be good that we cover this 
> > exotic case from the start, but it may also be really bad that we don't 
> > cover the *basic* case.
> > 
> > C++ offers a different way to declare variadic functions: //variadic 
> > templates// (https://en.cppreference.com/w/cpp/language/parameter_pack). 
> > These are less valuable to test because they expand to AST that's very 
> > similar to the basic case, but that also allows you to cover the basic case 
> > better.
> > 
> > Or you can always make yourself happy with a few overloads that cover all 
> > your needs, it's not like we're worried about code duplication in tests:
> > ```lang=c++
> > void foo(int);
> > void foo(int, int);
> > void foo(int, int, int);
> > void foo(int, int, int, int);
> > void foo(int, int, int, int, int);
> > void foo(int, int, int, int, int, int);
> > ```
> IMO its fine. I would probably call it `sink()` though. Ive used the same 
> construct for the same reason in CSA tests with this name.
I don't quite get what "basic case" refers to.  Could you explain it to me a 
little more?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

NoQ wrote:
> ziqingluo-90 wrote:
> > steakhal wrote:
> > > I would expect this test file to grow quite a bit.
> > > As such, I think we should have more self-descriptive names for these 
> > > functions.
> > > 
> > > I'm also curious what's the purpose of `foo()`in the examples.
> > Thanks for the comment.  I agree that they should have better names or at 
> > least explaining comments.
> > 
> > > I'm also curious what's the purpose of `foo()`in the examples.
> > 
> > I make all testing expressions arguments of `foo` so that I do not have to 
> > create statements to use these expressions while avoiding irrelevant 
> > warnings.
> That's pretty cool but please note that when `foo()` is declared this way, it 
> becomes a "C-style variadic function" - a very exotic construct that you 
> don't normally see in code (the only practical example is the 
> `printf`/`scanf` family of functions). So it may be good that we cover this 
> exotic case from the start, but it may also be really bad that we don't cover 
> the *basic* case.
> 
> C++ offers a different way to declare variadic functions: //variadic 
> templates// (https://en.cppreference.com/w/cpp/language/parameter_pack). 
> These are less valuable to test because they expand to AST that's very 
> similar to the basic case, but that also allows you to cover the basic case 
> better.
> 
> Or you can always make yourself happy with a few overloads that cover all 
> your needs, it's not like we're worried about code duplication in tests:
> ```lang=c++
> void foo(int);
> void foo(int, int);
> void foo(int, int, int);
> void foo(int, int, int, int);
> void foo(int, int, int, int, int);
> void foo(int, int, int, int, int, int);
> ```
IMO its fine. I would probably call it `sink()` though. Ive used the same 
construct for the same reason in CSA tests with this name.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:168-169
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(

aaron.ballman wrote:
> xazax.hun wrote:
> > As per some of the discussions, in the future the compiler might be able to 
> > recognize certain safe patterns, e.g., when there is a simple for loop with 
> > known bounds, or when both the index and the array size is statically known.
> > 
> > I think here we need to make a very important design decision: Do we want 
> > the gadgets to have the right "safety" category when it is created (e.g., 
> > we have to be able to decide if a gadget is safe or not using matchers), or 
> > do we want some mechanisms to be able to promote an unsafe gadget to be a 
> > safe one? (E.g., do we want to be able to prove some unsafe gadgets safe 
> > using dataflow analysis in a later pass?)
> (FWIW, this is a great question and I really appreciate you asking it!)
My original design implies that safe gadgets are a separate hierarchy, so there 
will be a new gadget class for index zero, and this gadget will be changed to 
skip index zero. But I don't immediately see why such early separation is 
strictly necessary, other than for a bit of extra type safety (extra virtual 
methods of the `UnsafeGadget` class don't make sense on safe gadgets). We *do* 
have time to make this distinction later, before we get to emitting warnings.

So maybe eventually we'll end up replacing `isSafe()` with a pure virtual 
method and deprecate `SafeGadget` and `UnsafeGadget` base classes, if we see it 
cause too much duplication or it turns out that the extra analysis necessary to 
establish safety can't be nicely implemented in ASTMatchers. In this case I'll 
admit that I over-engineered it a bit.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

ziqingluo-90 wrote:
> steakhal wrote:
> > I would expect this test file to grow quite a bit.
> > As such, I think we should have more self-descriptive names for these 
> > functions.
> > 
> > I'm also curious what's the purpose of `foo()`in the examples.
> Thanks for the comment.  I agree that they should have better names or at 
> least explaining comments.
> 
> > I'm also curious what's the purpose of `foo()`in the examples.
> 
> I make all testing expressions arguments of `foo` so that I do not have to 
> create statements to use these expressions while avoiding irrelevant warnings.
That's pretty cool but please note that when `foo()` is declared this way, it 
becomes a "C-style variadic function" - a very exotic construct that you don't 
normally see in code (the only practical example is the `printf`/`scanf` family 
of functions). So it may be good that we cover this exotic case from the start, 
but it may also be really bad that we don't cover the *basic* case.

C++ offers a different way to declare variadic functions: //variadic 
templates// (https://en.cppreference.com/w/cpp/language/parameter_pack). These 
are less valuable to test because they expand to AST that's very similar to the 
basic case, but that also allows you to cover the basic case better.

Or you can always make yourself happy with a few overloads that cover all your 
needs, it's not like we're worried about code duplication in tests:
```lang=c++
void foo(int);
void foo(int, int);
void foo(int, int, int);
void foo(int, int, int, int);
void foo(int, int, int, int, int);
void foo(int, int, int, int, int, int);
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 478356.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -6,3 +6,57 @@
   --p; // expected-warning{{unchecked operation on raw buffer in expression}}
   p--; // expected-warning{{unchecked operation on raw buffer in expression}}
 }
+
+void foo(...);   // let arguments of `foo` to hold testing expressions
+
+void * voidPtrCall(void);
+char * charPtrCall(void);
+
+void testArraySubscripts(int *p, int **pp) {
+  foo(p[0], // expected-warning{{unchecked operation on raw buffer in expression}}
+  pp[0][0], // expected-warning2{{unchecked operation on raw buffer in expression}}
+  0[0[pp]], // expected-warning2{{unchecked operation on raw buffer in expression}}
+  0[pp][0]  // expected-warning2{{unchecked operation on raw buffer in expression}}
+  );
+
+  if (p[3]) {   // expected-warning{{unchecked operation on raw buffer in expression}}
+void * q = p;
+
+foo(((int*)q)[10]); // expected-warning{{unchecked operation on raw buffer in expression}}
+  }
+
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unchecked operation on raw buffer in expression}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unchecked operation on raw buffer in expression}}
+  charPtrCall()[3], // expected-warning{{unchecked operation on raw buffer in expression}}
+  3[charPtrCall()]  // expected-warning{{unchecked operation on raw buffer in expression}}
+  );
+
+  int a[10], b[10][10];
+
+  // not to warn subscripts on arrays
+  foo(a[0], a[1],
+  0[a], 1[a],
+  b[3][4],
+  4[b][3],
+  4[3[b]]);
+}
+
+void testArraySubscriptsWithAuto(int *p, int **pp) {
+  int a[10];
+
+  auto ap1 = a;
+
+  foo(ap1[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+
+  auto ap2 = p;
+
+  foo(ap2[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+
+  auto ap3 = pp;
+
+  foo(pp[0][0]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+
+  auto ap4 = *pp;
+
+  foo(ap4[0]);  // expected-warning{{unchecked operation on raw buffer in expression}}
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -149,6 +149,31 @@
 
   const Stmt *getBaseStmt() const override { return Op; }
 };
+
+/// Array subscript expressions on raw pointers as if they're arrays. Unsafe as
+/// it doesn't have any bounds checks for the array.
+class ArraySubscriptGadget : public UnsafeGadget {
+  const ArraySubscriptExpr *ASE;
+
+public:
+  ArraySubscriptGadget(const MatchFinder::MatchResult )
+  : UnsafeGadget(Kind::ArraySubscript),
+ASE(Result.Nodes.getNodeAs("arraySubscr")) {}
+
+  static bool classof(const Gadget *G) {
+return G->getKind() == Kind::ArraySubscript;
+  }
+
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(
+arraySubscriptExpr(hasBase(ignoringParenImpCasts(hasPointerType(
+.bind("arraySubscr"));
+  }
+
+  const Stmt *getBaseStmt() const override { return ASE; }
+};
 } // namespace
 
 // Scan the function and return a list of gadgets found with provided kits.
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
===
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -20,6 +20,7 @@
 
 UNSAFE_GADGET(Increment)
 UNSAFE_GADGET(Decrement)
+UNSAFE_GADGET(ArraySubscript)
 
 #undef SAFE_GADGET
 #undef UNSAFE_GADGET
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

steakhal wrote:
> I would expect this test file to grow quite a bit.
> As such, I think we should have more self-descriptive names for these 
> functions.
> 
> I'm also curious what's the purpose of `foo()`in the examples.
Thanks for the comment.  I agree that they should have better names or at least 
explaining comments.

> I'm also curious what's the purpose of `foo()`in the examples.

I make all testing expressions arguments of `foo` so that I do not have to 
create statements to use these expressions while avoiding irrelevant warnings.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added a comment.

> One test case I'd like to see is: `sizeof(p[0])` -- should code in an 
> unevaluated context be warned?

I think they should NOT be warned.  We haven't addressed the issue of 
unevaluated context in our patches.  I'm adding a test for code in unevaluated 
context so that we don't forget about it later.

> Can you also add tests for function declarations like:
>
>   void foo(int not_really_an_array[10]) { ... }
>   
>   template 
>   void bar(int (_an_array)[N]) { ... }
>   
>   // Using a dependent type but we know it's a pointer.
>   template 
>   void baz(Ty *ptr) { ... }
>   
>   // Using a dependent type where we have no idea if it's a pointer.
>   template 
>   void quux(Ty ptr) { ... }

Thanks for suggesting these test cases.   They have been added in one of the 
following patches (https://reviews.llvm.org/D138318). That patch improves the 
matchers to recognize these cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

I would expect this test file to grow quite a bit.
As such, I think we should have more self-descriptive names for these functions.

I'm also curious what's the purpose of `foo()`in the examples.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:168-169
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(

xazax.hun wrote:
> As per some of the discussions, in the future the compiler might be able to 
> recognize certain safe patterns, e.g., when there is a simple for loop with 
> known bounds, or when both the index and the array size is statically known.
> 
> I think here we need to make a very important design decision: Do we want the 
> gadgets to have the right "safety" category when it is created (e.g., we have 
> to be able to decide if a gadget is safe or not using matchers), or do we 
> want some mechanisms to be able to promote an unsafe gadget to be a safe one? 
> (E.g., do we want to be able to prove some unsafe gadgets safe using dataflow 
> analysis in a later pass?)
(FWIW, this is a great question and I really appreciate you asking it!)



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:16
+void testArraySubscripts(int *p, int **pp) {
+  foo(p[0], // expected-warning{{unchecked operation on raw buffer 
in expression}}
+  pp[0][0], // expected-warning2{{unchecked operation on raw 
buffer in expression}}

One test case I'd like to see is: `sizeof(p[0])` -- should code in an 
unevaluated context be warned?



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:43
+}
+
+void testArraySubscriptsWithAuto(int *p, int **pp) {

Can you also add tests for function declarations like:
```
void foo(int not_really_an_array[10]) { ... }

template 
void bar(int (_an_array)[N]) { ... }

// Using a dependent type but we know it's a pointer.
template 
void baz(Ty *ptr) { ... }

// Using a dependent type where we have no idea if it's a pointer.
template 
void quux(Ty ptr) { ... }
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:168-169
+  static Matcher matcher() {
+// FIXME: What if the index is integer literal 0? Should this be
+// a safe gadget in this case?
+return stmt(

As per some of the discussions, in the future the compiler might be able to 
recognize certain safe patterns, e.g., when there is a simple for loop with 
known bounds, or when both the index and the array size is statically known.

I think here we need to make a very important design decision: Do we want the 
gadgets to have the right "safety" category when it is created (e.g., we have 
to be able to decide if a gadget is safe or not using matchers), or do we want 
some mechanisms to be able to promote an unsafe gadget to be a safe one? (E.g., 
do we want to be able to prove some unsafe gadgets safe using dataflow analysis 
in a later pass?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137379/new/

https://reviews.llvm.org/D137379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits