Summary:
We currently can only model the return values of functions as sources.
In order to model inputs of endpoints as sources, we need the capability to treat the formals of certain functions as sources too.
This diff adds that capability by adding a function for getting the tainted sources to the source module, then using that info in the analysis.
Reviewed By: jeremydubreil
Differential Revision: D4314738
fbshipit-source-id: dd7d423
Summary: Access to std::vector shouldn't be treated as SKIP. Implementation is simple enough to use one from std:: headers
Reviewed By: jvillard
Differential Revision: D4339577
fbshipit-source-id: d1fbbee
Summary:
Previously, summaries worked by flattening the access tree representing the post of the procedure into (in essence) a list of functions from caller input traces to callee output traces.
This is inefficient in many ways, and is also much more complex than just using the original access tree as the summary.
One big inefficiency of the old way is this: calling `Trace.append` is slow, and we want to do it as few times as possible.
Under the old summary system, we would do it at most once for each "function" in the summary list.
Now, we'll do it at most once for each node in the access tree summary.
This will be a smaller number of calls, since each node can summarize many input/output relationships.
Reviewed By: jeremydubreil
Differential Revision: D4271579
fbshipit-source-id: 34e407a
Summary: Don't warn on NotThreadSafe class, particularly when super is ThreadSafe
Reviewed By: sblackshear
Differential Revision: D4334417
fbshipit-source-id: 0df3b9d
Summary:
SuppressWarnings annotations are hardly used and add considerable
complexity due to requiring recompilation with an annotation processor.
Reviewed By: jvillard
Differential Revision: D4312193
fbshipit-source-id: c4fc07e
Summary:
Change the domain of SIOF to be based on sets of pvar * location instead of
single pvars. This allows us to group several accesses together. However, we
still get different trace elems for different instructions in a proc. We do two
things to get around this limitation and get a trace where all accesses within
the same proc are grouped together, instead of one trace for each access:
1. A post-processing phase at the end of the analysis of one proc collects all
the globals directly accessed in the proc into a single trace elem.
2. When creating the error trace, unpack this set into several trace elements
to see each access (at its correct location) separately in the trace.
This is a bit hacky and another way would be to extend the API of traces to
handle in-procedure accesses natively instead of shoe-horning them. However
since SIOF is the only one to use this, it introduces less boilerplate to do it
that way for now.
Also, a few .mlis for good measure.
Reviewed By: sblackshear
Differential Revision: D4299070
fbshipit-source-id: 3bbb5c2
Summary: These direct tests were still mostly relying on PHONY targets.
Reviewed By: jberdine
Differential Revision: D4326469
fbshipit-source-id: 37b2d0a
Summary:
If these collections don't encapsulate their state properly, there are bigger problems than thread safety issues :).
Plus, these warnings are less-than-actionable for non-Guava maintainers.
Reviewed By: peterogithub
Differential Revision: D4324277
fbshipit-source-id: cacfbf0
Summary:
Maintain an "ownership" set of access paths that hold locally allocated memory that has not escaped.
This memory is owned by the current procedure, so modifying it outside of synchronization is safe.
If an owned access path does escape to another procedure, we remove it from the ownership set.
Reviewed By: peterogithub
Differential Revision: D4320034
fbshipit-source-id: 64f9169
Summary:
In particular, the method for retrieving an element from the array, this should never be nil. Also added a model for count
similar to the one for NSString length.
Reviewed By: ddino
Differential Revision: D4306655
fbshipit-source-id: 0ecb25a
Summary: Our C++ model magic didn't work when instantiating smart pointers with volatile types. Fix it
Reviewed By: jvillard
Differential Revision: D4313271
fbshipit-source-id: 55ffb98
Summary: Rename the intermediate .exp.test files to .exp.test.noreplace so that they don't match the regexp used by `make test-replace`. Otherwise they can accidentally become .exp files that will show up in `git status`.
Reviewed By: cristianoc
Differential Revision: D4319436
fbshipit-source-id: df2ef21
Summary:
Before the diff, the code was considering as Nullable any annotation ending with `...Nullable`, including `SuppressParameterNotNullable`.
Closes#533
Reviewed By: jberdine
Differential Revision: D4317356
fbshipit-source-id: 6091c0f
Summary: Adding Buck `DEFS` macros for generating Infer genrules. The generated genrules can be used to run the analysis on any existing `java_library` targets.
Reviewed By: sblackshear
Differential Revision: D4291234
fbshipit-source-id: 6430e2e
Summary:
The Java frontend translates exceptions by assigning them to the return value.
This leads to weird behavior when the return type of the function is void.
Already handled one case of this in Quandary (ignoring assignments of exceptions to return value), but was missing the case where null is assigned to the return value.
The frontend does this to "clear" the value of previously assigned exceptions.
Reviewed By: jeremydubreil
Differential Revision: D4294060
fbshipit-source-id: 6bef5ef
Summary: Globals that are constexpr-initializable do not participate in SIOF.
Reviewed By: sblackshear
Differential Revision: D4277216
fbshipit-source-id: fd601c8
Summary:
Although the Builder pattern is not actually thread-safe, Builder's are not expected to be shared between threads.
Handle this by ignoring all unprotected accesses in classes the end with "Builder".
We might be able to soften this heuristic in the future by ensuring rather than assuming that Builder are not shared between methods (or, ideally, between threads).
Reviewed By: peterogithub
Differential Revision: D4280761
fbshipit-source-id: a4e6738
Summary:
When calling function g_realloc(gpointer mem,gsize n_bytes) one of the spec considers the case
whereby n_bytes is zero. In that case g_realloc would return null.
If we call with sizeof(int), infer would compare sizeof(int) with zero. But the prover would fail to
understand that sizeof(int) != 0.
This diff fix this. We try to convert expression to constant when they can be converted (eg in case of sizeof).
The method currently make a partial set of conversion. This could be extended.
Reviewed By: jberdine
Differential Revision: D4166944
fbshipit-source-id: 3ec4fd7
Summary:
Remember which globals are static locals.
It's useful to distinguish those from global variables in objc and in the SIOF
checker. Previously in ObjC we would accomplish that by looking at the name of
the variable, but that wouldn't work reliably in C++. Keep the old method around for
now as the way we deal with static locals in ObjC needs some fixing.
Reviewed By: akotulski
Differential Revision: D4198993
fbshipit-source-id: 357dd11
Summary: This should no work even when Infer is not setup in the PATH
Reviewed By: jvillard
Differential Revision: D4262356
fbshipit-source-id: e3fa779
Summary: `ReentrantReadWriteLock.ReadLock` and `ReentrantReadWriteLock.WriteLock` are commonly used lock types that were not previously modeled.
Reviewed By: peterogithub
Differential Revision: D4262032
fbshipit-source-id: 4ff81a7
Summary:
`o.<init>` cannot be called in parallel with other methods of `o` from outside, so it's less likely to have thread safety violations in `o.<init>`.
This diff suppresses reporting of thread safety violations for fields touched (transitively) by a constructor.
We can do better than this in the future (t14842325).
Reviewed By: peterogithub
Differential Revision: D4259719
fbshipit-source-id: 20db71f
Summary: Originially, there was a missing package declaration meaning that the generated class was ending in a different place. I also added a test for equality of Integer to complement the test of no equality, which could be always true.
Reviewed By: sblackshear
Differential Revision: D4263676
fbshipit-source-id: 86ab0d3
Summary:
We only ought to report a source-sink flow at the call site where the sink is introduced.
Otherwise, we will report silly false positives.
Reviewed By: jeremydubreil
Differential Revision: D4234766
fbshipit-source-id: 118051f
Summary: This should make it easier to understand complex error reports.
Reviewed By: peterogithub
Differential Revision: D4254341
fbshipit-source-id: fb32d73
Summary: We'll eventually want fancy interprocedural traces. This diff adds the required boilerplate for this and adds the line number of each access to the error message. Real traces will come in a follow-up
Reviewed By: peterogithub
Differential Revision: D4251985
fbshipit-source-id: c9d9823
Summary: Adding this so we can test interprocedural trace-based reporting in a subsequent diff.
Reviewed By: peterogithub
Differential Revision: D4243046
fbshipit-source-id: 7d07f20
Summary: We're at risk for some silly false positives without these models.
Reviewed By: peterogithub
Differential Revision: D4244795
fbshipit-source-id: b0367e6
Summary:
Currently cfg nodes are written into dot files in whatever order they
appear in a hash table. This seems unnecessarily sensitive, so this
diff sorts the nodes.
Reviewed By: dulmarod
Differential Revision: D4232377
fbshipit-source-id: a907cc6
Summary: Add some basic command line API to run Infer using Buck genrules. Remains to fix issues with absolute vs relative paths and to see how to create these genrules on the fly for a given java or android library.
Reviewed By: sblackshear
Differential Revision: D4245622
fbshipit-source-id: 1cda4ee
Summary:
Dealing with symbolic links in project root is tricky. To avoid it, always normalize all paths to sources with `realpath`.
Changes to tests are expected - infer started to resolve symbolic links which screws up with our testing mechanism.
Reviewed By: jberdine
Differential Revision: D4237587
fbshipit-source-id: fe1cb01
Summary:
Before, we were using a set domain of strings to model a boolean domain.
An explicit boolean domain makes it a bit clear what's going on.
There are two things to note here:
(1) This actually changed the semantics from the old set domain. The set domain wouldn't warn if the lock is held on only one side of a branch, which isn't what we want.
(2) We can't actually test this because the modeling for `Lock.lock()` etc doesn't work :(.
The reason is that the models (which do things like adding attributes for `Lock.lock`) are analyzed for Infer, but not for the checkers.
We'll have to add separate models for thread safety.
Reviewed By: peterogithub
Differential Revision: D4242487
fbshipit-source-id: 9fc599d
Summary:
In Java, we handle unknown code by propagating behavior from the parameters of the unknown function call to the return value (or constructed object, in the case of a constructor). But we do this in a somewhat silly way--generating a new summary with these semantics at each unknown call site. Instead, this diff introduces these two options as predefined behaviors and adds specialized code for them.
As a side effect of this approach, unknown functions are no longer counted as passthroughs. This is ok; the original behavior was less of a reasoned decision and more of an unintended consequence of the way we decided to handle unknown code.
This new approach ought to be more efficient than the old one, and as a virtuous side effect it will be easier to specify how to handle unknown code in other languages like C++.
Reviewed By: jeremydubreil
Differential Revision: D4205624
fbshipit-source-id: bf97445
Summary: Run all java tests with project-root at `infer/tests`. Do it to keep things consistent between clang and java tests
Reviewed By: sblackshear
Differential Revision: D4233236
fbshipit-source-id: c3f24fd
Summary:
Run all clang tests with project-root at `infer/tests`. I need it because we'll start resolving symbolic links
soon and some tests would lead outside of project root which means we'd start seeing absolute paths in recorded tests.
Diff that does same thing for java tests: D4233236
Reviewed By: jberdine
Differential Revision: D4233194
fbshipit-source-id: c261a2b
Summary:
Let's introduce some concepts. A "known unknown" function is one for which no Java code exists (e.g., `native`, `abstract`, and `interface methods`). An "unknown unknown" function is one for which Java code may or may not exist, but we don't have the code or we choose not to analyze it (e.g., non-modeled methods from the core Java or Android libraries).
Previously, Quandary handled both known unknowns and unknown unknowns by propagating taint from the parameters of the unknown function to its return value. It turns out that it is really expensive to do this for known unknown functions. D4142697 was the diff that starting handling known unknown functions in this way, and bisecting shows that it was the start of the recent performance problems for Quandary.
This diff essentially reverts D4142697 by handling known unknowns as skips instead. Pragmatically, doing the propagation trick for Java/Android library functions (e.g., `String` functions!) matters much more, so i'm not too worried about the missed behaviors from this. Ideally, we will go back to the old handling once performance has improved (have lots of ideas there). But I need this to unblock me in the meantime.
Reviewed By: jeremydubreil
Differential Revision: D4205507
fbshipit-source-id: 79cb9c8
Summary:
Developers will sometimes write GuardedBy("T.f") with the intended semantics: "guarded by the field f of the object with type T in the current state".
We want to support this to avoid false positives.
Reviewed By: peterogithub
Differential Revision: D4197476
fbshipit-source-id: acd00d9
Summary:
The way interfaces are dealt with led to a false positive,
where tryLock() works OK for a Lock but not for a ReentrantLock.
The solution is just to provide the model.
While I am at it I am adding some more standard tests for Lock and ReentrantLock, which were not present.
Reviewed By: sblackshear
Differential Revision: D4204551
fbshipit-source-id: 9b6de28
Summary: These are dangerous if you are trying to compare a type to a string, and they're also unsightly.
Reviewed By: jvillard
Differential Revision: D4189956
fbshipit-source-id: 14ce127
Summary:
SIOF is only for interactions between objects of non-POD types. Previously the
checker was also reporting for POD types.
Reviewed By: akotulski
Differential Revision: D4197620
fbshipit-source-id: 7c56571
Summary:
Record an abstraction of the bug traces in the tests. The abstraction of a
trace is the sequence of descriptions. In practice, descriptions are either
empty, or of the form "start/end/return from/call to procedure X". They seem
pretty stable.
Motivation: there is nothing testing the traces reported by Infer right now,
even though they are surfaced to developers. For instance, Quandary uses
--issues-txt instead of --issues-tests to make sure the traces do not regress.
This change would make this approach more widespread.
Reviewed By: sblackshear
Differential Revision: D4159597
fbshipit-source-id: 9c83952
Summary:
`make` doesn't delay variable evaluation in targets' dependencies, so
`$(OBJECTS)` was always empty. Including clang.make after having defined
`OBJECTS` fixes it.
Reviewed By: jberdine
Differential Revision: D4159522
fbshipit-source-id: 6925f8a
Summary:
When loading results from a json file, sort them. This prints results in some
sane order for both --issues-test and --issues-txt, removing the need for
post-processing of the result.
Reviewed By: cristianoc
Differential Revision: D4167029
fbshipit-source-id: 37e9f1c
Summary:
- rename java.make -> javac.make, config.make -> java.make, and move to infer/tests/ so it's easier to use from infer/tests/build_systems/
- use these from ant's test Makefile, much code reuse!
- factor out common functionality between java and clang
A wrinkle: sorting is now done the same way for --issues-tests and
--issues-txt, which produces bogus (but still as deterministic) sorting for
--issues-txt. This is more of a cosmetic issue, but I hope to fix it in a later
diff that gets rid of calls to `sort` in favour of sorting directly from
`InferPrint`.
Reviewed By: jberdine
Differential Revision: D4166841
fbshipit-source-id: ed6f232
Summary: The thread safety checker is run independently of other analyses, using the command "infer -a threadsafety -- <build-command>".
Reviewed By: sblackshear
Differential Revision: D4148553
fbshipit-source-id: bc7b3f9
Summary:
Our patch to Javalib has been accepted, so we can parse programs with invokedynamic!
invokedynamic still crashes Sawja, but I have worked around this by replacing all invokedynamic's with invokestatic's before passing them to Sawja.
This means we can handle everything about invokedynamic except calling the correct function (I call a dummy function with the correct signature for now).
We can try to actually call the right method in the future.
Reviewed By: jvillard
Differential Revision: D4160384
fbshipit-source-id: a8ef4e1
Summary: When searching for cast errors, types that were not Java objects, e.g. arrays of primitive types were not taken into account, leading to incorrect class cast excpetion reports.
Reviewed By: sblackshear
Differential Revision: D4166184
fbshipit-source-id: 7157c95
Summary:
This adds generic support for reporting error traces as usual infer issues
traces (instead of putting them in the textual description of the error) to
Trace.ml and SinkTrace.ml.
The siof checker is made to use these new traces, and gets an improved error
message mentioning the name of the problematic global as well, which requires a
slight API change in Pvar.re.
The support in Trace.ml is incomplete: passthroughs are ignored. This missing
feature will be needed by Quandary to migrate its error messages.
Reviewed By: sblackshear
Differential Revision: D4159542
fbshipit-source-id: 8c1101d
Summary:
- set SHELL to bash explicitly in Makefiles (Debian uses dash)
- avoid using system headers when using our own clang's headers in tests
- do not rely on the name of the object file to write the frontend debugging scripts. It turns out that `-o` is *not* always present in the arguments of `-cc1` functions so the `Option.get` could crash. Since we don't actually need to get the object file name, just a nice enough name, don't try to be smarter at guessing what object will be created and pick a different name built from the source name instead.
Reviewed By: akotulski
Differential Revision: D4159516
fbshipit-source-id: c7bc2b9
Summary: If a procedure is both a source and a sink for the same value, and it's a sink first, you will get a false positive when applying the summary for the procedure.
Reviewed By: cristianoc
Differential Revision: D4145246
fbshipit-source-id: 97f0022
Summary: `make test` was always exiting with exit code 0, even in the case of test failures. This is definitely not what we want.
Reviewed By: sblackshear
Differential Revision: D4154912
fbshipit-source-id: 87b4b2b
Summary: Mark native methods as defined so that the analysis generates a summary for those methods. When analyzing Java projects compiled with Buck, the summaries for the dependencies methods of are retrieved from the classpath. In this case, having access to the summary is useful to access the attributes of a callee when the callee is part of a, previously analyzed, Buck target.
Reviewed By: sblackshear
Differential Revision: D4141362
fbshipit-source-id: 75888c8
Summary:
Analyses should handle methods whose code is unknown and methods whose summary is a no-op differently.
Previously, this was done correctly for some kinds of methods (e.g., native methods, which were recognized as unknown), but not for others (interface and abstract methods).
This diff makes sure we correctly treat all three kinds as unknown.
Reviewed By: jeremydubreil
Differential Revision: D4142697
fbshipit-source-id: c88cff3
Summary:
Instead of the custom filtering done by `InferPrint --issues-tests`, use the
filtering done by `infer` and run without filtering for our e2e tests. We still
test the filtering for our build systems integration tests, and this diff
restores that behaviour for the ant test (hence the bugs removed from
ant/issues.exp).
Also add internal exceptions to most tests to get more signal out of them (eg,
knowing when we add assertion failures and the like).
Retire the old `--issues-tests` to limit the number of ways we do filtering.
Reviewed By: jeremydubreil
Differential Revision: D4131308
fbshipit-source-id: 35805cc
Summary:
This makes the tests depend on much fewer phony targets, thus reducing the need
to rerun the tests when nothing has changed.
Reviewed By: jberdine
Differential Revision: D4118457
fbshipit-source-id: 664b6e3
Summary:
Our default strategy for handling unknown code is to propagate taint from the actuals to the return value.
But for commonly-used methods like `StringBuilder.append` (used every time you do `+` with a string in Java), this doesn't work.
The taint should be propagated to both the receiver and the return value in these cases.
I'm considering a solution where we always propagate taint to the receiver of unknown functions in the future, but I am concerned about the performance.
So let's stick with a few special string cases for now.
Reviewed By: cristianoc
Differential Revision: D4124355
fbshipit-source-id: 5b2a232
Summary: A must-have for reporting taint errors and any other interprocedural error where the trace is sufficiently complex.
Reviewed By: jvillard
Differential Revision: D4124072
fbshipit-source-id: 26b3b2b
Summary: A must-have for reporting taint errors and any other interprocedural error where the trace is sufficiently complex.
Reviewed By: jvillard
Differential Revision: D4106352
fbshipit-source-id: b2677e6
Summary:
New version of clang plugin exports `-x` arg information as a part of
TranslationUnitDecl. Get it from there instead of reading it from
clang argv
Reviewed By: jvillard
Differential Revision: D4112652
fbshipit-source-id: 5c3af1f
Summary: We want to skip readwrite locks for now, maybe report on their misuses later.
Reviewed By: sblackshear
Differential Revision: D4110998
fbshipit-source-id: 986f77e
Summary:
Previously, we recorded direct sinks as sinks and transitive sinks as passthroughs. This makes it difficult to create an expanded interprocedural trace when recording an error because we can't distinguish between sinks (which we want to expand) and passthroughs (which we don't). This diff changes recording of sinks so that a sink is now the *last* function in a trace to call a sink. To find out what the original sink was, the summary for the transitive sink in the trace will now need to be (recursively) expanded until we bottom out in the original sink.
Will do the same for sources in a follow-up diff.
Reviewed By: cristianoc
Differential Revision: D4103759
fbshipit-source-id: 6f435f5
Summary:
Needed to support upcoming diff(s) that change the nature of sources/sinks in a trace. Today they are the *original* source/sink, but in the future they will be the *transitive* source/sink (last procedure to return a source/call a sink).
This new convention will make the `returnAllSources`/`callAllSinks` form of these tests not so useful, since `returnAllSources`/`callAllSinks` will now show up as a single source/sink in the trace (at least without expanding the trace). By making these tests intraprocedural, we can make sure that we're still testing everything that we want to.
Reviewed By: cristianoc
Differential Revision: D4103754
fbshipit-source-id: 1733ecf
Summary:
See code comment about `throw exn` being translated as `return exn`.
This problem was revealed by D4081279, which started grabbing access paths from exceptions.
Reviewed By: jvillard
Differential Revision: D4096391
fbshipit-source-id: 9d91513
Summary: Doing `sychronized(A.class)` where `A` is an inner class was not previously recognized by the `GuardedBy` checker.
Reviewed By: peterogithub
Differential Revision: D4095094
fbshipit-source-id: c832f9e
Summary:
We issue a thread safety warning on a class not
marked ThreadSafe, when it has a super that is. This makes some sense. But,
it will be nice to remind that a super is so maeked, else the mesg could
seem out of context or surprising
Reviewed By: sblackshear
Differential Revision: D4075145
fbshipit-source-id: ebc2b83
Sam Blackshear
Josh Berdine
Andrzej Kotulski
Peter O'Hearn
Dulma Churchill
Cristiano Calcagno
Jules Villard
Jeremy Dubreil
Dino Distefano
Ryan Rhee