Page MenuHomePhabricator

[Analyzer][WebKit] UncountedCallArgsChecker
ClosedPublic

Authored by jkorous on Mar 31 2020, 2:44 PM.

Diff Detail

Event Timeline

jkorous created this revision.Mar 31 2020, 2:44 PM
jkorous updated this revision to Diff 265440.May 21 2020, 12:07 AM

reflected feedback from reviews of other related checkers

NoQ added a comment.Jun 1 2020, 5:57 AM

*requires a bit more time to digest this whole tryToFindPtrOrigin thing*
(thx for more comments!)

clang/docs/analyzer/checkers.rst
1434

I think it's worth it to add a few examples on which the checker *will* warn.

1464

You probably meant baz(this).

jkorous marked 3 inline comments as done.Jun 3 2020, 1:32 PM
jkorous added inline comments.
clang/docs/analyzer/checkers.rst
1434

Actually, this is spot on! It feels like I am struggling to step back and not think about this from the implementation perspective...

jkorous updated this revision to Diff 268291.Jun 3 2020, 1:33 PM
jkorous marked an inline comment as done.
  • rebased
  • improved the docs
NoQ added a comment.Jun 8 2020, 11:07 AM

Aha, ok, anyway, i think understand the rough idea. You're trying to make sure that the object is "pinned" onto a ref-counted smart pointer every time it's used. So you're banning use of raw pointers for any potentially prolonged duration of time.

Even if it's just within an expression, this way you make sure that it lasts for the entire duration of the full-expression. This makes your approach a bit stricter than ownership rules a-la Objective-C but i guess not intolerably so, so i don't mind.

I think this is a solid start. In most cases there shouldn't be a need to use a raw pointer, and it's also very easy to temporarily convert a raw pointer to a smart pointer in order to suppress the warning (yay C++ destructors) so it's likely that most of the warnings can be addressed or silenced with relative ease. The only reason not to have smart pointers is old APIs that you can't afford to break, but then NoUncountedMemberChecker makes sure that these raw pointers aren't stored anywhere for too long.

Please accept a bunch of minor nits.

clang/docs/analyzer/checkers.rst
1432

The first sentence seems to be lacking a predicate (probably better: "Here are some examples...").

1439

I definitely wouldn't mind banning direct delete invocations entirely. They're terrifying because they invalidate all references that were held by everyone else. This might as well be worth runtime protection, i.e. in the destructor of every reference-countable object there probably should be an assertion that there are no remaining references. I'd rather make sure that any normal code, in the worst case, calls deref() manually, but definitely not delete.

1505

Did you intend to capitalize those differently?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
1628–1640

I'd like to remove the redundant WebKit prefix from these names as well. There's package name for this :)

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
31

I'm still worried about unwrapping implicit-lvalue-to-rvalue-casts. Do we have a test for it? Like, even if you're not sure about what's the right ownership model would be in this case, i'd rather add a test and have a quick look to make sure it isn't horribly wrong.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
31

I still advocate for an even bolder syntax:

BugType Bug{this,
            "Uncounted call argument for a raw pointer/reference parameter",
            "WebKit coding guidelines"};

... and drop the constructor.

Also we usually tend to leave out private: when it's at the beginning of the class (where it's assumed), not sure why (so i don't really care).

82–84

I don't think this is actually the case. You'll get some CXXDefaultArgExpr as the argument but it'll still be something.

90

Not sure if i already asked - why not let isUncountedPtr() accept a QualType and drop the getTypePtrOrNull()? Like, QualType::operator->() still works so you don't need to change implementation, it's just less code this way.

aaron.ballman added inline comments.Jun 9 2020, 4:40 AM
clang/docs/analyzer/checkers.rst
1466

Can you pick a different term that "whitelist" to use here and elsewhere? How about "Allowed Arguments", "Allowed Values", or something along those lines?

jkorous marked an inline comment as done.Jun 9 2020, 4:24 PM

I'll add a bit more context still. The WebKit folks are ultimately aiming for increased memory safety while (and this is just as important) doing it in as non-intrusive way as possible and with regards to practical considerations - bandwidth available for eventual code changes mostly. We were brainstorming ideas and iterating prototypes against the WebKit codebase.
What's important is that we aren't aiming for "perfect" (or 100% correct) but "improvement" (in a statistical sense) here. I'd say we've gone a long way since the start but I won't mind making this checker alpha for now.

In D77179#2080379, @NoQ wrote:

Aha, ok, anyway, i think understand the rough idea. You're trying to make sure that the object is "pinned" onto a ref-counted smart pointer every time it's used. So you're banning use of raw pointers for any potentially prolonged duration of time.

Yes, that's the underlying idea. For both conceptual and implementation simplicity we decided to formulate it as multiple simpler rules/checkers that refer to only small isolated part of code - we hope that by applying a set of "local" rules (think a single function scope or single class scope) we'd reduce propensity to memory related bugs (even those whose cause can't be pinpointed to a single location but orchestrated actions of different parts of the program).

The idea behind this checker is that if we're able to make sure an argument is safe at a call-site then no matter what happens in callee's code (short of delete-ing it which we'll address in another checker) it'll remain safe for the duration of the call. Doing that we're able to prevent bugs caused by say another function called from callee that would delete a parameter:

void maybe_delete(Foo*);
void callee(Foo* f) {
  assert(f);
  maybe_delete(f);
  if (f) {
    do_more_stuff(f); // potentially dangerous
  }
}

Given the complexity of and nature of web-browser engine's work things and potential security consequences it's apparently worth to avoid situations that might be fine in other codebases.

Of course, given we're dealing with C++ if someone goes out of their way they can still shoot themselves in the foot. The goal here is to reduce propensity to honest mistakes. Preventing people determined to do scary things is both a lost battle and too restrictive for the kind of project WebKit is. There are probably better means to reach such goals anyway (code review, sanitizers, etc).

Even if it's just within an expression, this way you make sure that it lasts for the entire duration of the full-expression. This makes your approach a bit stricter than ownership rules a-la Objective-C but i guess not intolerably so, so i don't mind.

The exact measure of intolerability is actually still yet to be assessed. I've done some measurements on WebKit codebase and we discussed the results with WebKit folks - it's possible we'd have to tweak the checker before we're happy to deploy it.

I think this is a solid start. In most cases there shouldn't be a need to use a raw pointer, and it's also very easy to temporarily convert a raw pointer to a smart pointer in order to suppress the warning (yay C++ destructors) so it's likely that most of the warnings can be addressed or silenced with relative ease. The only reason not to have smart pointers is old APIs that you can't afford to break, but then NoUncountedMemberChecker makes sure that these raw pointers aren't stored anywhere for too long.

That and apparently also churn caused by reference counting in some cases.

Please accept a bunch of minor nits.

Going through them now!

jkorous marked 11 inline comments as done.Jun 9 2020, 5:44 PM
jkorous added inline comments.
clang/docs/analyzer/checkers.rst
1439

Ah! This is actually wrong!
Delete is forbidden, I should've said "calling deref()" on the raw pointer (intrusive ref-counting).

1466

Of course!

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
31

My thinking was:

  • By being overly permissive the worst that can happen is a false negative.
  • The prototype of this checker was finding more true positives (as defined by the rule we are considering) than we could handle anyway.
  • We would initially be fine doing some very bad things from the correctness perspective as long as they don't add any kind of positives and silence significant number of false negatives (in absolute numbers).
  • This checker would be iterated on.

Would you be fine with me just talking my way out of writing proper tests now (to save the effort in case we change the checker later) if we put this checker to alpha?

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
82–84

Oh, ok. I'll look into it. All I remember is that args <-> params correspondence was more complicated than I expected.

90

My concern is that since isUncountedPtr is a generic predicate used in different contexts we'd need to introduce a way how to return "unknown". We couldn't just return false as in some contexts that wouldn't work. Having to deal with Optional<bool> values (or similar) is IMO a bigger hassle than this.

jkorous updated this revision to Diff 269714.Jun 9 2020, 5:49 PM
jkorous marked 2 inline comments as done.

addressed some of the comments

NoQ added inline comments.Jun 10 2020, 1:27 AM
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
31

Ok but let's at least put a FiXME there so that we didn't forget to eventually ask ourselves this question. Because it's a completely counterintuitive part of this code's behavior that is also very hard to notice.

Note: I'm mostly fighting for this out of my PTSD from multi-week investigation of D37023. This was... hard to forget.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
82–84

I also vaguely remember a hazard on member operators in which parameters in decls and arguments in exprs are indexed differently (offset by one).

90

*visible confusion*
I mean, i was questioning the parameter of isUncountedPtr(), not its return value(?)

jkorous marked 4 inline comments as done.Jun 10 2020, 6:29 PM
jkorous added inline comments.
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
31

Oh! Good to know about this!

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
82–84

I see. I've ran into member call operators but didn't realize it's a general thing. Are you saying I should simplify ParamIdx initialization on L70 to this?

unsigned ParamIdx = dyn_cast<CXXOperatorCallExpr>(CE);
82–84

You were right about default args. For some reason they don't have source location so I guess finding the relevant parameter, getDefaultArg() and using its source location is what I should do.
https://clang.llvm.org/doxygen/classclang_1_1CXXDefaultArgExpr.html#a1539fe8f7e94440aed15edab794f0ab2

90

Sorry - I wasn't clear. What I am saying is that IIUC we need to use getTypePtrOrNull at some point anyway since:

https://clang.llvm.org/doxygen/Type_8h_source.html#l00712

const Type *operator->() const {
  return getTypePtr();
}

and from the doc comments it seems we can't just use this:

/// Retrieves a pointer to the underlying (unqualified) type.
///
/// This function requires that the type not be NULL. If the type might be
/// NULL, use the (slightly less efficient) \c getTypePtrOrNull().
const Type *getTypePtr() const;
 
const Type *getTypePtrOrNull() const;

... I am making an assumption that "type not be NULL" won't always be true because templates etc. Please tell me if that's wrong!

So, assuming we need to handle the possibility of type == NULL then if we move this into code isUncountedPtr() implementation I think we'd have to change the return value to a tri-state type (or similar alternative) which I consider a PITA to handle at all the call-sites.

jkorous updated this revision to Diff 270020.Jun 10 2020, 6:31 PM

addressed the remaining comments except the C++ member call operators ParamIdx discussion

NoQ added inline comments.Jun 10 2020, 9:17 PM
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
82–84

unsigned ParamIdx = dyn_cast<CXXOperatorCallExpr>(CE);

I think it's only broken for member operators that are also non-static. CXXOperatorCallExpr is produced for all calls with operator syntax which includes both global overloads and methods, so in order to treat them uniformly it has to treat this in methods as a separate argument rather than something special.

(side note: can we call this variable ArgIdx instead of ParamIdx in order to minimise confusion in this very messy situation?)

90

There's also QualType.isNull() for that:

QualType ArgType = (*P)->getType();
if (ArgType.isNull())
  continue; // FIXME? Should we bail?

isUncountedPtr(ArgType);

I think it's almost never beneficial to call getTypePtr()/getTypePtrOrNull() in terms of conciseness and readability; it's mostly for low-level manipulations like hashing :/ Even for the purposes of dyn_casting the pointer it's easier to simply call QualType->getAs<>().

clang/test/Analysis/Checkers/WebKit/call-args.cpp
345

Phabricator doesn't seem to like this :(

jkorous marked 3 inline comments as done.Jun 11 2020, 3:56 PM
jkorous added inline comments.
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
82–84

Ahh, right. I oversimplified it. Anyway, let me just add a bunch of test cases and see how it behaves. And thanks for the ArgIdx!

90

I see what you mean.

Actually my preferred interface would be isUncountedPtr(const Type&). That way the interface clearly states what are the valid inputs and type system takes care of enforcing it. Not sure why I chose Type* T and assert(T) - might be because "that's what we do in clang most of the time"?

Would this be ok with you?

QualType ArgType = (*P)->getType();
if (ArgType.isNull())
  continue; // FIXME? Should we bail?

isUncountedPtr(ArgType->getAs<Type>());
jkorous updated this revision to Diff 270252.Jun 11 2020, 3:59 PM
  • addressed most of the feedback
  • added couple tests
jkorous marked 7 inline comments as done.Jun 11 2020, 4:00 PM
NoQ accepted this revision.Jun 12 2020, 12:36 AM

Great, thank you! I think this is good to land and i'm looking forward for more updates.

clang/docs/analyzer/checkers.rst
2407

I guess let's fix this checker name as well eventually?

This revision is now accepted and ready to land.Jun 12 2020, 12:36 AM
jkorous marked an inline comment as done.Jun 15 2020, 1:21 PM
jkorous added inline comments.
clang/docs/analyzer/checkers.rst
2407

Definitely, I will just will land it as a separate patch

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 2:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript