Summary:
This diff adds fields for ObjC properties to the type environment. For example,
```
property type fieldname;
```
when the property name is "fieldname", this diff adds a struct field of the same name.
The missing type information were problematic in inferbo, since its semantics depend on types.
Reviewed By: ezgicicek
Differential Revision: D28421998
fbshipit-source-id: e24059846
Summary:
Most/all of the time we expect the history of the value to faithfully
trace how it got allocated. That history was then added as a prefix of
the trace leading to the same place, leading to duplicate information in
the report trace.
We may need to do the same for other bug types.
Reviewed By: ezgicicek
Differential Revision: D28536891
fbshipit-source-id: a83a2d038
Summary: Showcase the trace duplication, fixed in a further diff.
Reviewed By: ezgicicek
Differential Revision: D28536889
fbshipit-source-id: f23636368
Summary:
Make it more obvious why we don't add an Allocated attribute in these
models.
Reviewed By: ezgicicek
Differential Revision: D28536892
fbshipit-source-id: 643539ae6
Summary:
More straightforward (and better asymptotic complexity, not that it
matters) that way. Also log when a leak is found in the debug html.
Reviewed By: ezgicicek
Differential Revision: D28536443
fbshipit-source-id: 08c329100
Summary:
The returned options were never used or only used in cases when they can
only be `None`, as far as I can tell.
Reviewed By: da319
Differential Revision: D28536428
fbshipit-source-id: c16ed4698
Summary:
As explained in the code comment, these reports are generally
non-actionable at best and false positives at worst:
skip reporting for constant dereference (eg null dereference) if the source of the null value is
not on the path of the access, otherwise the report will probably be too confusing: the actual
source of the null value can be obscured as any value equal to 0 (or the constant) can be
selected as the candidate for the trace, even if it has nothing to do with the error besides
being equal to the value being dereferenced
Reviewed By: da319
Differential Revision: D28350193
fbshipit-source-id: 0cd76d252
Summary:
Turns out the mistake was pretty simple: we just forgot to keep the
history of the return value in the callee and add it to the caller's.
Reviewed By: skcho
Differential Revision: D28385941
fbshipit-source-id: 40fe09c99
Summary:
This PR adds race condition detection support on CIL backed languages, such as .NET platform languages.
We will add unit tests later since we are still fine tunning Infer# translation module.
Pull Request resolved: https://github.com/facebook/infer/pull/1443
Reviewed By: jvillard
Differential Revision: D28505195
Pulled By: ngorogiannis
fbshipit-source-id: f263f6ba6
Summary:
This diff fixes inefficient config impact data checking.
Problem: When writing `config-impact-report.json`, it checks if a procedure (`f`) is included in the config impact data set as follows. `cut_parameter` is a function that removes parameters from ObjC method names.
```
ConfigProcnameSet.exists (fun g -> cut_parameter f = cut_parameter g) config_data
```
However, this was very inefficient because it must have iterated all members in the set always. This diff changes it to call `Set.mem` by preparing revised config impact data set (`config_data'`) in which parameters were cut in advance:
```
ConfigProcnameSet.mem (cut_parameter f) config_data'
```
Reviewed By: ezgicicek
Differential Revision: D28506113
fbshipit-source-id: 434d1f083
Summary: Similar as for NSDictionary, nil issues for array literals are caught because of the additional load instruction in the frontend, and we leave modelling arrayWithObjects:count: for later.
Reviewed By: jvillard
Differential Revision: D28442767
fbshipit-source-id: a2f0d4dbf
Summary:
Follow similar approach as in the translation of dictionary literal to insert load instruction to catch nil insertion into collection issues. The missing load instruction was causing false negatives in biabduction. This will also help Pulse to catch nil insertion into collection issues for array literals.
Facebook
Reviewed By: skcho
Differential Revision: D28442642
fbshipit-source-id: b530ac21b
Summary: The counter that accumulates the number of modified source files was logged before it is computed, leading to always zero results.
Reviewed By: jvillard
Differential Revision: D28505378
fbshipit-source-id: 833fb6072
Summary: Similar as for other collections we leave modelling setWithObjects:count: and initWithObjects:count for later.
Reviewed By: skcho
Differential Revision: D28473361
fbshipit-source-id: 4bf57035a
Summary:
In Buck/Java the global type environments of each buck target captured need to be merged. So do the capture DBs. These two tasks can be done concurrently, as both have a computation and an I/O component, and interleaving them should improve perf.
Indeed, profiling the merge process with `offcputime.py` and `cpudist.py` (BPF tools) showed a significant amount of off-cpu time in tests (>40%) as well as a distribution of timings for off-cpu intervals that agrees with IO on a fast medium (ssd).
This diff forks a process to merge the type environments while doing the DB merge as normal. Initial results show an almost 2x improvement.
Reviewed By: skcho
Differential Revision: D28438808
fbshipit-source-id: 89c96f25b
Summary: This diff comments out a test that introduces non-deterministic analysis result.
Reviewed By: rgrig
Differential Revision: D28440794
fbshipit-source-id: 95e6fbe06
Summary:
Collect imports and exports in a data structure ("names environment")
that is easy to look up.
Background:
A function call f(a1,...,an) is shorthand for m:f(a1,...,an) if there is
a -import(m, [..., f/n, ...]); otherwise it is shorthand for c:f(a1,...,an)
where c is the current module. There is an implicit import of the
special "erlang" module. Any ambiguity (e.g., imported twice, or
imported and local) is an error. Also, if there is a -export([...,
f/n,...]) then f/n should be marked as public (ProcAttributes)
Reviewed By: jvillard
Differential Revision: D28290252
fbshipit-source-id: f6d777eb6
Summary: `dictionaryWithObjectsForKeysCount` is a bit more complicated as we need to know if an element of an array is nil. Leaving it for later.
Reviewed By: skcho
Differential Revision: D28413859
fbshipit-source-id: 7b5116de8
Summary:
- Changed "passed as argument to f" to "in call to f", as these do not
always correspond to passing an argument (eg could be a value returned
from f)
- Changed "assigned" to "returned" when appropriate
- Changed the model of malloc() to not say "allocated" in the null case
- Don't print "returned from f" when there was no event inside f: just
print "in call to f".
Reviewed By: da319
Differential Revision: D28413900
fbshipit-source-id: bc85625e3
Summary: This diff copies each field values inside setter/getter of ObjC++.
Reviewed By: da319
Differential Revision: D28413584
fbshipit-source-id: 4c663fc9e
Summary: There is no need to model anything, Pulse is able to catch nil insertion into NSDictionary literals because the frontend dereferences keys and values during the translation of NSDictionary literals
Reviewed By: jvillard
Differential Revision: D28383176
fbshipit-source-id: 01a064daf
Summary: Current traces are difficult to read since they keep mentioning the same leaf call at each step. This diff improves the traces by tracking the intermediate callers.
Reviewed By: skcho
Differential Revision: D28384762
fbshipit-source-id: 78c4cbf7f
Summary:
The order was reversed when printing the trace, leading to confusion.
Also make sure we indicate which part of the trace we are printing when
there is more than one part (either context + access or invalidation +
access, or all three).
Also start nesting at <calling context length> to better represent the
role of the calling context visually.
Reviewed By: da319
Differential Revision: D28329263
fbshipit-source-id: b691fb1f4
Summary:
This diff addresses `GenericArrayBackedCollection.field` and others as pointers. The modeled fields are used as non-pointer struct fields, but their actual semantics are pointers that may have side effects.
For example, `GenericArrayBackedCollection.field` is used for keeping an information that the previous vector's address could be invalid.
```
void foo(vector v) {
v.push_back(0); // v's previous address may be invalid after push_back
// PRE: {v -> {backing_array -> v1}}
// POST: {v -> {backing_array -> v2}}
// ATTR: {v1 may be invalidated}
}
```
However, if we revert the modeled field values, it will return incorrect summary as follows, by reverting non-pointer parameter values.
```
// PRE: {v -> {backing_array -> v1}}
// POST: {v -> {backing_array -> v1}}
// ATTR: {v1 may be invalidated}
```
Reviewed By: jvillard
Differential Revision: D28324161
fbshipit-source-id: 96451d4b0
Summary:
`mutableDictionary[key] = value`, crashes if key is nil, however, if value is nil, any object corresponding to a key will be removed from the dictionary.
Under the hood, `NSMutableDictionary.setObject:forKeyedSubscript:` is called by `mutableDictionary[key] = value`.
Reviewed By: ezgicicek
Differential Revision: D28288789
fbshipit-source-id: e4e1c4288
Summary:
Rebar3.capture now calls into ErlangTranslator to obtain Sil. For now,
ErlangTranslator does nothing interesting.
Reviewed By: skcho
Differential Revision: D28261799
fbshipit-source-id: 0603db671
Summary:
This diff compares ObjC method names loosely when checking whether it is in the config impact data
file or not. This is to cover the cases where method parameters changed.
Reviewed By: jvillard
Differential Revision: D28259169
fbshipit-source-id: e6070df9c
Summary:
The wrapper in `infer/lib/erlang/erlang.sh` dumps Erlang AST forms [1]
in a JSON format. The current commit parses that JSON to obtain an
internal representation (ErlangAst). The main parts of the commit are:
- data structures for Erlang AST
- parser (Erlang abstract forms in JSON format -> Eralng AST)
- Rebar3.ml now drives the parser
[1] https://erlang.org/doc/apps/erts/absform.html
Reviewed By: mmarescotti, jvillard
Differential Revision: D28096896
fbshipit-source-id: b21263817
Summary:
There's been regressions in --pulse-isl. Without tests, everything is
temporary!
Note: the regressions are presumably still there, this just records the
current status of pulse.isl.
Also, no objective-C(++) at the moment. Should we add them too? (in
another diff)
Reviewed By: skcho
Differential Revision: D28256703
fbshipit-source-id: 700b2cc57
Summary:
Added a simple Erlang project to be used as a test for Rebar3
integration, in the following commits. Also, updated the copyright
linter to understand Erlang.
Reviewed By: ngorogiannis, mmarescotti
Differential Revision: D28096899
fbshipit-source-id: 94f15c277
Summary:
A previous change made pulse look into value histories for causes of
invalidation in case the access trace of a value already contained the
reason why that value is invalid, in order to save printing the
invalidation trace in addition to the access trace. It also made
reporting more accurate for null dereference as the source of null was
often better identified (in cases where several values are null or
zero).
But, the history is also relevant to the bug type and the error message.
Make these take histories into account too.
Also fix a bug where we didn't look inside the sub-histories contained
within function calls when looking for an invalidation along the
history.
Reviewed By: da319
Differential Revision: D28254334
fbshipit-source-id: 5ca00ee54
Summary:
There's already all the ingredients to treat function pointers pretty
well, even when stored inside (const) globals.
In OpenSSL they use something like the added tests but the globals are
not const... This may need tweaking via an option, eg to inline all
global initializers, or filtered by global names/file names. Or just
use the existing --pulse-model-{alloc,release}-pattern options.
Reviewed By: skcho
Differential Revision: D28221651
fbshipit-source-id: 5399f1141
Summary:
When garbage-collecting addresses we would also remove their attributes.
But even though the addresses are no longer allocated in the heap, they
might show up in the formula and so we need to remember facts about
them.
This forces us to detect leaks closer to the point where addresses are
deleted from the heap, in AbductiveDomain.ml. This is a nice refactoring
in itself: doing so fixes some other FNs where we sometimes missed leak
detection on dead addresses.
This also makes it unecessary to simplify InstanceOf eagerly when
variables get out of scope.
Some new {folly,std}::optionals false positives that either are similar to existing ones or involve unmodelled smart pointers.
Reviewed By: da319
Differential Revision: D28126103
fbshipit-source-id: e3a903282
Sungkeun Cho
Jules Villard
Xiaoyu Liu
Daiva Naudziuniene
Nikos Gorogiannis
Artem Pianykh
Radu Grigore
Ezgi Çiçek
Loc Le
Josh Berdine