Summary:
The problem in Reporting.ml:log_issue_from_summary is that it merely
checks the presence of `SuppressLint` annotation on method's body to
decide whether to log or not the issue. This means that regardless of
issue types specified in `SuppressLint`, all issues on such method will
get blocked.
Here we fix that.
Reviewed By: ngorogiannis, mityal
Differential Revision: D24726604
fbshipit-source-id: c9cae3833
Summary:
This diff adds an option hiding function pointers in costs to users: `cost-suppress-func-ptr` is
true by default.
Reviewed By: ezgicicek
Differential Revision: D24448212
fbshipit-source-id: 88f6b5ea1
Summary:
Take another page from the Incorrectness Logic book and refrain from reporting issues on paths unless we know for sure that this path will be taken.
Previously, we would report on paths that are merely *not impossible*. This goes very far in the other direction, so it's possible we'll want to go back to some sort of middle ground. Or maybe not. See the changes in the tests to get a sense of what we're missing.
Reviewed By: ezgicicek
Differential Revision: D24014719
fbshipit-source-id: d451faf02
Summary: We add a naive model for `forEach` idiom for Java's Iterable and Maps. The model is naive because it doesn't take the cost of the lambda into account. This will be fixed later.
Reviewed By: da319
Differential Revision: D23868203
fbshipit-source-id: 37d169c6f
Summary:
This can be used by additional tooling for further analysis (e.g.
codemods, autofixes, etc).
Reviewed By: ngorogiannis
Differential Revision: D23987694
fbshipit-source-id: b9fa343ac
Summary: Subtle false positives and negatives in Hil make Sil preferable. This diff gets rid of the CFG-emulation of Hil, while still using Hil expressions.
Reviewed By: da319
Differential Revision: D23815026
fbshipit-source-id: 731a6d299
Summary:
The previous diffs recorded it for the case when the unvetted value is
dereferenced or otherwise used wrongly. This case finishes the work,
recording the needed signature for the remaining case (when the
offending third party has a non-nullable param with nullable passed
inside)
Reviewed By: ngorogiannis
Differential Revision: D23706679
fbshipit-source-id: e6f641223
Summary:
If the issue is related to the use of an unvetted third party method
that can not be trusted, we record this fact.
Reviewed By: ngorogiannis
Differential Revision: D23705626
fbshipit-source-id: 851328fe5
Summary:
It was inconsistent before (we recorded it only for meta issues).
Now we always record it, which will simplify further diffs.
In the next diffs, we are going to add more fields inside `nullsafe_extra`.
Reviewed By: ngorogiannis
Differential Revision: D23705592
fbshipit-source-id: 8bbb0e7c8
Summary:
Handle the case of
- computing the range of `[lb, ub]` where `lb` contains length path and `ub` is constant. E.g. `[a.length, 2]` would give `3` range
- simplifying `c1 +/- min(c2, a.length)` to `c1 +/- min(c2, 0)`
since the length of a collection/array is always nonnegative.
This also removes some existing FPs in tests results.
Facebook
Context: we stumbled upon this issue in ObjC results TV109717121 which is corresponding to the added test case.
Reviewed By: skcho
Differential Revision: D23648807
fbshipit-source-id: 1e89ea246
Summary: We only added suffix-stripped prefixes of props but this causes FPS if a component declares props that actually contain suffixes since we strip them when we are adding builder calls. The fix is to add both normal and stripped versions.
Reviewed By: skcho, Katalune
Differential Revision: D23375405
fbshipit-source-id: e99cb4dd8
Summary:
Report errors found by running Topl on top of Pulse, when using
--topl-pulse. Topl tests now run on top of Pulse.
Reviewed By: jvillard
Differential Revision: D23030771
fbshipit-source-id: 8770c2902
Summary:
The generated code used to contain Prune statements that had boolean
connectives in their conditions. After this commit, all conditions
should have no boolean connective (LNot, LOr, LAnd) at top-level; that
is, prune conditions should be atomic.
The main motivation behind this change is that (a) frontends follow this
convention, and (b) Pulse assumes this convention.
Reviewed By: jvillard
Differential Revision: D23022273
fbshipit-source-id: 1313328e4
Summary:
We already take into account inheritance if the method inherits a known
modelled initializer method (e.g. Activity.onCreate()).
But if the method is explicitly marked as Initializer, we require its
overrides to be also marked.
This diffs fixes the behavior and makes it consistent, now this is
enough to annotate only parent class.
Reviewed By: jvillard
Differential Revision: D23135177
fbshipit-source-id: a21ff4a0e
Summary:
Current handling of lambdas is quite rudimentary. Looking at test
results we can see that errors are all over the place: False Positives,
False Negatives and just plain wrong results.
These tests can be grouped in 2 sets:
1. Basic support which implies:
- understanding method signatures,
- providing comprehensible error messages.
2. Extended support with implies:
- understanding scoping of values captures in lambdas (needs proper aliasing analysis).
- understanding parametric nullability in generics (needs "some"
support for Generics in our Java frontend).
With follow-up patches I'll attempt to implement "Basic" support for
Lambdas. "Extended" support will be out of scope unless there's
significant demand.
Reviewed By: mityal
Differential Revision: D23058673
fbshipit-source-id: 621551cca
Summary:
This diff adds a false positive test that is introduced imprecise loop
invariant detection.
In the example, `i` and a temp variable `$irvar = get_size(arr)` are
all included in the control variables, so the complexity becomes
quadratic. The problem is that `$irvar` is not addressed as a loop
invariant:
* `is.read` is analyzed as modifying a global
* all return variables, including `$irvar`, of unmodeled functions are invalidated
Reviewed By: ezgicicek
Differential Revision: D23051414
fbshipit-source-id: 011fd086b
Summary:
Synthetic/autogenerated methods/fields usually contain `$` in their names.
Reporting nullability violations on such code doesn't make much sense
since the violations are not actionable for users and likely need to be
resolved on another level.
This diff contributes:
1. Several test cases that involve synthetic code of different
complexity.
2. Code that handles some particular types of errors (but not all!).
Reviewed By: mityal
Differential Revision: D22984578
fbshipit-source-id: d25806209
Summary:
`java_type` aka `JavaSplitName.t` is a pair of strings. It's used in a procname to store the return type. Replace that with `Typ.t`.
This is step one of deleting `JavaSplitName`. Step two will be to replace parameters with `Typ.t`.
Reviewed By: skcho
Differential Revision: D20323748
fbshipit-source-id: f0029c3ca
Summary:
This diff removes a dead field `Struct.subs`, which was used in
heuristics finding methods from sub-classes.
Reviewed By: ezgicicek
Differential Revision: D22945346
fbshipit-source-id: 4b3bf0093
Summary:
In Cost/Inferbo checkers, it tried to find a subclass with heuristics when a method of interface or
abstract class is called. While it makes preciser analysis results in general, sometimes introduced
tricky FPs since the behavior of the heuristics depends on targets. This diff revert the heuristics
to suppress the FPs.
Reviewed By: ngorogiannis
Differential Revision: D22924485
fbshipit-source-id: 9f151231f
Summary:
There are two ways to suppress it.
1. Field level suppression annotation (was already tested). This will
apply to all constructors.
2. Constructor level annotation (this is what this test does). Sometimes
there are "fake" constructors that are not intended to be called in
prod, they might leave some fields not initialized.
Note that there are two ways to add a suppression; one has a known
problem that is documented here.
Reviewed By: artempyanykh
Differential Revision: D22864869
fbshipit-source-id: f95aaa26a
Summary:
When expressions use generics or typecasts, CFG contains intermediate `_fun_cast` nodes which break some nullsafe heuristics and lead to false positives.
This affects different types of checks (null-check on assignment expressions, `map.containsKey` checks in assignment expressions, regular typecasts, etc.
See added tests for examples.
Reviewed By: mityal
Differential Revision: D22815631
fbshipit-source-id: 80d444b1c
Summary:
The old --topl-only is now --topl-biabd-only, and there's also
--topl-pulse-only. This is WIP: the latter runs pulse, but it doesn't yet
extract Topl errors from pulse summaries. (The citv part of pulse path
conditions appears to have the necessary information.)
Reviewed By: jvillard
Differential Revision: D22815250
fbshipit-source-id: a01792945
Summary:
This diff:
1. Adds general capability to model any field as nullable /
non-nullable.
2. Uses it for Boolean.TRUE and Boolean.FALSE
Reviewed By: artempyanykh
Differential Revision: D22794226
fbshipit-source-id: 95f586592
Summary: D17500386 had added the ability to give symbolic values on functions returning exceptions. However, this might cause FPs or cryptic complexity reports (especially with subclass heuristics). This diff aims to revert it back.
Reviewed By: skcho
Differential Revision: D22764266
fbshipit-source-id: 1615544d8
Summary:
The java frontend used an unsound flow insensitive class analysis to devirtualize
some virtual calls. We remove it and let the recent devirtualizer preanalysis do the job.
This unsoudness in the Java frontend may have been here for a long time. Removing it may
modify several analysis results (specially Nullsafe) where virtual calls may look different
now.
Reviewed By: jvillard
Differential Revision: D22662739
fbshipit-source-id: c45296dce
Summary: `addAll` adds elements one by one and hence takes linear time. We didn't have a model for this and considered it O(1).
Reviewed By: skcho
Differential Revision: D22375157
fbshipit-source-id: 65b82bfae
Summary: This diff prevents printing line numbers of loop in the trace description, which helps to keep the same descriptions even when the line number of a function is changed in tests.
Reviewed By: ezgicicek
Differential Revision: D22375584
fbshipit-source-id: 676d1a7cc
Summary:
This diff adds a model of `File.listFiles` as returning an array with
a symbolic length.
Reviewed By: ezgicicek
Differential Revision: D22332258
fbshipit-source-id: 6ca593b8b
Summary: This diff adds support for `com.facebook.litho.sections.Section` which mimics the behavior for `com.facebook.litho.Component`.
Reviewed By: skcho
Differential Revision: D22309039
fbshipit-source-id: 3510441a8
Summary:
We already had a heuristic to deal with assignment expressions, but it
relied on the very previous CFG node to have a non-empty list of instrs.
In some cases, however, this previous node is a Join_node with no instrs,
so we need to take one more step back to find what we're looking for.
I've also added a bit more logging around this functionality, so it's
easier to debug/tune in future.
Reviewed By: ngorogiannis
Differential Revision: D22282930
fbshipit-source-id: 024eec145
Summary:
This diff tries to support a specific form of linked list iteration in Java.
```
while (p != null) {
p = p.getNext();
}
```
This example was a constant cost before because the cost checker could not detect that it is an iteration on a linked list.
The heuristic this diff implemented is:
(1) `p = p.getNext()`: It tries to find this specific form of assignment. Then, it increments `p.linked_list_index` by 1. Note that `linked_list_index` is a virtual field for keeping an index in the linked list. Its initial value is always 0.
(2) At `p != null`, it tries to prune the value of `p.linked_list_index`: the upper-bound of `p.linked_list_index` is pruned by `<= p.linked_list_length`. Here again, `p.linked_list_length` is also a virtual field to denote the length of the linked list.
Reviewed By: ezgicicek
Differential Revision: D22234892
fbshipit-source-id: 2fee176bb
Summary: Let's make package name match the directory name to follow Java's file lookup conventions
Reviewed By: skcho
Differential Revision: D22183964
fbshipit-source-id: b9958b975
Summary:
Document FP due to imprecision in tracking outer lock release. In a nested `synchronized` block the outer release is not registered by the abstract domain. The reason is that HIL is not resolving what `$bcvarX` is pointing to (in this case to `lockE`).
Reported by Andreea Costea.
Reviewed By: ezgicicek
Differential Revision: D22186240
fbshipit-source-id: 84e5e72b1
Summary:
Nullability of the assignment result is not refined in code snippets
like:
```
while ((a = foo.getA()) != null) {
nonNullableVal = a;
}
```
Let's add a test for this.
Reviewed By: jvillard
Differential Revision: D22136218
fbshipit-source-id: 206c368d6
Summary:
This diff adds a prototype of a new checker that collects config checkings between markers.
Basically, what the checker is doing is a taint analysis.
* Taint source: function calls of "marker start"
* Taint sink: function calls of config checking
* Taint remove: function calls of "marker end"
By the taint analysis, the analysis results will say that which taints can reach to the sink. In other words, which marker ID that has been started can reach to the config checks, before marker's ending.
I am separating the diff into three steps:
(1/3) Add basic abstract semantics
(2/3) Add trace information
(3/3) Add reporting with test examples
Reviewed By: jvillard
Differential Revision: D21998170
fbshipit-source-id: e7759f62f
Artem Pianykh
Sungkeun Cho
Ezgi Çiçek
Jules Villard
Mitya Lyubarskiy
Nikos Gorogiannis
Dulma Churchill
Radu Grigore
David Pichardie