- User Since
- Sep 3 2015, 9:16 AM (228 w, 3 d)
Sat, Jan 18
This looks fantastic. Let's enable by it default!
@ddcc, sure, np!
Fri, Jan 17
Wed, Jan 15
I'm excited about this effort!~
Mon, Jan 13
Uh-oh, what happened here?
Fri, Jan 10
Thu, Jan 9
Fri, Jan 3
Would changing the literal in the attribute have the same effect? I.e., acquire_handle("Fuchsia_But_Please_Ignore_Me").
Tue, Dec 31
Mon, Dec 23
Sun, Dec 22
Wait, i didn't real @Szelethus's reply :)
Sat, Dec 21
I am not sure what would be the best way to test this change here.
Dec 20 2019
LGTM! Yeah, please update the ASCII-art, it's great and every checker should have it.
Yay, thanks! It does seem to work on python2 after the fix.
Dec 19 2019
Looks straightforward and useful, i don't see any problems, LGTM!
Dec 18 2019
I wonder if this checker will find any misuses of placement into llvm::TrailingObjects.
Dec 17 2019
Even though this is probably not the right solution for the static analyzer use-case (because we may end up having duplicate expressions in the CFG), it might actually make the static analyzer perform better than before (because it's still better than not having these expressions at all in the CFG). I guess we could experiment.
Dec 14 2019
Dec 13 2019
Thanks! This looks like a simple and efficient check. I have one overall suggestion.
Dec 11 2019
Looks great but i keep worrying that you're re-inventing CallDescription which already supports this feature (and many more) and we really need a single implementation of this logic because it has been historically very annoying and bugprone. Like, if you can convert your configs to a CallDescriptionMap<> while loading that'd be awesome.
I doubt we'll ever need these callbacks. I can't imagine any kind of analysis that would need them. Like, these statements have absolutely no effect on the runtime program state.
Hmm, it was supposed to get a green tick mark. Let me re-accept.
One way to criticize the current solution would be to put an InitListExpr in a loop and immediately discard it and see how states after different numbers of iterations no longer join together only because they have different NoCachingOutForConsts values.
So the AST is like
| |-DeclStmt 0x7f9f3b8932e0 <line:6:3, col:43> | | `-VarDecl 0x7f9f3b892f50 <col:3, col:42> col:7 used a 'int ' cinit | | `-InitListExpr 0x7f9f3b893268 <col:14, col:42> 'int ' | | |-array_filler: ImplicitValueInitExpr 0x7f9f3b8932d0 <<invalid sloc>> 'int' | | |-IntegerLiteral 0x7f9f3b893118 <col:41> 'int' 4 | | |-ImplicitValueInitExpr 0x7f9f3b8932d0 <<invalid sloc>> 'int' | | |-IntegerLiteral 0x7f9f3b893078 <col:31> 'int' 15 | | |-ImplicitValueInitExpr 0x7f9f3b8932d0 <<invalid sloc>> 'int' | | `-IntegerLiteral 0x7f9f3b892fd8 <col:21> 'int' 29
Dec 10 2019
Instead, i conservatively assume that they don't overlap, unless they're already known to certainly overlap on the current execution path. This loses a bit of coverage but the lost path is where they do actually overlap. This path is in my opinion not only rare but also fairly useless, as it immediately introduces an aliasing problem that we aren't quite ready to deal with.
Fair point. Yeah, i wonder what the real difference is :/ Added tests.
Thanks!! Nothing controversial remains here, right? :)
What about __attribute__((acquire_handle("fuchsia")))?
I plan some refactoring but this first patch is meant to be a single separation of code.
That's a fair point, these days we always have to account for unknown values.
In any case, every checker is allowed to make their own decisions about escaping. Escape on its own is not material, it's all about how the checker reacts to escapes. Say, it's up to MallocChecker to decide whether the function may or may not release memory that escapes on call.
Dec 9 2019
Looks great, thanks!~
Usually this kind of code is hard to re-use because all use cases require different definitions of "has descendant". We already have at least 3 such definitions floating around and we can't re-use them.
It still mildly worries me that the attributes aren't truly reusable, as the exact meaning of the attribute depends on the domain. In particular, every domain is expected to have different approaches to error handling, i.e. a function that creates or destroys a handle may fail to, respectively, create or destroy the handle, but instead indicate the failure in a domain-specific manner, eg. through magical return values or null handle or errno or whatever.
Looks great, thanks! I'd love to see what effect it immediately causes and whether there will be any true or false negatives because of this change.
Ok, so, what should the checker warn on? What should be the intended state machine for this checker?
I think you don't need to smuggle WasConservative all the way up. It's implied that if the evaluation was not conservative, then the respective ExplodedNodeSet is going to be empty, as all nodes will be put directly into the worklist instead. Eg., checkPostCall isn't going to be invoked immediately after inlineCall, but only after enqueueEndOfFunction.
Dec 6 2019
Hiding the trait in ExprEngine has the additional benefit of disallowing *direct* access to the trait - it can only be accessed through getters. It also doesn't prevent us from exposing the trait to checkers as a method on ProgramState in the future, because ProgramStateManager has access to SubEngine.
I've been hiding these internal traits in ExprEngine lately but i don't have a strong preference. I don't see any immediate need to expose this info to the checkers, nor do i see a need to actively hide it.
Dec 5 2019
Thanks!! This definitely doesn't sort out all the problems of this kind, but that's a strict improvement.
Dec 4 2019
A while back I had a look into implementing some of the CERT checkers and my vague recollection from these attempts was that it's a bad idea to take CERT examples literally. Most of the CERT rules are explained in an informal natural language and it makes CERT great for demonstrating common mistakes that programmers make when they write in C/C++. Humans can easily understand the problem by reading CERT pages. But it doesn't necessarily mean that static analysis tools can be built by generalizing over the examples from the CERT wiki.
Dec 3 2019
Hey, thanks for waiting on me! I'm slow these days, just 50 more mails to go >.<
Mmm, right, i guess you should also skip adding the tag if the notes array is empty.
Yay, it actually works!
Dec 2 2019
Of course i mean something like this:
std::string text = note(); if (!text.empty()) return text;
(and return something like "" on the other return path in the inner lambdas).
I feel like the 2. is a better solution. Of course, that change might have a performance impact as well.
Sooooo... note tags?
Nov 22 2019
Ah, the good old "stringly typed" APIs.
Nov 21 2019
MSan doesn't catch it >.<
I'll commit in order to get rid of the crash, but i'm open for discussions :)
Nov 20 2019
I think it would really help if you draw a state machine for the checker, like the ASCII-art thing in D70470; you don't need to spend a lot of time turning it into ASCII-art, a photo of a quick hand-drawn picture would be totally fine, because it's, first and foremost, for discussion :)