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:
It seems that the `close()` method that should normally be called on an object `obj` of type `java.io.Closeable` is sometimes called on `obj` of type `java.lang.Object`. It did not fully understand in which case this happens but it could be coming from a bug in Sawja since the type of `obj` in the bytecode is correct, but the Sawja reciever expression given to the Java frontend has the type `java.lang.Object`.
In any case, it does not hurt to always consider that `obj.close()` will replace the `FILE` attribute on `obj` by a `MEM` attribute.
Reviewed By: sblackshear
Differential Revision: D4540627
fbshipit-source-id: 71f9c95
Summary:
This reorganises the contents of `infer --help`:
- Headings are more prominent (start with `**`)
- New "Java" section
- Delete "Analysis" section, distribute contents over other sections
- New "Quandary" section
- Under the hood, new "Buffer Overruns" and "Crashcontext" sections, but do not show them as we don't expect external use yet, although that may be a bit arbitrary
- typo: `--bufferoverrn` -> `--bufferoverrun`
- move some options from one section to another
Reviewed By: jberdine
Differential Revision: D4537500
fbshipit-source-id: a789375
Summary:
Native compilation seems to take a couple of seconds to build the unit test
binary, but we don't need it to be native (and neither do users building infer
to install it on their machines). In fact, the test mode rebuilds it in
bytecode mode. For the sake of being able to run the unit tests when developing
on infer (and thus possibly while having some fatal warnings showing up in the
code, preventing us from building the test target), also build InferUnit in
bytecode mode.
Reviewed By: sblackshear, martinoluca
Differential Revision: D4537455
fbshipit-source-id: 374c84c
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 will make it a cinch to track new "attributes" of memory locations, and to propagate more complex attributes such as conditional ownership (coming in a future diff).
Reviewed By: peterogithub
Differential Revision: D4523143
fbshipit-source-id: 57aa133
Summary: The diff remove the no-op model for `Cursor.close()` by the frontend-based `Closeable` as resources mechanism where every call of the form `object.close()` removes the file attribute on `object` when `object` is of type `Closeable`.
Reviewed By: sblackshear
Differential Revision: D4519386
fbshipit-source-id: 83633d4
Summary: This fixes a wrong level of indirection when performing the type substitution.
Reviewed By: sblackshear
Differential Revision: D4521008
fbshipit-source-id: 7324ea6
Summary: Those should be treated angelically during the analysis with the same end results
Reviewed By: sblackshear
Differential Revision: D4518930
fbshipit-source-id: ee5bae8
Summary: Being forced to separately define `pp_element`/`pp_key` is uneccessary and makes it more cumbersome to create a set/map from an existing module that already defines `pp`.
Reviewed By: jeremydubreil
Differential Revision: D4517308
fbshipit-source-id: 9b17c9c
Summary: Not clear why we need to disable this case and in which case is Infer creating too many disjunctions.
Reviewed By: sblackshear
Differential Revision: D4509394
fbshipit-source-id: fbc106d
Summary: This method can return `null` if the parameter is not a supported system service. However, since this method tends to be called with a constant value as parameter, it does seem to be returning null often in practice.
Reviewed By: sblackshear
Differential Revision: D4509185
fbshipit-source-id: 4cb80ce
Summary:
At one point I thought we'd want to have lots of different schedulers for things like exploring loops in different orders, but that hasn't materialized.
Let's make the common use-case simpler by hiding the `Scheduler` parameter inside the `AbstractInterpreter` module.
We can always expose `MakeWithScheduler` later if we want to.
Reviewed By: jberdine
Differential Revision: D4508095
fbshipit-source-id: 726e051
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:
Propagating the arguments read in .inferconfig shouldn't be necessary as they
are parsed by each executable. There are corner cases where I think this diff
could change the behaviour of infer (eg, .inferconfig redefines project-root
and subsequent exes read a different .inferconfig thanks to the project-root in
INFER_ARGS), but I don't think there are good use cases like that.
Reviewed By: jberdine
Differential Revision: D4475092
fbshipit-source-id: 5c020d0
Summary:
Clients should use `Config.parse_action` instead to figure in what mode they
are operating.
In particular, the biggest change is in logging. Take the `parse_action` into
account instead of the exe, and change the log/ subdirectories to be "capture",
"driver", "analyze", and "print", corresponding to the various phases of an
infer run.
Reviewed By: jberdine
Differential Revision: D4474943
fbshipit-source-id: 6d33ad3
Summary:
Support several parsing modes: Infer, Javac, NoParse
The "Infer" mode specifies a list of sections, ie the parts of infer that are affected by an option (corresponds to the old notion of "exes"):
analysis, clang frontend, print, ...
- .inferconfig and INFER_ARGS always parsed
- outside .inferconfig and INFER_ARGS, do not parse subcommand arguments before the subcommand has been activated
- command-line is parsed or not based on the subcommand/executable selected
- executable dictates subcommand, so almost nothing depends on the executable outside of Config. Another diff will restrict the API around exes to reflect this.
Reviewed By: jberdine
Differential Revision: D4474886
fbshipit-source-id: 442dfef
Summary:
Some classes may have deleted new operator for them. To fix it, run global `new` operator instead
```
struct X {
void* operator new(size_t) = delete;
};
X *p = new X; // compilation error
X *p = ::new X; // no compilation error
```
This change is following same strategy standard headers follow.
Reviewed By: jvillard
Differential Revision: D4500977
fbshipit-source-id: 20babfa
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:
When the receiver type and return type of an unknown call are the same, propagate taint to both the receiver and the return type.
This does the right thing for common "builder-style" methods that both update and return the receiver.
We already had custom models for a few such methods (e.g., `StringBuilder.append`), but we can remove them now.
Reviewed By: jeremydubreil
Differential Revision: D4490071
fbshipit-source-id: 325ea88
Summary: The Buck integration assumes that the output jar file of all the dependency targets are available locally in order to retrieve the analysis from these targets. However, this is not guaranteed to be true when there is a cache hit on some targets. Running `buck build` with the option `--deep` forces this property.
Reviewed By: jvillard
Differential Revision: D4474036
fbshipit-source-id: accabfa
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:
The thread-safety and annotation reachibility analyses were defining ugly custom versions of override_exists with refs.
Let's give them a canonical, ref-free version instead.
Reviewed By: jberdine
Differential Revision: D4475777
fbshipit-source-id: 0bb45fc
Summary: Simple model for List methods that write to the collection.
Reviewed By: peterogithub
Differential Revision: D4453381
fbshipit-source-id: 19edc51
Summary:
One of the things that confuses me about the current annotations API is that there's a lot of ways to do the same thing.
Some of the concepts like `annotated_signature` are only really needed by Eradicate.
This diff removes usages of `annotated_signature` outside of Eradicate (everyone else was just using `get_annotated_signature` as a roundabout way to get the return annotation of a procedure).
In the future, I'll move `get_annotated_signature` and other Eradicate-specific functionality into its own module inside the Eradicate directory.
Reviewed By: jberdine
Differential Revision: D4472058
fbshipit-source-id: 5bb0846
Summary:
Stmt shouldn't have to return anything in res_trans.exps - those results shouldn't be used by parent nodes.
I need this change for another diff but given the risk I decided to put it as separate diff
Reviewed By: jvillard
Differential Revision: D4474936
fbshipit-source-id: 0537749
Summary:
`pdesc_has_annot` checks the annotations of both the return values and the parameters, which seems like a bad idea in general.
The client should have to specify which annotations they actually care about.
Converting existing uses of `pdesc_has_annot` to what I read as the intended behavior (checking the return annotation).
Will make better use of the other new functions in a follow-up.
Reviewed By: jeremydubreil
Differential Revision: D4469885
fbshipit-source-id: de5531e
Summary:
Passing the context was a bit messy and there was duplicated code. The flow is as follows:
For each node, we analyze that node and then it's children, and in some cases we use the parent node
for specifying a context for its children. Now this should be a bit more clear.
In the case of if, we specify a context based on the condition that is expected to be true inside the
if body, but not inside the else body. Here only the framework is set, more if conditions will be added to the
context in future diffs.
Reviewed By: martinoluca
Differential Revision: D4462247
fbshipit-source-id: 3512bd2
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:
Remove the remaining uses of polymorphic equality `=`.
In case of basic types, this is replaced by String.equal or Int.equal.
In case of `= []`, this is replaced by `List.is_empty`.
In case of `= None`, this is replaced by `is_none`.
In case of a datatype definition such as `type a = A | B`,
a `compare_a` function is defined by adding `type a = A | B [@deriving compare]`
and a `equal_a` function is defined as `let equal_a = [%compare.equal : a]`.
In case of comparison with a polymorphic variant `= `Yes`, the equality
defined in `PVariant.(=)` is used. Typically, `open! Pvariant` is added
at the beginning of the file to cover all the uses.
Reviewed By: jberdine
Differential Revision: D4456129
fbshipit-source-id: f31c433
Summary: Also updated the doc comment of `parse`, it had gone a bit out of sync.
Reviewed By: jberdine
Differential Revision: D4435580
fbshipit-source-id: bb1a6f2
Summary:
Some of the new thread-safety annotations are somewhat difficult to understand based on the name alone.
Devs can link a sources JAR with the annotations in their IDE to quickly understand the purpose of each annotation.
Reviewed By: jeremydubreil
Differential Revision: D4451748
fbshipit-source-id: 1ba6060
Summary: The previous version of the code could fail if the class filename would contain "class" as part of the name and not be a valid class filename ending with the `.class` suffix
Reviewed By: sblackshear
Differential Revision: D4451859
fbshipit-source-id: 874832a
Sam Blackshear
Jules Villard
Cristiano Calcagno
Jeremy Dubreil
Dulma Churchill
Dino Distefano
Andrzej Kotulski
Kihong Heo
Josh Berdine