[analyzer] Suppress false positive warnings form security.insecureAPI.strcpy
ClosedPublic

Authored by leanil on Dec 19 2017, 3:00 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM
leanil created this revision.Dec 19 2017, 3:00 AM

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?

leanil updated this revision to Diff 127739.Wed, Dec 20, 8:47 AM

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

NoQ added a comment.Wed, Jan 3, 3:18 PM

This patch makes a totally valid point :)

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
513 ↗(On Diff #127739)

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

leanil updated this revision to Diff 129465.Thu, Jan 11, 9:48 AM

Change result types to match the query return types.

leanil marked an inline comment as done.Thu, Jan 11, 9:49 AM
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
522 ↗(On Diff #129465)

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

526 ↗(On Diff #129465)

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)

test/Analysis/security-syntax-checks.m
151 ↗(On Diff #129465)

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.

leanil updated this revision to Diff 129508.Thu, Jan 11, 1:33 PM

Nest condition checking. Add positive test.

leanil marked 3 inline comments as done.Thu, Jan 11, 1:37 PM
leanil added inline comments.
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
526 ↗(On Diff #129465)

Good point. This makes StrLen itself redundant as well.

NoQ accepted this revision.Thu, Jan 11, 1:47 PM
NoQ added inline comments.
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517 ↗(On Diff #129508)

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

518 ↗(On Diff #129508)

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.

This revision is now accepted and ready to land.Thu, Jan 11, 1:47 PM
NoQ added a comment.Thu, Jan 11, 1:48 PM

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

leanil updated this revision to Diff 129676.Fri, Jan 12, 11:58 AM
leanil marked an inline comment as done.

Measure array size in bytes instead of elements.

leanil marked 2 inline comments as done.Fri, Jan 12, 12:03 PM
In D41384#973851, @NoQ wrote:

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

I don't have, please commit it.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517 ↗(On Diff #129508)

Using getAs yielded:

error: static assertion failed: ArrayType cannot be used with getAs!

NoQ added inline comments.Fri, Jan 12, 12:08 PM
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517 ↗(On Diff #129508)

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 :)

This revision was automatically updated to reflect the committed changes.