Page MenuHomePhabricator

[analyzer] Fix displayed checker name for InnerPointerChecker
ClosedPublic

Authored by rnkovacs on Aug 2 2018, 6:39 PM.

Details

Summary

For InnerPointerChecker to function properly, both the checker itself and MallocChecker's capabilities that handle relevant use-after-free problems need to be turned on. So far, the latter part has been developed under the name of MallocChecker's NewDelete sub-checker, often causing warnings to appear under that name. This patch defines a new CheckKind within MallocChecker for the inner pointer checking functionality, so that the correct name is displayed on warnings.

Tested on clang-tidy.

Diff Detail

Event Timeline

rnkovacs created this revision.Aug 2 2018, 6:39 PM
NoQ accepted this revision.Aug 2 2018, 7:49 PM

I see, so that's how it's done!

I also noticed that checker name was weird in exploded graph dumps, i.e. it was showing regular new/delete stuff as if it was done by InnerPointer checker. I'll check if this is fixed tomorrow.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1689

It's usually just None, but i guess whoever touched this code before didn't know; it's pretty hard to discover.

This revision is now accepted and ready to land.Aug 2 2018, 7:49 PM
In D50211#1186630, @NoQ wrote:

I see, so that's how it's done!

I also noticed that checker name was weird in exploded graph dumps, i.e. it was showing regular new/delete stuff as if it was done by InnerPointer checker. I'll check if this is fixed tomorrow.

This seemed interesting, so I tried out myself on the NewDelete-path-notes.cpp test file. I saw the following (using IP and ND abbreviations for InnerPointer and NewDelete respectively):

Before the patch,

  • If only IP was turned on, I got ND's warnings with IP's name everywhere in the exploded graph.
  • If only ND was turned on, I got the warnings under ND's tag.
  • If both IP and ND were turned on, I got the warnings, and saw ND's name beside any allocation state change in the graph, but the bugs seemed to be tagged with IP's name in the end.

After this patch,

  • If only IP is turned on, I get no warning but I see IP's tag in the graph on the last PurgeDeadSymbols node (I guess it's normal?).
  • If only ND is turned on, I get the warnings with ND's tag everywhere.
  • If both IP and ND are turned on, I get the warnings, and I see no trace of IP, only ND tags everywhere.

It was really messed up before, but I think I see an improvement there.

NoQ added a comment.Aug 3 2018, 7:16 PM

Yay thx! Yeah, that's the best behavior i can imagine here.

rnkovacs updated this revision to Diff 159244.Aug 5 2018, 11:12 PM
rnkovacs marked an inline comment as done.

Replace empty Optionals with Nones.

rnkovacs closed this revision.Aug 6 2018, 3:19 PM

Committed in r339067, I just messed up the revision-closing line in the commit message.

NoQ added a comment.Aug 6 2018, 3:20 PM

Welcome to the club!

In D50211#1190146, @NoQ wrote:

Welcome to the club!

:D
Thanks, makes me feel better.

Bad news, this approach doesn't work too well, and here's why: https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/CheckerRegistry.cpp#L115

void CheckerRegistry::initializeManager(CheckerManager &checkerMgr,
                                  SmallVectorImpl<CheckerOptInfo> &opts) const {
  // Sort checkers for efficient collection.
  llvm::sort(Checkers, checkerNameLT);

  // Collect checkers enabled by the options.
  CheckerInfoSet enabledCheckers;
  for (auto &i : opts)
    collectCheckers(Checkers, Packages, i, enabledCheckers);

  // Initialize the CheckerManager with all enabled checkers.
  for (const auto *i :enabledCheckers) {
    checkerMgr.setCurrentCheckName(CheckName(i->FullName));
    i->Initialize(checkerMgr);
  }
}

Note that Initialize is a function pointer that points to register##CHECKERNAME, so a single registry function should only register one checker, because setCurrentCheckName is only called once, resulting in MallocChecker's checker object receiving the cpluscplus.InnetPointer name. It's very not-obvious how to fix this gracefully :/ You could call CheckerManager::setCurrentCheckName within the registry function, but you have to supply a full name, which adds unnecessary maintenance cost (for example, if someone moves a checker out of alpha, or puts one back).

I found this bug while trying to fix checker options, and noticed that the Optimistic flag of MallocChecker is acquired as cplusplus.InnerPointer:Optimistic, instead of unix.Malloc:Optimistic. Let's keep this in for now, and I'll try to look for ways to express dependencies while avoiding this issue.

I personally find the registration process very error-prone, the current infrastructure around it makes it super easy to mess up in a way that won't be detected for months, so we probably need a better approach altogether. It shouldn't be possible to do any harm within a registry function, especially not in a way that affects other checkers.