Page MenuHomePhabricator

[clang-tidy] Add bugprone-suspicious-memory-comparison check
AcceptedPublic

Authored by gbencze on Oct 18 2020, 10:26 AM.

Details

Summary

The check warns on suspicious calls to memcmp.
It currently checks for comparing types that do not have unique object representations or are non-standard-layout.
Based on
https://wiki.sei.cmu.edu/confluence/display/c/EXP42-C.+Do+not+compare+padding+data
https://wiki.sei.cmu.edu/confluence/display/c/FLP37-C.+Do+not+use+object+representations+to+compare+floating-point+values
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 and cert-flp37-c.

Some tests are currently failing at head, the check depends on D89649.
Originally started in D71973

Diff Detail

Event Timeline

gbencze created this revision.Oct 18 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2020, 10:26 AM
gbencze requested review of this revision.Oct 18 2020, 10:26 AM

Should point out there is already a check for cert-oop57-cpp, added in D72488. Do these play nice with each other or should they perhaps be merged or share code?

Should point out there is already a check for cert-oop57-cpp, added in D72488. Do these play nice with each other or should they perhaps be merged or share code?

I would certainly expect some duplicate warnings with there two checks, but as far as I can tell that check does not currently warn on using memcmp with non-standard-layout types.
I personally would leave the exp42 and flp37 parts of this check as separate, because those are useful in C as well. But maybe it'd be better to move the warning for non-standard-layout types from here to the existing one.

Should point out there is already a check for cert-oop57-cpp, added in D72488. Do these play nice with each other or should they perhaps be merged or share code?

I would certainly expect some duplicate warnings with there two checks, but as far as I can tell that check does not currently warn on using memcmp with non-standard-layout types.
I personally would leave the exp42 and flp37 parts of this check as separate, because those are useful in C as well. But maybe it'd be better to move the warning for non-standard-layout types from here to the existing one.

The thrust of the idea behind OOP57-CPP is that you shouldn't use C functions to do work that would normally be done via a C++ overloaded operator. e.g., don't call memcmp() on two structs, define the appropriate set of relational and equality operators for the type instead. It's required that you do this for non-trivial types, but it's aspirational to do it for all types. I don't think the proposed check should relate to OOP57-CPP all that much -- I see it more closely relating to EXP62-CPP instead: https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP62-CPP.+Do+not+access+the+bits+of+an+object+representation+that+are+not+part+of+the+object%27s+value+representation except that this check is currently only about comparisons and not accessing, so it's not quite perfect coverage.

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

The lint warning here is actually correct, which is a lovely change of pace.

71

Note that this may produce false positives as the list of objects with unique representations is not complete. For instance, it doesn't handle _Complex or _Atomic types, etc.

74

unique object representations -> a unique object representation

WDYT about:
consider comparing the values manually -> consider comparing %select{the values|the members of the object}0 manually

to make it more clear that these cases are different:

memcmp(&some_float, &some_other_float, sizeof(float));
memcmp(&some_struct, &some_other_struct, sizeof(some_struct));

The use of "values" make it sound a bit like the user should be able to do if (some_struct == some_other_struct) to me.

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
3

Wouldn't picking a 32-bit target suffice instead of -m32? e.g., -target i386-unknown-unknown

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
122

Just to make it obvious, I think a test like this would also be handy:

struct Whatever {
  int i[2];
  char c;
};

struct Whatever one, two;
memcmp(&one, &two, 2 * sizeof(int)); // Shouldn't warn either

which brings up a pathological case that I have no idea how it should behave:

struct Whatever {
  int i[2];
  int : 0; // What now?!
};

struct Whatever one, two;
memcmp(&one, &two, 2 * sizeof(int)); // Warn? Don't Warn? Cry?
199

How about a test where everything lines up perfectly for bit-fields?

struct Whatever {
  int i : 10;
  int j : 10;
  int k : 10;
  int l : 2;
};
_Static_assert(sizeof(struct Whatever) == sizeof(int));
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
230

Another case we should be careful of is template instantiations:

template <typename Ty>
void func(const Ty *o1, const Ty *o2) {
  memcmp(o1, o2, sizeof(Ty));
}

We don't want to diagnose that unless it's been instantiated with types that are a problem.

steakhal added a subscriber: steakhal.
gbencze updated this revision to Diff 303643.Sat, Nov 7, 8:06 AM
  • Added some new test cases
  • Fixed assertion in templates
  • Changed loop variable name from i to ArgIndex
  • Changed wording of warnings
  • Changed CHECK-MESSAGES to be in a single line: turns out that only the first line is checked as a prefix if clang-tidy breaks it into multiple lines
gbencze marked an inline comment as done.Sat, Nov 7, 8:16 AM
gbencze added inline comments.
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
46

Changed to ArgIndex

71

Yes, this could definitely cause some false positives. I'm not sure how we could easily fix it thought, especially if they are in some nested record.
Do you think this is a serious issue?

74

I wasn't entirely happy with the wording either; changed it according to your suggestions.

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
3

To be fully honest, I have no idea, I saw some other tests using -m32 so I decided to copy that.

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
122

Added two new test cases for these: Test_TrailingPadding2 and Bitfield_TrailingUnnamed.
Purely empirically the latter one seems to have a size of 2*sizeof(int), so I assume there is no need to warn there.

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

Thanks, I added two new tests for these. I also made a minor fix in registerMatchers to make sure I don't try to evaluate a dependant expression.

aaron.ballman added inline comments.Tue, Nov 17, 8:22 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
71

I don't think it's a serious issue except for possibly the _Atomic case in C code. It may be worth testing that scenario explicitly and putting a FIXME comment on the test case.

77

It looks like this part of the string literal can be moved up a line.

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

May also want to mention EXP62-CPP which is similarly related to OOP57-CPP -- but the docs should be clear that this isn't a check for those rules, just that it has some partial overlap.

17

Type -> Types
representations -> representation

19

representaions -> representation

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
122

Thanks for the new test cases, and I think that emergent behavior is reasonable.

gbencze updated this revision to Diff 306679.Fri, Nov 20, 6:36 AM

Address comments:

  • add new test case for _Atomic (with a FIXME comment)
  • fix string literal formatting
  • update docs
aaron.ballman accepted this revision.Fri, Nov 20, 8:30 AM

LGTM, thank you for the new check!

This revision is now accepted and ready to land.Fri, Nov 20, 8:30 AM

Thanks for the review! I'll push this as soon as the updated version of D89649 is also accepted.