This is an archive of the discontinued LLVM Phabricator instance.

[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

Event Timeline

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.Dec 20 2017, 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.Jan 3 2018, 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.Jan 11 2018, 9:48 AM

Change result types to match the query return types.

leanil marked an inline comment as done.Jan 11 2018, 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.Jan 11 2018, 1:33 PM

Nest condition checking. Add positive test.

leanil marked 3 inline comments as done.Jan 11 2018, 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.Jan 11 2018, 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.Jan 11 2018, 1:47 PM
NoQ added a comment.Jan 11 2018, 1:48 PM

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

leanil updated this revision to Diff 129676.Jan 12 2018, 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.Jan 12 2018, 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.Jan 12 2018, 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.
dcoughlin added inline comments.Jan 21 2018, 2:19 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517

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.

leanil updated this revision to Diff 135795.Feb 24 2018, 6:38 AM
leanil marked an inline comment as done.

Use getTypeSizeInChars to get array size.