Page MenuHomePhabricator

[clang-tidy] Add new checker for suspicious sizeof expressions
ClosedPublic

Authored by etienneb on Apr 12 2016, 8:30 AM.

Details

Summary

This check is finding suspicious cases of sizeof expression.

Sizeof expression is returning the size (in bytes) of a type or an
expression. Programmers often abuse or misuse this expression.

This checker is adding common set of patterns to detect some
of these bad constructs.

Some examples found by this checker:

R/packages/ifultools/ifultools/src/fra_neig.c

/* free buffer memory */
(void) mutil_free( dist_buff, sizeof( ctr * sizeof( double ) ) );
(void) mutil_free( nidx_buff, sizeof( ctr * sizeof( sint32 ) ) );

graphviz/v2_20_2/lib/common/utils.c

static Dtdisc_t mapDisc = {
    offsetof(item, p),
    sizeof(2 * sizeof(void *)),
    offsetof(item, link),
    (Dtmake_f) newItem,
    (Dtfree_f) freeItem,
    (Dtcompar_f) cmpItem,
    NIL(Dthash_f),
    NIL(Dtmemory_f),
    NIL(Dtevent_f)
};

mDNSResponder/mDNSShared/dnsextd.c

	context = ( TCPContext* ) malloc( sizeof( TCPContext ) );
	require_action( context, exit, err = mStatus_NoMemoryErr; LogErr( "AcceptTCPConnection", "malloc" ) );
	mDNSPlatformMemZero( context, sizeof( sizeof( TCPContext ) ) );
	context->d		 = self;

Diff Detail

Event Timeline

etienneb updated this revision to Diff 53405.Apr 12 2016, 8:30 AM
etienneb retitled this revision from to [clang-tidy] Add new checker for suspicious sizeof expressions.
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
alexfh edited edge metadata.Apr 12 2016, 8:49 AM

The check is really awesome! Thank you for coming up with all the patterns that frequently happen to result from coding errors!

Please update release notes.

A few inline comments as well.

clang-tidy/misc/SizeofExpressionCheck.cpp
23

Seems like you could use the isAnyCharacter matcher.

28

nit: extra 'n' in "relational"

28

I'd put this to ASTMatchers.h together with a test and docs.

41

The !(E = ...) construct is a bit hard to read (needs time to figure out the intention). I'd prefer a simpler alternative.

197

It looks like you've created a tool for what can be done in a simpler way. It should be possible to express this matcher without going through the parent map: sizeOfExpr(hasDescendant(sizeOfExpr(unless(SizeOfZero)))) or something like that.

237

I'm not sure I would understand what "incompatible" means here without reading the code of the check. Please expand the message. Same in the messages below.

259

s/multiplation/multiplication/

etienneb updated this revision to Diff 53428.Apr 12 2016, 10:53 AM
etienneb marked 6 inline comments as done.
etienneb edited edge metadata.

fix alexfh@ comments

etienneb updated this revision to Diff 53429.Apr 12 2016, 10:57 AM

release notes.

etienneb updated this revision to Diff 53431.Apr 12 2016, 11:08 AM

more unittests

etienneb updated this revision to Diff 53436.Apr 12 2016, 11:27 AM

fix typos and doc.

etienneb updated this object.Apr 12 2016, 11:28 AM
alexfh added inline comments.Apr 12 2016, 11:39 AM
clang-tidy/misc/SizeofExpressionCheck.cpp
198

What about this comment?

204

nit: missing trailing question mark

210

nit: 'strlen' should be in single quotes for consistency with other messages.

258

nit: I'd say of 'sizeof' by 'sizeof' instead of of two 'sizeof'.

docs/clang-tidy/checks/misc-sizeof-expression.rst
59

nit: the titles need to be consistent - either all have or not have warning:.

I'd also make the first letters capital.

103

s/make/makes/

etienneb updated this revision to Diff 53437.Apr 12 2016, 11:46 AM
etienneb marked 5 inline comments as done.

alexfh comments.

clang-tidy/misc/SizeofExpressionCheck.cpp
28

There are a few matchers used in different checkers that worth lifting.
It seems there is a place to lift some of them in clang-tidy/utils, and some of them should be lifted to astmatcher directly.
I wonder what should be the rules to lift them.

197

This is 'almost' working!
We cannot pass through all expression.
As an example, callExpr or arrayExpr

sizeof( A[index(sizeof(int))] )

In this example, the checker won't warn.

So, the hasDescendant (or has ancestor) in this case is only allow to follow a subset of Expressions.

The name may be wrong, or unclear. If you have better proposition...

198

unsubmited. (not sure to get when arc is pushing them)

alexfh added inline comments.Apr 12 2016, 12:26 PM
clang-tidy/misc/SizeofExpressionCheck.cpp
29

There are no firm rules. It mostly depends on how generic/useful in other tools the matcher could be. This one seems pretty generic and it could be used in a couple of other clang-tidy checks at least (e.g. readability/ContainerSizeEmptyCheck.cpp), so it seems to be a good fit for ASTMatchers.h. This can also be done as a separate step, if you prefer.

198

I see the reasoning, but in this case I'd still prefer if this custom matcher looked at the children instead of the parents, since it's a more effective approach in general (at least because it doesn't use the parent map).

We should probably add a variant of hasDescendant (and maybe hasAncestor) with a way to limit which nodes it can look through.

258

"'sizeof' by 'sizeof' expression" is ambiguous, "'sizeof' by 'sizeof' multiplication" would be better.

alexfh requested changes to this revision.Apr 12 2016, 12:27 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 12 2016, 12:27 PM
etienneb updated this revision to Diff 53442.Apr 12 2016, 12:40 PM
etienneb edited edge metadata.
etienneb marked an inline comment as done.

alexfh comments.

alexfh added inline comments.Apr 12 2016, 12:58 PM
clang-tidy/misc/SizeofExpressionCheck.cpp
199

I'm probably wrong about "a more effective approach in general", but for sizeof case it might be more effective, if we assume that sizeof doesn't have large subtrees under it.

A separate concern is that the matchers looking up the tree are somewhat more difficult to understand (at least to me).

Ultimately, I'm not sure what's the best approach here. Maybe find a huge file and measure runtime of both implementations? ;)

alexfh accepted this revision.Apr 12 2016, 1:03 PM
alexfh edited edge metadata.

Looks good with one more nit. I'm not sure what to do with hasSizeOfAncestor, we can leave it as is for now.

clang-tidy/misc/SizeofExpressionCheck.cpp
48

The type here doesn't help, I'd use auto.

This revision is now accepted and ready to land.Apr 12 2016, 1:03 PM
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
100

Please put this check after misc-misplaced-widening-cast.

malcolm.parsons added inline comments.
docs/clang-tidy/checks/misc-sizeof-expression.rst
17

s/it's/its/

etienneb updated this revision to Diff 53609.Apr 13 2016, 1:15 PM
etienneb marked 2 inline comments as done.
etienneb edited edge metadata.

alexfh@ comment: top-down approach.

clang-tidy/misc/SizeofExpressionCheck.cpp
29

I'm not against at all to lift this.
Now, or in a separate step.

I just leave it here so the patch is still working as-is (if people want to play with it and found invalid cases!).

198

I was thinking it's more effective to look to ancestor than descendants?
There are less ancestors than descendants?

199

I'm uploading the top-down approach.
As I suspected, this is changing nothing on large cases I tried.

I put the limit of 8 level depth, which won't cause any combinatory explosion.
Worse case: 2^8 nodes.

etienneb closed this revision.Apr 15 2016, 9:41 AM