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: Previously, we wouldn't report races where the write was under synchronization.
Reviewed By: peterogithub
Differential Revision: D4658850
fbshipit-source-id: e9f4c41
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: 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: 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: 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: 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 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:
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:
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:
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: 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:
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: 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: Don't warn on NotThreadSafe class, particularly when super is ThreadSafe
Reviewed By: sblackshear
Differential Revision: D4334417
fbshipit-source-id: 0df3b9d
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:
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: `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: 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:
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: 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:
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:
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: 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
Sam Blackshear
Peter O'Hearn
Andrzej Kotulski
Jules Villard