[PATCH] [StaticAnalyzer] New checker Sizeof on expression

[Daniel Marjamäki]

Hello!

> Do we have evidence that we want this? Does it catch bugs?

I have some experience with such checking, Cppcheck has had such checking for a while. It does catch bugs sometimes. I would not say that it hits 'commonly'. I haven't seen any reports about false positives yet. So I guess Cppcheck users choose to ignore or suppress these false positives.

In addition to user reports, I also test Cppcheck myself. I don't know how many projects I have scanned with Cppcheck personally since the check was added, but it's somewhere in the range 20-50. And in these I don't saw such false positives. I scan projects that people report they have trouble with. I scan some legacy code that I have access to. I also scan some well-known large and popular projects.

> If so, what do they look like?

I think the most interesting mistake is when there is misplaced parentheses. Such as this:

http://netbsd.2816.n7.nabble.com/bin-44773-Calculation-inside-sizeof-td108718.html#a31244258

         return (off_t)(sizeof(HD_VCPIO) + sizeof(TRAILER +
                       (VCPIO_PAD(sizeof(HD_VCPIO) + sizeof(TRAILER))))); 


About sizeof(sizeof(x)) I have seen couple of such fixes also. Here is one example (Apache HTTP server):

        sa->nLength = sizeof(sizeof(SECURITY_ATTRIBUTES));

This is the fixed code:

        sa->nLength = sizeof(SECURITY_ATTRIBUTES);

I've also seen a mistake when there is no parentheses (I don't rem):
    sizeof sizeof XYZ


> sizeof(expression) is a common idiom in SFINAE contexts. Is that covered here?

Interesting, I was not aware of this. I don't know what this common idiom looks like. Do you know some FOSS project that uses SFINAE (I could try this checker on it)?


> sizeof(sizeof(int)) is a "traditional" way to get sizeof(size_t). Why should we warn on that?

I was not aware of this idiom neither. I thought sizeof(size_t) was better.

If it's "traditional" code then it sounds like this is common in legacy code. Do you think anybody would dislike changing sizeof(sizeof(int)) for non-legacy code? For legacy code I assume most warnings are unwanted.


Best regards,
Daniel Marjamäki



..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile:                 +46 (0)709 12 42 62
E-mail:                 Daniel.Marjamaki at evidente.se

www.evidente.se