Summary: It seems that we were not really using the `Bottom` part of the domain as a pair of (empty call map, empty tracking var map) was already acting as bottom.
Reviewed By: sblackshear
Differential Revision: D4759757
fbshipit-source-id: 53dedfe
Summary:
Most of our tests work by comparing the output of some command to a baseline
expected output. It's often needed to update that baseline.
Previously, that was crudely done by attempting to move every foo.exp.test file
to foo.exp. This does not work terribly well, in particular because
foo.exp.test might be stale.
Instead, add a `replace` target to every test that knows how to update the
baseline. This allows custom behaviours too, eg in the differential tests.
Most of the tests include base.make or differential.make, so add a replace target
there. A few tests are completely custom, add a replace target to them too.
Reviewed By: jeremydubreil
Differential Revision: D4754279
fbshipit-source-id: ec34273
Summary:
This issue was spotted in the wild. There may be more of those, unfortunately it's hard to predict
More general problem is that types in infer frontend diverge from clang's types for DerivedToBase cast.
Then, infer uses types from clang anyway and that confuses backend. Getting it always right is very hard
Reviewed By: jvillard
Differential Revision: D4754081
fbshipit-source-id: 5fb7069
Summary:
If I read off the main thread and write on the main we
could have a race. (Writes off main are already reported.)
Reviewed By: sblackshear
Differential Revision: D4746138
fbshipit-source-id: 8b6e9c5
Summary:
All tests were redirecting `stderr` into duplicates.txt which made it much harder to see other error messages in stderr (such as uncaught exceptions).
To mitigate it, write duplicates to separate file and don't redirect `stderr` to another file.
Reviewed By: jvillard
Differential Revision: D4728938
fbshipit-source-id: 8ad2fc8
Summary:
One limitation of Eradicate is that certain nullability patterns are not expressible using simply the `Nullable` annotation.
One such pattern is using the knowledge that a function returns null when passed null, but returns an object otherwise.
The annotation `PropagatesNullable` is a variant of `Nullable` applied to parameters when their value propagates to the return value.
A method annotated
```
B m(PropagatesNullable A x) { return x == null ? x : B(x); }
```
indicates that `m` returns null if `x` is null, or an object of class `B` if the argument is not null.
Examples with multiple parameters are in the test cases.
This diff builds some infrastructure for annotation transformers: the example above represents the identity function on nullability annotations.
Reviewed By: jvillard
Differential Revision: D4705938
fbshipit-source-id: 9f6194e
Summary:
Before, `trace_of_pname` only grabbed unprotected writes from the summary, so the traces ending in an unprotected read were truncated.
We now look at reads too when appropriate.
Reviewed By: peterogithub
Differential Revision: D4719740
fbshipit-source-id: 28f6e63
Summary: Now, running `infer -a checkers -- ...` will also run the ThreadSafety checker
Reviewed By: sblackshear
Differential Revision: D4691330
fbshipit-source-id: 04fc781
Summary: Run all the checkers one after each other, which allows the Infer AI framework to run several checkers together, including the possibility for them to collaborate.
Reviewed By: sblackshear
Differential Revision: D4621838
fbshipit-source-id: e264d67
Summary:
This makes sure that one can run `./build-infer.sh` then `make`. Otherwise it's
not always clear what one should do to recompile infer, eg when `make` will
work and when `./build-infer.sh` should be used instead, in particular when the
user doesn't have opam configured for her terminal.
Reviewed By: jberdine
Differential Revision: D4698159
fbshipit-source-id: 5df8059
Summary:
We were including hex of empty string if mangled name was not empty (so for all C++ functions).
Instead, include hex of a source file only if it's not empty
Reviewed By: mbouaziz
Differential Revision: D4705388
fbshipit-source-id: 55b6587
Summary:
Procnames files are now reversed qualifier lists with `#` as separator (instead of `::` which needs to be escaped in bash).
Because of the mechanism that is used to obtain qualifiers, it also affects naming for ObjC classes.
Examples:
```
std::unique_ptr<int>::get -> get#unique_ptr<int>#std#__MANGLED,...__ // C++ method
folly::split -> split#folly#__MANGLED,..._ // function within namespace
NSNumber numberWithBool: -> numberWithBool:#NSNumber#class // ObjC method
```
Reviewed By: jvillard
Differential Revision: D4689701
fbshipit-source-id: c3acfc6
Summary:
We want to stop supporting '-multiletter' options altogether in the future as
they are not standard.
For boolean switches, we used to have '-short' for true and '-nshort' for
false, but that second form is necessarily > 1 letters, which doesn't work in
this scheme. Replace these by '-s' for true and '-S' for false.
Reviewed By: jberdine
Differential Revision: D4674079
fbshipit-source-id: 95bacfe
Summary:
When both an unprotected write and a read/write race emanate from the same line,
undoubtedly because of interprocedurality, strip the read/write report (for now).
Perhaps report the info in more succinct form later, but keep to one report/line.
Reviewed By: sblackshear
Differential Revision: D4685102
fbshipit-source-id: 291cf20
Summary:
There was a bug where we allowed ourselves to project local variables from the callee summary into an access path in the caller.
We should only be able to project callee variables that are in the footprint.
Reviewed By: jeremydubreil
Differential Revision: D4684868
fbshipit-source-id: 53a2b9d
Summary:
Updated version of the plugin exports some missing `VarDecls`. To make sure it doesn't break again,
add a test that didn't work before.
Reviewed By: dulmarod
Differential Revision: D4674123
fbshipit-source-id: 0c1677a
Summary:
Eradicate detects circular field initializations (e.g. a field initialized with itself)
by checking in the typestate at the end of the constructor whether the origin
of the field is a field name in the current class.
This has the problem that the following initialization pattern is not recognized as correct:
C(C x) { this.field = x.field }
To fix the issue, the origin information for field accesses x.f is extended
with the origin information of the inner object x.
Circularities are detected if the origin of x is "this".
Reviewed By: jberdine
Differential Revision: D4672472
fbshipit-source-id: 9277bdd
Summary: Previously, we wouldn't report races where the write was under synchronization.
Reviewed By: peterogithub
Differential Revision: D4658850
fbshipit-source-id: e9f4c41
Summary: I encountered cases where the class name part of the method name was passed as `(None, "package.Class")` instead of `("package", "Class")` and therefore incorrectly failing some inequality checks
Reviewed By: sblackshear
Differential Revision: D4662617
fbshipit-source-id: 98ee3e3
Summary:
Stop multiple reports per line happening. These come about
because of interprocedural access to multiple fields. Present one trace,
and summary information about other accesses.
Reviewed By: sblackshear
Differential Revision: D4636232
fbshipit-source-id: 9039fea
Summary:
All intermediate `.exp` files used for tests can be generated with custom info, based on what is needed for the tests purposes.
This customisation happens via command-line argument `--issues-fields`.
Reviewed By: cristianoc, jvillard
Differential Revision: D4628062
fbshipit-source-id: feaa382
Summary:
- The package declaration was wrong
- There was a leftover copy-pasted resource leak test from `CursorLeak.java`.
Reviewed By: sblackshear
Differential Revision: D4612687
fbshipit-source-id: 42c1a35
Summary: Rather than having three separate annotations related to checking/assuming thread-safety, let's just have one annotation instead.
Reviewed By: peterogithub
Differential Revision: D4605258
fbshipit-source-id: 17c935b
Summary: distinguish writes via method calls (e.g., add) from writes via assignment in the error messages
Reviewed By: sblackshear
Differential Revision: D4611748
fbshipit-source-id: 7594d3b
Summary: Report at most one read/write race or unprotected write per access path per method
Reviewed By: sblackshear, jvillard
Differential Revision: D4590815
fbshipit-source-id: 3c3a9d9
Summary:
To address a common source of false positives observed in D4494901.
We don't do anything with `release` yet, but can model it as releasing ownership in the future if we want to enforce correct usage of `SynchronizedPool`'s.
Reviewed By: peterogithub
Differential Revision: D4593635
fbshipit-source-id: 621e937
Summary: Reports on reads that have one or more conflicting writes. When you report, say which other methods race with it.
Reviewed By: sblackshear
Differential Revision: D4538793
fbshipit-source-id: 47ce700
Summary: Thread-local variables can't be shared between threads, so it's safe to mutate them outside of synchronization
Reviewed By: jeremydubreil
Differential Revision: D4568316
fbshipit-source-id: 0634cad
Summary:
Enrich the domain of SIOF to contain, as well as the globals needed by a
procedure, the globals that the procedure initializes. Also add the possibility
to model some procedures as initializing some variables. Use that mechanism to
teach the checker about `std::ios_base::Init`.
Reviewed By: mbouaziz
Differential Revision: D4588284
fbshipit-source-id: d72fc87
Summary:
This gives a way for users to flag safe methods regarding SIOF, for instance if
the problematic paths in the method cannot happen before `main()` has started.
Reviewed By: akotulski
Differential Revision: D4578700
fbshipit-source-id: 6542dcf
Summary:
I couldn't figure out why, but from within an infer release the traces we get
for this test are different than the expected ones. This is even consistent
across osx and linux.
In order to restore sanity, let's just hide this incomprehensible fact. Let's
come back to it if more tests exhibit this, maybe traces are not guaranteed to
be exactly the same across runs.
Reviewed By: sblackshear
Differential Revision: D4559405
fbshipit-source-id: dd88c59
Summary: Should stop us from reporting on benign races of fields that are caching resources.
Reviewed By: peterogithub
Differential Revision: D4538037
fbshipit-source-id: 15236b4
Summary: This annotation can then be used to suppress the warnings on non-android Java projects.
Reviewed By: sblackshear
Differential Revision: D4544858
fbshipit-source-id: 8a0b8fa
Summary:
The Java models for resources are way to complex. The main issue I am facing with these models is that small changes in the analysis can affect the generation of the models in some weird ways. For instance, I get different specs for some of the models between my devserver and my devvm, which seems to be mostly related with the backend treatment of `instanceof`.
The objective here is to simplify the models as much as possible in order to:
1) make debugging regressions easier
2) get simpler specs and less modeled methods shipped in `models.jar`
Reviewed By: sblackshear
Differential Revision: D4536115
fbshipit-source-id: 577183a
Summary: Better documentation, and could perhaps be checked instead of trusted later if the analysis understands threads better.
Reviewed By: jaegs
Differential Revision: D4537463
fbshipit-source-id: 4323c78
Summary: In some cases where a function is called directly on a formal (e.g, `def foo(o) { callSomething(o) }`, we were failing to propagate the footprint trace to the caller.
Reviewed By: jeremydubreil
Differential Revision: D4502404
fbshipit-source-id: d4d632f
Summary: This case was already working but there was no tests for it
Reviewed By: sblackshear
Differential Revision: D4529473
fbshipit-source-id: ca3ff02
Summary: This will be important for maintaining ownership of `View`'s, which involve a lot of casting.
Reviewed By: peterogithub
Differential Revision: D4520441
fbshipit-source-id: fdef226
Summary:
Previously, we would lose track of ownership in code like
```
Obj owned = new Obj();
Obj stillOwned = id(owned); // would lose ownership here
stillOwned.f = ... // would report false alarm here
```
This diff partially addresses the problem by adding a notion of "unconditional" (always owned) or "conditional" (owned if some formal at index i is owned) ownership.
Now we can handle simple examples like the one above.
I say "partially" because we still can't handle cases where there are different reasons for conditional ownership, such as
```
oneOrTwo(Obj o1, Obj o2) { if (*) return o1; else return o2; } // we won't understand that this maintains ownership if both formals are owned
Obj stillOwned = oneOrTwo(owned1, owned2);
stillOwned.f = ... // we'll report a false alarm here
```
This can be addressed in the future, but will require slightly more work
Reviewed By: peterogithub
Differential Revision: D4520069
fbshipit-source-id: 99c7418
Summary: This fixes a wrong level of indirection when performing the type substitution.
Reviewed By: sblackshear
Differential Revision: D4521008
fbshipit-source-id: 7324ea6
Summary:
This fixes false positives we had in fields written by callees of a constructor (see new E2E test).
This is also a bit cleaner than what we did before; instead of special-casing constructors, we just use the existing ownership concept.
Reviewed By: peterogithub
Differential Revision: D4505161
fbshipit-source-id: a739ebc
Summary:
Constants are always "owned" in the sense that no one can mutate them.
In code like
```
Obj getX(boolean b) {
if (b) {
return null;
}
return new Obj();
}
```
, we need to understand this in order to infer that the returned value is owned.
This should fix a few FP's that I've seen.
Reviewed By: peterogithub
Differential Revision: D4485452
fbshipit-source-id: beae15b
Summary:
Infer used to report null dereference when field was accessed later:
```
vector<int> v;
int& a = v[0]; // should be EMPTY_VECTOR_ACCESS here, but it wasn't reported
int b = a; // was NULL_DEREFERENCE here
```
To avoid this problem, model all accesses to vector as dereference of its internal `beginPtr` field.
Reviewed By: jberdine
Differential Revision: D4481942
fbshipit-source-id: 2142894
Summary: This should fix the issue with broken invariants when the method specialization on pointer ends up doing a substitution on non pointer types
Reviewed By: sblackshear
Differential Revision: D4487232
fbshipit-source-id: f3fce84
Summary: The method `junit.framework.TestCase.setUp()` is always run before the other methods by the JUnit testing framework. So the method act as a class initializer.
Reviewed By: sblackshear
Differential Revision: D4487371
fbshipit-source-id: 1998801
Summary: Just adding some more test cases on how Infer handles dynamic dispatch.
Reviewed By: jberdine
Differential Revision: D4486529
fbshipit-source-id: d90ef42
Summary:
This modifies `CTrans_utils.dereference_value_from_result` function. Reasons:
1. Type in Sil.Load tuple is supposed to be the type of Ident.t field (result), not Exp.t (source)
2. Inside `dereference_value_from_result` we used wrong type when `~strip_pointer:true` - it used original type instead of the resulting one
3. Fixing (2), uncovered similar issue in `CTrans_utils.cast_operation` - it should have used resulting type, not original one
Changed tests are expected:
1. `deprecated_hack.cpp` test was how I discovered this issue (it was very wrong)
2. the other test also had issues, now it's correct
Sadly, there was no logic I followed when writing this change - I stuffed changes in a way that fits, but it may be breaking something somewhere else.
Reviewed By: jberdine
Differential Revision: D4481895
fbshipit-source-id: b139c59
Summary:
This diff adds a set of access paths holding a value returned from a method annotated with Functional to the domain.
If a "functional" value is written to a field, we won't count that right as an unprotected access.
The idea is to be able to use the Functional annotation to get rid of benign race false positive, such as:
```
Functional T iAlwaysReturnTheSameThing();
T mCache;
T memoizedGetter() {
if (mCache == null) {
mCache = iAlwaysReturnTheSameThing();
}
return mCache;
}
```
Although there is a write-write race on `mCache`, we don't care because it will be assigned to the same value regardless of which writer wins.
Reviewed By: peterogithub
Differential Revision: D4476492
fbshipit-source-id: cfa5dfc
Summary:
We warn on unsafe accesses to fields that occur in a public method (or are reachable from a public method).
We ought not to consider VisibleForTesting methods as public, since they are only public for testing purposes.
Reviewed By: peterogithub
Differential Revision: D4477648
fbshipit-source-id: 5f58914
Summary: Simple model for List methods that write to the collection.
Reviewed By: peterogithub
Differential Revision: D4453381
fbshipit-source-id: 19edc51
Summary:
Sometimes some instructions were never part of any CFG node and were not written to CFG at all. Add a mechanism that will create node and add them to CFG when they reach compoundStmt translation.
This is step forward to make `*x;` instruction actualy dereference x (it works in C already in C++ AST looks different though)
Reviewed By: dulmarod
Differential Revision: D4469139
fbshipit-source-id: b03b11c
Summary:
Previously, we would correctly be silent on code like `x = new T(); x.f = ...`, but would wrongly warn on code like `x = makeT(); x.f = ...`.
The reason is that we only allowed ownership through direct allocation.
This diff adds a boolean that specifies whether the return value is owned as part of the summary.
This allows us to correctly handle many common cases of (transitively) returning a freshly allocated object, but still won't work for understanding that ownership is maintained in examples like
`x = new T(); y = id(x); y.f = ...`.
Reviewed By: jvillard
Differential Revision: D4456864
fbshipit-source-id: b5eec02
Summary:
Eradicate currently considers a field initialized if it's simply accessed (not written to),
or initialized with another initialized field.
This fixes the issue.
Reviewed By: jvillard
Differential Revision: D4449541
fbshipit-source-id: 06265a8
Summary:
If we have code like
```
o.setF(source())
sink(o)
```
and `setF` is an unknown method, we probably want to report.
Reviewed By: jeremydubreil, mburman
Differential Revision: D4438896
fbshipit-source-id: 5edd204
Summary:
In code like
```
foo(o) {
iWriteToF(o)
}
```
, the condtional write to `f` in `iWriteToF` should become a conditional write for `foo`.
Reviewed By: peterogithub
Differential Revision: D4429160
fbshipit-source-id: f111ac4
Summary:
In code like
```
foo() {
Object local = new Object();
iWriteToAField(local);
}
```
, we don't want to warn because the object pointed to by `local` is owned by the caller, then ownership is transferred to the callee.
This diff supports this by introducing a notion of "conditional" and "unconditional" writes.
Conditional writes are writes that are rooted in a formal of the current procedure, and they are safe only if the actual bound to that formal is owned at the call site (as in the `foo` example above).
Unconditional writes are rooted in a local, and they are only safe if a lock is held in the caller.
Reviewed By: peterogithub
Differential Revision: D4429131
fbshipit-source-id: 2c6112b
Summary:
Races on volatile fields are less concerning than races on non-volatile fields because at least the read/write won't result in garbage.
For now, let's de-prioritize these writes by ignoring them.
Reviewed By: peterogithub
Differential Revision: D4434023
fbshipit-source-id: 05043ba
Summary:
Also make sure we don't introduce deprecated options in our repo, eg when
calling infer from infer.
Reviewed By: jeremydubreil
Differential Revision: D4430379
fbshipit-source-id: 77ea7fd
Summary: Just cleanup; gives us slightly less test code to maintain.
Reviewed By: jeremydubreil
Differential Revision: D4429265
fbshipit-source-id: d43c308
Summary:
Similar to marking classes ThreadConfined, we want to support marking fields as well.
The intended semantics are: don't warn on writes to the marked field outside of syncrhonization, but continue to warn on accesses to subfields.
Reviewed By: peterogithub
Differential Revision: D4406890
fbshipit-source-id: af8a114
Summary: Generalized the CppTrace into a Clang trace because we don't currently have separate checkers for Obj-C and Cpp. Happy to separate them later if there is a good reason
Reviewed By: akotulski
Differential Revision: D4394952
fbshipit-source-id: e288761
Summary:
Adding models that allow us to warn on unguarded accesses to subclasses of `Map`, but not on accesses of threadsafe containers like `ConcurrentMap`.
Lots more containers to model later, but stopping at `Map`s for now to make sure the approach looks ok.
Reviewed By: jvillard
Differential Revision: D4385306
fbshipit-source-id: d791eee
Summary: Need to upgrade in order to specify some taint properties on a more recent `WebView` API.
Reviewed By: cristianoc
Differential Revision: D4382590
fbshipit-source-id: 0925742
Summary: These methods should only be called from other methods that also run on the UI thread, and they should not be starting new threads.
Reviewed By: peterogithub
Differential Revision: D4383133
fbshipit-source-id: 6cb2e40
Summary: Use the lazy dynamic dispatch by default in prod for the Java analysis
Reviewed By: sblackshear
Differential Revision: D4356872
fbshipit-source-id: 491e92e
Summary:
Without this it's not always obvious which test fails. It also makes it easier
to mass-patch test failures from the CI jobs to replace expected outputs with
actual outputs (eg, when debugging osx frontend tests from linux).
Reviewed By: jberdine
Differential Revision: D4352205
fbshipit-source-id: 8887d7b
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
Jeremy Dubreil
Sam Blackshear
Jules Villard
Andrzej Kotulski
Peter O'Hearn
Mehdi Bouaziz
Martino Luca
Dulma Churchill
Cristiano Calcagno
Dino Distefano
Kihong Heo
Josh Berdine
Ryan Rhee