This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add bugprone-suspicious-memory-comparison check
Needs ReviewPublic

Authored by gbencze on Dec 29 2019, 9:07 AM.

Details

Summary

The check warns on suspicious calls to memcmp.
It currently checks for comparing padding and non-standard-layout types.
Based on
https://wiki.sei.cmu.edu/confluence/display/c/EXP42-C.+Do+not+compare+padding+data
and part of
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions
Add alias cert-exp42-c.

Diff Detail

Event Timeline

gbencze created this revision.Dec 29 2019, 9:07 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
39

You could use const auto * because type is specified in same statement.

45

Please don't use auto when type is not specified in same statement or iterator.

129

Please don't use auto when type is not specified in same statement or iterator.

140

You could use const auto * because type is specified in same statement.

clang-tools-extra/docs/ReleaseNotes.rst
103

Please move into aliases section (in alphabetical order)

gbencze updated this revision to Diff 235526.Dec 29 2019, 3:37 PM

Update style based on comments.

gbencze marked 5 inline comments as done.Dec 29 2019, 3:41 PM
gbencze added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
103

I'm not quite sure where these are supposed to be. I can only find one other alias (cert-pos44-c) in the release notes, should I put it immediately before that?

Eugene.Zelenko added inline comments.Dec 29 2019, 4:10 PM
clang-tools-extra/docs/ReleaseNotes.rst
103

In 9.0 Release Notes aliases were placed after new checks.

gbencze updated this revision to Diff 235564.Dec 30 2019, 1:21 AM

ReleaseNotes: move alias after new checks

gbencze marked 2 inline comments as done.Dec 30 2019, 1:21 AM
JonasToth added inline comments.Dec 30 2019, 5:53 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
20

I think you can remove the constant and hardcode it in the matcher. that is common practice and because its used twice not a big issue (and shorter).

22

You are in clang namespace, so you can ellide the clang:: here and in the following functions/decls.

28

Maybe return FD.isBitField() ? FD.getBitWidthValue(Ctx) : Ctx.getTypeSize(FieldType);? I find it cleaner, but its preference i guess.

47

value-types are not defined as const per coding guideline.

63

plz no const

64

a short descriptive error message for this assertion would be great.

67

Please make this comment a full sentence with punctuation. I think it should be above the if.

73

const

77

Please make this comment a full sentence with punctuation.

Both ifs can be merged to one with an &&?

89

Please provide an error message

99

Full sentence for comment.

110

No else after return. You can simply return None;

126

Always true, otherwise matcher don't work / didn't fire. I think this assert is not required.

129

please provide a short error message why this should always be true (i guess because memcmp is standardized and stuff, but when reading this code alone it might not be super obvious).

130

value-type that is const, please remove the const.

135

const, next line the * const is uncommon in llvm-code. I think you should remove the pointer-consting. The other pointers are not const either.

137

error message.

153

i think this diagnostic could be a bit more expressive, similar to the one above.
Unexperienced programmer might not understand the issue or even know what padding bytes are.

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
2

We should explicitly test, that the check runs under C-code, either with two run-lines or with separate test files (my preference).

4

I think the test could be sensitive to cpu-architectures.
If you could bring up a case that is e.g. problematic on X86, but not on anything else (powerpc, or arm or anything, 64bit vs 32bit) and demonstrate with tests that such cases are caught as well this would be great. I see no reason it wouldn't.

That would maybe justify another alias into portability, as this is an issue in that case.

gbencze updated this revision to Diff 235900.Jan 2 2020, 10:03 AM

Coding guide and better diagnostic message for padding comparison

gbencze marked 18 inline comments as done.Jan 2 2020, 10:05 AM
gbencze added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
2

Should I duplicate the tests that are legal C or try to split it up so that only c++ specific code is tested in this one?

4

I may have misunderstood you but the check currently only warns if you're comparing padding on the current compilation target.
Or do you mean adding another test file that is compiled e.g. with -m32 and testing if calling memcmp on the following doesn't warn there but does on 64bit?

struct S {
    int x;
    int* y;
};
JonasToth added inline comments.Jan 2 2020, 10:34 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
2

Rather splitting. Duplication might be painfull in the future.

4

Yes. Because it is only tested for the current archtiecture the buildbots will probably fail on it, because they test many architectures.

I think it is necessary to specify the arch in the RUN-line (%t -- -- -target x86_64-unknown-unknown) at the end of it.

And yes: Another test-file would be great to demonstrate that it works as expected.

gbencze updated this revision to Diff 235915.Jan 2 2020, 11:43 AM

Tests: Split C/C++ tests and add 32/64bit specific test.

LG from my side, besides the small nits.
Please let @aaron.ballman have a look as well.

thanks for the patch :)

clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
45

Nit: Please make this comment a full sentence with proper punctuation.

clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
10

Maybe this link is not proper, because of the newline. could you please check if the documentation builds? (you need sphinx for that and enable it in cmake.)

clang-tools-extra/docs/clang-tidy/checks/list.rst
75

i believe the classification of the checks was changed today. did you rebase to master? hopefully it still applies clean.

gbencze updated this revision to Diff 235965.Jan 2 2020, 4:03 PM

Punctuation in comment

gbencze marked 4 inline comments as done.Jan 2 2020, 4:07 PM

Thanks for all the feedback @JonasToth :)

clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
10

It seems to work fine

clang-tools-extra/docs/clang-tidy/checks/list.rst
75

I did rebase today and removed the classification for these two by hand

ping @aaron.ballman - any thoughts on the patch?

aaron.ballman added inline comments.Jan 10 2020, 8:32 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
31

Drop top-level const qualifiers on locals (it's not a style we typically use), elsewhere as well.

83–84

I am surprised that we have to do this much manual work, I would have expected this to be something that RecordLayout would handle for us. I am a bit worried about this manual work being split in a few places because we're likely to get it wrong in at least one of them. For instance, this doesn't seem to be taking into account things like [[no_unique_address]] or alignment when considering the layout of fields.

I sort of wonder if we should try using the RecordLayout class to do this work instead, even if that requires larger changes.

131

Can you add a test case like this:

struct S {
  operator void *() const;
};

S s;
memcmp(s, s, sizeof(s));

to ensure that this assertion doesn't trigger?

134–135

Sink the declaration into the if conditional to limit its scope.

141

You can pass in PointeeType->getAsTagDecl() -- the diagnostic printer will automatically get the correct name from any NamedDecl.

149

Same here as above.

gbencze updated this revision to Diff 237529.Jan 12 2020, 2:59 AM

Address (most of the) comments by @aaron.ballman

  • remove top-level const on locals
  • move declaration into if
  • pass TagDecl to diag
  • added test for operator void *
  • fixed [[no_unique_address]]
    • remove assertion that checks for overlapping fields
    • in hasPaddingBetweenFields: only do recursive call and add field size to TotalSize if not isZeroSize
    • + added tests
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
83–84

Thanks, I didn't realize the [[no_unique_address]] attribute existed but I fixed it (in this check for now) and also added some tests to cover it as well as alignas.

If you think this information should be provided by RecordLayout I'm willing to work on that as well (though I guess those changes should be in a different patch). Do you have any ideas as to how it should expose the necessary information?

Another option that came to my mind is using a BitVector to (recursively) flag bits that are occupied by the fields. This solution would be slightly slower and uses more memory but is probably a lot easier to understand, maintain and more robust than the currently proposed implementation. This would also catch a few additional cases as it could also look inside unions.

I stared to experiment with an implementation of this here.

Which direction do you think would be a better solution?

Another option that came to my mind is using a BitVector to (recursively) flag bits that are occupied by the fields. This solution would be slightly slower and uses more memory but is probably a lot easier to understand, maintain and more robust than the currently proposed implementation. This would also catch a few additional cases as it could also look inside unions.

I stared to experiment with an implementation of this here.

Which direction do you think would be a better solution?

This is a neat approach, but I think I still prefer asking the RecordLayout for this information if possible.

clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
83–84

I do think this should be provided by RecordLayout (and yes, please as a separate patch -- feel free to add myself and Richard Smith as reviewers for it). Intuitively, I would expect to be able to query for whether a record has any padding within it whatsoever. For your needs, I would imagine an interface like bool hasPaddingWithin(CharUnits InitialOffset, CharUnits FinalOffset) would also help -- though I suspect we may not want to use CharUnits because of bit-fields that may have padding due to 0-sized bit-fields.

dkrupp added a subscriber: dkrupp.Jul 3 2020, 5:13 AM
MTC added a subscriber: MTC.Aug 16 2021, 7:13 PM