[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-02-24 Thread András Leitereg via Phabricator via cfe-commits 
leanil updated this revision to Diff 135795.
leanil marked an inline comment as done.
leanil added a comment.

Use `getTypeSizeInChars` to get array size.


https://reviews.llvm.org/D41384

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks.m


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as 
it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure 
as it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,17 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  auto ArraySize = BR.getContext().getTypeSizeInChars(Array).getQuantity();
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,17 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  auto ArraySize = BR.getContext().getTypeSizeInChars(Array).getQuantity();
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-21 Thread Devin Coughlin via Phabricator via cfe-commits 
dcoughlin added inline comments.



Comment at: 
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:517
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+  if (const auto *String = dyn_cast(Source)) {

Rather than dividing by '8', I suggest using ASTContext's getTypeSizeInChars(). 
This will make sure we handle those annoying platforms that don't have 8-bit 
chars.


Repository:
  rL LLVM

https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-12 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL322410: [analyzer] Dont flag strcpy of string literals 
into sufficiently large buffers. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D41384?vs=129676=129706#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41384

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  cfe/trunk/test/Analysis/security-syntax-checks.m


Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,17 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
Index: cfe/trunk/test/Analysis/security-syntax-checks.m
===
--- cfe/trunk/test/Analysis/security-syntax-checks.m
+++ cfe/trunk/test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as 
it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure 
as it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===


Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,17 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
Index: cfe/trunk/test/Analysis/security-syntax-checks.m
===
--- cfe/trunk/test/Analysis/security-syntax-checks.m
+++ cfe/trunk/test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-12 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:517
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr())) {
+  unsigned long long ArraySize = Array->getSize().getLimitedValue();

leanil wrote:
> NoQ wrote:
> > This can be simplified to `const auto *Array = 
> > DeclRef->getType()->getAs()`.
> > `.getTypePtr()` is almost always redundant because of the fancy 
> > `operator->()` on `QualType`.
> Using `getAs` yielded: 
> > error: static assertion failed: ArrayType cannot be used with getAs!
> 
> 
Whoops, yeah, right, array types are the rare exception. It should be 
`ASTContext.getAsConstantArrayType()`, see the docs for `getAsArrayType()` for 
some explanation of why it was made this way. I guess we don't really care 
about these aspects, so your code is fine :)


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-12 Thread András Leitereg via Phabricator via cfe-commits 
leanil marked 2 inline comments as done.
leanil added a comment.

In https://reviews.llvm.org/D41384#973851, @NoQ wrote:

> Do you have commit access or should someone else commit it for you?


I don't have, please commit it.




Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:517
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr())) {
+  unsigned long long ArraySize = Array->getSize().getLimitedValue();

NoQ wrote:
> This can be simplified to `const auto *Array = 
> DeclRef->getType()->getAs()`.
> `.getTypePtr()` is almost always redundant because of the fancy 
> `operator->()` on `QualType`.
Using `getAs` yielded: 
> error: static assertion failed: ArrayType cannot be used with getAs!




https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-12 Thread András Leitereg via Phabricator via cfe-commits 
leanil updated this revision to Diff 129676.
leanil marked an inline comment as done.
leanil added a comment.

Measure array size in bytes instead of elements.


https://reviews.llvm.org/D41384

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks.m


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as 
it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure 
as it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,17 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,17 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-11 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

Do you have commit access or should someone else commit it for you?


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-11 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:517
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr())) {
+  unsigned long long ArraySize = Array->getSize().getLimitedValue();

This can be simplified to `const auto *Array = 
DeclRef->getType()->getAs()`.
`.getTypePtr()` is almost always redundant because of the fancy `operator->()` 
on `QualType`.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:518
+DeclRef->getDecl()->getType().getTypePtr())) {
+  unsigned long long ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source)) {

Hmm, actually, `ArraySize` is the number of elements in the array, not its byte 
size, so you may want (if you like) to also suppress the warning when the array 
is not a `char` array (even if it's a weird code anyway) by instead taking 
`ASTContext.getTypeSize()` here instead.

Also i guess it's nice to use `uint64_t` because that's the return type of 
`.getLimitedValue()` and that's what we usually use all over the place when we 
have to deal with those values.


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-11 Thread András Leitereg via Phabricator via cfe-commits 
leanil marked 3 inline comments as done.
leanil added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:526
+  }
+  if (StrLenFound && ArraySize >= StrLen + 1)
+return;

george.karpenkov wrote:
> Why not put this if-expression into the one above where `StrLen` is found?
> That would eliminate `StrLenFound` and remove the potential error surface of 
> uninitialized read from `StrLen` (the declaration for which should probably 
> be inside this block as well)
Good point. This makes `StrLen` itself redundant as well.


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-11 Thread András Leitereg via Phabricator via cfe-commits 
leanil updated this revision to Diff 129508.
leanil added a comment.

Nest condition checking. Add positive test.


https://reviews.llvm.org/D41384

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks.m


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as 
it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure 
as it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,18 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr())) {
+  unsigned long long ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,16 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,18 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr())) {
+  unsigned long long ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source)) {
+if (ArraySize >= String->getLength() + 1)
+  return;
+  }
+}
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-11 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:522
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source)) {
+StrLen = String->getLength();

Nesting this if- inside the previous one would simplify the outer scope and let 
you assign to `ArraySize` at declaration size.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:526
+  }
+  if (StrLenFound && ArraySize >= StrLen + 1)
+return;

Why not put this if-expression into the one above where `StrLen` is found?
That would eliminate `StrLenFound` and remove the potential error surface of 
uninitialized read from `StrLen` (the declaration for which should probably be 
inside this block as well)



Comment at: test/Analysis/security-syntax-checks.m:151
+  char x[5];
+  strcpy(x, "abcd");
+}

I think it would make sense to add another test case where the warning is 
expected, even though string length and array size are known at compile time.


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-11 Thread András Leitereg via Phabricator via cfe-commits 
leanil updated this revision to Diff 129465.
leanil added a comment.

Change result types to match the query return types.


https://reviews.llvm.org/D41384

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks.m


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,11 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as 
it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,22 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  unsigned long long ArraySize = 0;
+  unsigned StrLen;
+  bool StrLenFound = false;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr()))
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source)) {
+StrLen = String->getLength();
+StrLenFound = true;
+  }
+  if (StrLenFound && ArraySize >= StrLen + 1)
+return;
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,11 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,22 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  unsigned long long ArraySize = 0;
+  unsigned StrLen;
+  bool StrLenFound = false;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr()))
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source)) {
+StrLen = String->getLength();
+StrLenFound = true;
+  }
+  if (StrLenFound && ArraySize >= StrLen + 1)
+return;
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-03 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

This patch makes a totally valid point :)




Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:513
 
+  int ArraySize = -1, StrLen = -1;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),

You might want to use a wider integer type because 64-bit arrays may have 2³¹ 
or more elements (not sure about string literals).


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2017-12-20 Thread András Leitereg via Phabricator via cfe-commits 
leanil updated this revision to Diff 127739.
leanil added a comment.

Move negative test next to the positive ones, because the necessary macros and 
run-lines are already defined there.


https://reviews.llvm.org/D41384

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks.m


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,11 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as 
it does not provide bounding of the memory buffer. Replace unbounded copy 
functions with analogous functions that support length arguments such as 
'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,18 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  int ArraySize = -1, StrLen = -1;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr()))
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source))
+StrLen = String->getLength();
+  if (StrLen != -1 && ArraySize >= StrLen + 1)
+return;
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -146,6 +146,11 @@
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,18 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  int ArraySize = -1, StrLen = -1;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr()))
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source))
+StrLen = String->getLength();
+  if (StrLen != -1 && ArraySize >= StrLen + 1)
+return;
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2017-12-20 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added reviewers: NoQ, george.karpenkov.
xazax.hun added a comment.

In the tests there are multiple variants of the strcpy function guarded by 
macros. Maybe we should run the tests multiple times to test all variants with 
different defines?


https://reviews.llvm.org/D41384



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

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2017-12-19 Thread András Leitereg via Phabricator via cfe-commits 
leanil created this revision.
leanil added reviewers: dcoughlin, xazax.hun.
Herald added subscribers: a.sidorin, rnkovacs, szepet.

It is safe to copy a string literal to an array which is compile time known to 
be large enough.
This reduces the number of false positives, while (hopefully) not introducing 
any false negatives.


https://reviews.llvm.org/D41384

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks-no-emit.c


Index: test/Analysis/security-syntax-checks-no-emit.c
===
--- test/Analysis/security-syntax-checks-no-emit.c
+++ test/Analysis/security-syntax-checks-no-emit.c
@@ -32,3 +32,31 @@
   rand_r();  // no-warning
   random();// no-warning
 }
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+//===--===
+// strcpy()
+//===--===
+#ifdef VARIANT
+
+#define __strcpy_chk BUILTIN(__strcpy_chk)
+char *__strcpy_chk(char *restrict s1, const char *restrict s2, size_t destlen);
+
+#define strcpy(a, b) __strcpy_chk(a, b, (size_t)-1)
+
+#else /* VARIANT */
+
+#define strcpy BUILTIN(strcpy)
+char *strcpy(char *restrict s1, const char *restrict s2);
+
+#endif /* VARIANT */
+
+void test_strcpy() {
+  char x[5];
+  strcpy(x, "abcd");
+}
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,18 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  int ArraySize = -1, StrLen = -1;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr()))
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source))
+StrLen = String->getLength();
+  if (StrLen != -1 && ArraySize >= StrLen + 1)
+return;
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);


Index: test/Analysis/security-syntax-checks-no-emit.c
===
--- test/Analysis/security-syntax-checks-no-emit.c
+++ test/Analysis/security-syntax-checks-no-emit.c
@@ -32,3 +32,31 @@
   rand_r();	// no-warning
   random();	// no-warning
 }
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+//===--===
+// strcpy()
+//===--===
+#ifdef VARIANT
+
+#define __strcpy_chk BUILTIN(__strcpy_chk)
+char *__strcpy_chk(char *restrict s1, const char *restrict s2, size_t destlen);
+
+#define strcpy(a, b) __strcpy_chk(a, b, (size_t)-1)
+
+#else /* VARIANT */
+
+#define strcpy BUILTIN(strcpy)
+char *strcpy(char *restrict s1, const char *restrict s2);
+
+#endif /* VARIANT */
+
+void test_strcpy() {
+  char x[5];
+  strcpy(x, "abcd");
+}
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -510,6 +510,18 @@
   if (!checkCall_strCommon(CE, FD))
 return;
 
+  int ArraySize = -1, StrLen = -1;
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast(Target))
+if (const auto *Array = dyn_cast(
+DeclRef->getDecl()->getType().getTypePtr()))
+  ArraySize = Array->getSize().getLimitedValue();
+  if (const auto *String = dyn_cast(Source))
+StrLen = String->getLength();
+  if (StrLen != -1 && ArraySize >= StrLen + 1)
+return;
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
 PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits