Page MenuHomePhabricator

[clang-tidy] Add bugprone-suspicious-memset-usage check.
ClosedPublic

Authored by rnkovacs on May 1 2017, 6:06 AM.

Details

Summary

Created new module bugprone and placed the check in that.

Finds memset() calls with potential mistakes in their arguments.

Replaces and extends the existing google-runtime-memset-zero-length check. Cases covered:

  • Fill value is a character '0'. Integer 0 might have been intended.
  • Fill value is out of char range and gets truncated.
  • Byte count is zero. Potentially swapped with the fill value argument.

Hits on LLVM codebase:

Diff Detail

Repository
rL LLVM

Event Timeline

rnkovacs created this revision.May 1 2017, 6:06 AM

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/misc-suspicious-memset-usage.rst
6 ↗(On Diff #97279)

Please enclose memset on `` and add ().

25 ↗(On Diff #97279)

Please enclose memset on `` and add ().

Eugene.Zelenko set the repository for this revision to rL LLVM.
whisperity edited the summary of this revision. (Show Details)May 2 2017, 12:43 AM
rnkovacs edited the summary of this revision. (Show Details)May 2 2017, 1:40 AM

The destination is a this pointer within a class that has a virtual function. It might reset the virtual pointer.

Can you change this to match any pointer to a class with a virtual function?

I'd also like a warning for a pointer to a class with a constructor or destructor.

rnkovacs updated this revision to Diff 97646.May 3 2017, 8:05 AM
rnkovacs edited the summary of this revision. (Show Details)
  • Fixed function call format in docs.
  • Added check to release notes.

Can you change this to match any pointer to a class with a virtual function?

Well, I have just found a clang flag -Wdynamic-class-memaccess that finds all of these cases and is enabled by default. I guess I need to drop the this case entirely.

I'd also like a warning for a pointer to a class with a constructor or destructor.

This is a nice idea, I'll do it.

rnkovacs updated this revision to Diff 97917.May 5 2017, 1:45 AM
rnkovacs edited the summary of this revision. (Show Details)
  • Removed case related to virtual pointers as there is a diagnostic flag for that.
  • Added case warning for calls on classes with a constructor or destructor. Changed tests and docs accordingly.
xazax.hun added inline comments.May 7 2017, 3:24 AM
clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp
21 ↗(On Diff #97917)

I think this might not be the best approach.

For example, if the constructor is compiler generated, but there is a member of the class with non-trivial constructor, we still want to warn.

E.g.:

struct X { X() { /* something nontrivial */ } };

struct Y { X x; };

Maybe we should check instead whether the class is a POD? Other alternative might be something like
CXXRecordDecl::hasNonTrivialDefaultConstructor.

The current proposition could be that we only keep the first two cases, possibly merging in the google check for a third case (with its old name evoking original functionality). Separately, another check could be written that warns when the below mentioned memory management functions are used on not TriviallyCopyable objects.

I wonder if any of the commenters/reviewers/subscribers have any thoughts about this :)

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp
21 ↗(On Diff #97917)

So, we had a discussion yesterday that I'll try to sum up here. The root of the problem is not exactly about constructors or the object being a POD, but rather about calling memset() on an object that is not TriviallyCopyable. But as you suggested, this holds for memcpy() and memmove() as well, and might be better placed in another check.

JDevlieghere added a project: Restricted Project.May 11 2017, 10:54 AM
JDevlieghere added a subscriber: JDevlieghere.
alexfh edited edge metadata.May 17 2017, 5:04 AM

The existing google-runtime-memset-zero-length check is related. It finds cases when the byte count parameter is zero and offers to swap that with the fill value argument. Perhaps the two could be merged while maintaining backward compatibility through an alias.

I think, the checks should be merged, and there's no need to maintain the old behavior under the google-runtime-memset-zero-length alias. I'm not even sure we want to make this alias, as long as the new check isn't ridiculously noisy.

At this point I'd suggest that we go back to the idea of adding a separate module for checks that target patterns that are likely to be bugs rather than performance, readability or style issues. The name could be "bugprone" or something like this, and we could move many misc- checks there eventually.

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp
21 ↗(On Diff #97917)

This logic could be either here or in a separate check (covering 'memcpy', 'memmove' and friends), but it might even be reasonable to create a compiler diagnostic for this eventually (maybe after a test drive of the logic in a clang-tidy check).

rnkovacs updated this revision to Diff 105401.Jul 6 2017, 5:52 AM
rnkovacs edited the summary of this revision. (Show Details)
rnkovacs added a subscriber: szepet.Jul 6 2017, 6:54 AM
rnkovacs updated this revision to Diff 106172.Jul 12 2017, 4:36 AM
rnkovacs retitled this revision from [clang-tidy] Add misc-suspicious-memset-usage check. to [clang-tidy] Add bugprone-suspicious-memset-usage check..
rnkovacs edited the summary of this revision. (Show Details)
  • Created new module bugprone and moved the check into that.
  • Included summary of hits on LLVM in the description.
whisperity requested changes to this revision.Jul 12 2017, 5:05 AM

Considering the results published in the opening description:

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:55:29: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]

std::memset(NumberBuffer, '0', sizeof(NumberBuffer));
                          ^~~~
                          0

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:148:26: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]

::memset(NumberBuffer, '0', llvm::array_lengthof(NumberBuffer));
                       ^~~~
                       0

In these case, the buffer is of char[] type. This memset call initialises the array with the character literal ASCII 0, which is, in this context, the expected behaviour. Maybe you should add an exemption from the checker for these cases, i.e. where we do know that the buffer is a char[].

Tests should be extended with a case for this addition.

This revision now requires changes to proceed.Jul 12 2017, 5:05 AM
alexfh added inline comments.Jul 12 2017, 8:36 AM
clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp
127–130 ↗(On Diff #106172)

It looks like this can be replaced with clang::tooling::fixit::getText() or even createReplacement.

rnkovacs updated this revision to Diff 106268.Jul 12 2017, 12:00 PM
rnkovacs edited edge metadata.
  • Added char[] exception along with a test case. There are no more false positives on LLVM.
  • Simplified fix-its by using clang::tooling::fixit functions.
whisperity resigned from this revision.Jul 12 2017, 11:53 PM
whisperity added inline comments.
docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst
10 ↗(On Diff #106268)

Shouldn't this '0' be enclosed within backticks?

rnkovacs updated this revision to Diff 106381.Jul 13 2017, 12:12 AM
rnkovacs marked an inline comment as done.
rnkovacs added inline comments.
docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst
10 ↗(On Diff #106268)

Fixed that and the too short title underline.

alexfh accepted this revision.Jul 14 2017, 3:47 AM

Looks good with a single comment.

Thank you for working on this!

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp
74–75 ↗(On Diff #106381)

The formatting is rather misleading here. Let's put the comment inside the if. Same below.

This revision is now accepted and ready to land.Jul 14 2017, 3:47 AM
whisperity added inline comments.Jul 14 2017, 4:18 AM
clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp
57–58 ↗(On Diff #106381)

@alexfh Your review on putting the comments within their applicable branch bodies applies here too?

89–90 ↗(On Diff #106381)

Same as L57-58.

rnkovacs updated this revision to Diff 106620.Jul 14 2017, 4:47 AM
rnkovacs marked an inline comment as done.

Moved comments inside if bodies.

rnkovacs marked 4 inline comments as done.Jul 14 2017, 4:48 AM
This revision was automatically updated to reflect the committed changes.