Page MenuHomePhabricator

[analyzer] Correctly measure array size in security.insecureAPI.strcpy
AcceptedPublic

Authored by leanil on Mar 1 2018, 12:19 AM.

Details

Summary

This will handle those platforms that don't have 8-bit chars.
This is a follow up fix to review D41384, which has been committed since.

Diff Detail

Event Timeline

leanil created this revision.Mar 1 2018, 12:19 AM
leanil updated this revision to Diff 136474.Mar 1 2018, 12:28 AM

getQuantity() returns a signed type

xazax.hun accepted this revision.Mar 1 2018, 5:48 AM

LGTM!

This revision is now accepted and ready to land.Mar 1 2018, 5:48 AM
NoQ added inline comments.Mar 1 2018, 9:20 AM
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517

I suggest not using auto here, because it makes it harder to understand integer promotions (potential overflows or sign extensions) in the comparison.

leanil added inline comments.Mar 2 2018, 6:22 AM
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517

That's true. Should it be CharUnits::QuantityType then?

NoQ added a comment.May 4 2018, 5:51 PM

Sorry, i completely forgot about this one :(

I think this patch needs lit tests, eg. tell the analyzer to analyze a simple strcpy() call on any -target with non-8-bit chars and see if it's still crashes or behaves incorrectly.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
517

I think you should still getQuantity(), and then explicitly cast it to a type that's compatible with String->getLength(), so that there were no implicit integer casts around >=.