Summary:
The attribute `[no_return]` signifies that a function doesn't return. Previously, pre-analysis had cut the links to successor nodes of such no-return function nodes. This was intended to help with suppressing reporting on unreachable paths for some analyses. However, this results in having these nodes as dangling, with no connection to exit nodes.
This diff additionally shortcuts these no-return function nodes to exit node. This would allow us to enhance inter-procedural analyses like pulse to kepp track of paths that do not return since we will be keeping their connections at exit node rather than completely cutting them of as before. It would also allow us to assume that all paths start at the one start node and end at the one exit node (at least syntactically in the CFG).
Reviewed By: skcho
Differential Revision: D20736043
fbshipit-source-id: 0eace1bdb
Summary:
D20416859 introduced a new utility
`Process.create_process_and_wait_with_output` that:
1. executes the process to completion
2. reads stdout in full
3. reads stderr in full
Unfortunately, writing to stdout/stderr can be a blocking operation for
the callee process in that situation. Double unfortunately, reading both
stdout and stderr in a way that avoids starvation requires sophisticated
Unix-fu. Fortunately, callers of this utility only ever need to read
*one* of stdout or stderr.
Fix the starvation by:
1. reading *one* channel only (either stdout or stderr)
2. doing the reading *before* `wait`ing on the process to finish
3. redirecting the other channel to the console
Reviewed By: skcho, ngorogiannis
Differential Revision: D20737388
fbshipit-source-id: 2988ac865
Summary:
Knowing the number associated with each issue is useful to pass to
`infer explore --select XXX`.
Reviewed By: skcho
Differential Revision: D20696724
fbshipit-source-id: f6f368aa1
Summary:
Used `2to3` but had to (poorly, sorry!) fix byte -> string output of processes.
update-submodule: facebook-clang-plugins
Reviewed By: ngorogiannis
Differential Revision: D20672767
fbshipit-source-id: 852c7e973
Summary:
infer/lib/python/'s not pinin'! It's passed on! This library is no more!
It has ceased to be! It's expired and gone to meet its maker! It's a
stiff! Bereft of life, it rests in peace! If you hadn't nailed it to the
perch it'd be pushing up the daisies! 'ts metabolic processes are now
'istory! It's off the twig! It's kicked the bucket, it's shuffled off
its mortal coil, run down the curtain and joined the bleedin' choir
invisible!! THIS IS AN EX-PYTHON!!
Reviewed By: ngorogiannis
Differential Revision: D20672771
fbshipit-source-id: 7808c0ece
Summary:
Re-implement the generation of an HTML report (with bug traces) in
OCaml.
Kills the --only-show as a side-effect, it is of dubious use since there
is already infer-out/report.txt to get the report list as text. A
follow-up diff adds numbers to the list in infer-out/report.txt for easy
cross-referencing with `infer explore --select 123`.
Reviewed By: skcho
Differential Revision: D20672769
fbshipit-source-id: 39b3a299d
Summary:
When executing unit tests, don't parse the arguments as they will be
parsed by OUnit.
Reviewed By: skcho
Differential Revision: D20669675
fbshipit-source-id: 897ec10ee
Summary:
The text is ambiguous because it sounds as if it recommends annotating the current class as `ThreadSafe`, not the interface invoked.
Also, remove the not useful part "or using an interface known to be thread-safe" because developers don't know in general what interfaces Infer thinks are thread-safe.
Reviewed By: skcho
Differential Revision: D20675729
fbshipit-source-id: 9da438621
Summary:
Morally, INTERFACE_NOT_THREAD_SAFE is issued when an interface method is invoked from `ThreadSafe`-annotated code on an interface that is not known to be thread-safe or annotated so.
However, the ultimate purpose is to prevent races. Thus it should never be issued on an owned object or on objects we would not report races on for any reason (local variables, non-source variables, etc).
This diff equips interface call records with the abstract address they are invoked on, and uses the same rules for maintaining those records or not.
Reviewed By: skcho
Differential Revision: D20669259
fbshipit-source-id: 6c7841e6a
Summary:
For historical reasons, the record of an access is a three-level record:
1. `AccessSnapshot`, a record with info such as ownership and lock status, including
2. `TraceElem`, a record with a trace and an element which is
3. Access, the abstract addressed accessed and the type of access.
This stack flips the order to 2, 1, 3, leading up to the possibility of merging 1 and 3.
This diff improves the domain interface and consolidates all the various validity invariant checking for accesses inside their constructors.
Reviewed By: skcho
Differential Revision: D20668611
fbshipit-source-id: 45806d40d
Summary:
For historical reasons, the record of an access is a three-level record:
1. `AccessSnapshot`, a record with info such as ownership and lock status, including
2. `TraceElem`, a record with a trace and an element which is
3. Access, the abstract addressed accessed and the type of access.
This stack flips the order to 2, 1, 3, leading up to the possibility of merging 1 and 3.
This diff inverts 2 and 1.
Reviewed By: skcho
Differential Revision: D20644100
fbshipit-source-id: 89d810b68
Summary:
For historical reasons, the record of an access is a three-level record:
1. `AccessSnapshot`, a record with info such as ownership and lock status, including
2. `TraceElem`, a record with a trace and an element which is
3. `Access`, the abstract addressed accessed and the type of access.
This stack flips the order to 2, 1, 3, leading up to the possibility of merging 1 and 3.
This diff introduces functions in (1) that mask calls to (2), making the flip easier.
Reviewed By: skcho
Differential Revision: D20619614
fbshipit-source-id: 19fda0916
Summary: In an intra-procedural analysis we assume that parameters passed by reference to a function will be initialized inside that function. We use the type information of an actual parameter to initialize the fields of the struct. This does not work if a function has a parameter of type void* as the actual parameters also has type void*. To solve this issue, we use type information from local variables.
Reviewed By: jvillard
Differential Revision: D20670253
fbshipit-source-id: dc9f051ef
Summary:
This diff adds a procedure name to the head of the trace in order to distinguish issues in the same line.
"Updated Cost is ..." is changed to "Updated Cost of <proc name> is ..."
Reviewed By: ezgicicek
Differential Revision: D20672214
fbshipit-source-id: 303b4492f
Summary:
- Model `System.exit()` as early_exit and add a test
- Tweak message of methods that are impure due to having no pulse summary (and add a test)
Reviewed By: skcho
Differential Revision: D20668979
fbshipit-source-id: 6b5589aae
Summary:
Introducing a generalization of map_changed that can now
use a context on each instruction. The context is computed with
the previous instructions in the collection.
Reviewed By: skcho, jvillard
Differential Revision: D20669993
fbshipit-source-id: 58fdee1d9
Summary: This diff avoids that an invalid interval value, e.g. [0, -1], is genrated by interval pruning.
Reviewed By: ezgicicek
Differential Revision: D20645488
fbshipit-source-id: 6516c75d1
Summary:
Hopefully no one uses this. This is in Python and we'd like to get rid
of it. Easy enough to either re-implement if needed or to be
re-implemented by a third party.
Reviewed By: ngorogiannis
Differential Revision: D20626344
fbshipit-source-id: 484022482
Summary:
Seems like a more sensible name. Most tooling should read report.json so
won't notice.
Still output a bugs.txt file with a message to point to report.txt while
people migrate.
Reviewed By: mityal, artempyanykh
Differential Revision: D20626111
fbshipit-source-id: efb84d098
Summary:
The documentation of `--quiet` dates back from when it applied only to
`InferPrint.ml`. Make it more general and more in line with
expectations one might have about a `--quiet` option:
- change the doc
- make it disable the progress bar
Reviewed By: ngorogiannis
Differential Revision: D20626110
fbshipit-source-id: db096fd31
Summary:
This is a fairly popular class that have bunch of nullable methods.
Providing an alternative will make an error message actionable.
Note that this involves some duplication, and the whole API could be
improved. But let's leave it for future improvements.
Reviewed By: jvillard
Differential Revision: D20649099
fbshipit-source-id: bfcc7fd95
Summary: The current message is recommending to change `View.findViewById()` to `View.requireViewById()`, but the latter method is not supported in all API, so might lead to a crash in runtime.
Differential Revision: D20619361
fbshipit-source-id: 542746c79
Summary: Use the an LRUCache in Ondemand.LocalCache to avoid clearing it after every toplevel analysis.
Reviewed By: ngorogiannis
Differential Revision: D20281932
fbshipit-source-id: 752c8e1ea
Summary:
- the order of call state was wrong when printing contradiction for CItv
- add a test for impurity
Reviewed By: jvillard
Differential Revision: D20646181
fbshipit-source-id: 1c86fd0a4
Summary: There are no plans currently to track which lock protects each access, so reduce to the functional equivalent of having a singleton lock domain.
Reviewed By: skcho
Differential Revision: D20595013
fbshipit-source-id: d5100ac49
Summary:
As exemplified by added tests, pulse computes an empty summary (with 0 disjuncts) whenever it discovers a contradiction which might be caused by:
- discovering aliasing in memory
- widening limited number of times in loops and concluding that loop exit conditions are never taken
However, AFAIU, it is not possible to have a function with 0 disjunct apart from such anomalities. Even a function which does nothing like `void foo(){}` has 1 disjuncts:
```
Pulse: 1 pre/post(s)
#0: PRE:
{ roots={ };
mem ={ };
attrs={ };}
POST:
{ roots={ };
mem ={ };
attrs={ };}
SKIPPED_CALLS: { }
```
The aim of this diff is to consider functions with 0 disjuncts as **impure** because most often such cases are impure, rather than actually pure.
Reviewed By: skcho
Differential Revision: D20619504
fbshipit-source-id: 3a8502c90
Summary:
Although try-with-resource is supported by nullsafe this code pattern
throws it off and make nullsafe report on a virtual **b**yte-**c**ode
variable.
Check out debug output from `TryWithResource` (or attached
visualisation of CFG):
0. node14: $bcvar2=null (on entry to try-with-resource).
1. node16: n$14=$bcvar2, but **also** PRUNE(!(n$14 == null), true). Then we go to
2. node18: do something here and in case of exception go to
3. node25->node23->node19->node20: and here we do
$bcvar2->addSuppressed(...).
Because on step 1 we refined nullability of n$14, but didn't refine
nullability of $bcvar20, on step 3 we are sure that $bcvar is null and
therefore issue an error.
Reviewed By: mityal
Differential Revision: D20558343
fbshipit-source-id: 520505039
Summary:
This is likely not the final refinement, rather one step forward.
We classify all classes by 3 categories:
- Nullsafe and 0 issues
- can add Nullsafe and will be 0 issues
- the rest (class needs improvement)
Each class will fall into exactly one category.
Error messaging is WIP, they are not intended to be surfaced to the user
just yet.
Note how this diff uses the result of the previous refactoring.
Reviewed By: artempyanykh
Differential Revision: D20512999
fbshipit-source-id: 7f462d29d
Summary: Add a flag `is-inclusive-cost` (`true` by default) which computes inclusive cost for each function. Setting the flag to `false` computes exclusive cost of the function where the cost of the callees are assumed to be `0`.
Reviewed By: skcho
Differential Revision: D20558275
fbshipit-source-id: 6b5798916
Summary:
This function is used to adapt the callee summary at a call site. It did two things for every domain element in the callee summary:
- A linear search through the list of actuals.
- For each such actual, it would (repeatedly) compute its ownership (!).
For large summaries this can be substantial. The right way is to precompute the ownership for all actuals once and then simply retrieve it (via an array).
Reviewed By: jberdine
Differential Revision: D20564447
fbshipit-source-id: 1ca3121c2
Summary: The attribute types present are exclusive, so sets are not needed for the attribute map domain. This changes `Attribute` to a flat domain and removes the set on top of that.
Reviewed By: jberdine
Differential Revision: D20560240
fbshipit-source-id: 83e59d73e
Summary:
Both modules define properties the analysis maps to addresses, there is no reason to have two modules for this.
Also remove an instance of `Caml.Not_found` usage.
Reviewed By: jberdine
Differential Revision: D20558683
fbshipit-source-id: eacafd780
Summary:
# Problem
Consider
```
some_method(Object a) { a.deref(); }
```
What is nullability of `a` when we dereference it?
Logically, things like "LocallyCheckedNonnull" etc are not applicable
here.
This would be applicable if we called some_method() outside! But not
inside. Inside the function, it can freely treat params as non-null, as
long they are declared as non-nullable.
The best we can capture it is via StrictNonnull nullability.
Reviewed By: artempyanykh
Differential Revision: D20536586
fbshipit-source-id: 5c2ba7f0d
Summary:
# Problem
Yes, nullsafe is not null-safe, such an irony.
ErrorRenderingUtils overuses `option` and `let+` constructions. Most of
internal functions can return `None` when "something is wrong".
On top of this, "default" pattern match is overused either.
Because of this, `ErrorRenderingUtils.mk_nullsafe_special_issue` returns
optional type. In practice, this result can be None for many unclear
reasons, and it is super tricky to even understand them all.
This in turn forced AssignmentRule and DereferenceRule to process this
None is defensive way. The rules have some theory why None was returned,
and have assertions along the way.
Turns out those theories might be wrong. This diff will make triaging wrong assumptions easier.
Reviewed By: artempyanykh
Differential Revision: D20535720
fbshipit-source-id: 2b81e25b7
Summary: When we have clashing args to bug (for instance -j and -Xbuck --num-threads) CLI passed -Xbuck args should win.
Reviewed By: jvillard
Differential Revision: D20557060
fbshipit-source-id: 726fc501a
Summary:
`make test` failed in some test directories, because we were getting warnings
```
Foo.java uses unchecked or unsafe operations.
```
This diff fixes or suppresses these warnings.
Reviewed By: skcho
Differential Revision: D20557572
fbshipit-source-id: 63ecd3dfa
Summary:
First version of a new memory leak check based on Pulse. The idea is to examine unreachable cells in the heap and check that the "Allocated" attribute is available but the "Invalid CFree" isn't. This is done when we remove variables from the state.
Currently it only works for malloc, we can extend it to other allocation functions later.
Reviewed By: jvillard
Differential Revision: D20444097
fbshipit-source-id: 33b6b25a2
Sungkeun Cho
Ezgi Çiçek
Jules Villard
Nikos Gorogiannis
Daiva Naudziuniene
David Pichardie
Mitya Lyubarskiy
Fernando Gasperi Jabalera
Artem Pianykh
Martin Trojer
Dulma Churchill