Summary:
refactoring Java Integer model so that it uses the new
API designed for manipulating fields in Java.
Reviewed By: jvillard
Differential Revision: D27231810
fbshipit-source-id: 0d9e3c951
Summary:
## Issue:
On `master`, it seems that there is a missing newline when Infer prints the `tenv` for a structure type:
```bash
avj@platypus /tmp/infer_bug$ cat test.c
typedef struct {
int a;
} st1;
typedef struct {
int b;
} st2;
avj@platypus /tmp/infer_bug$ infer --version
Infer version v1.0.0-55871dd28
Copyright 2009 - present Facebook. All Rights Reserved.
avj@platypus /tmp/infer_bug$ rm -rf infer-out && infer --debug run -P -- gcc -c test.c
Logs in /tmp/infer_bug/infer-out/logs
Capturing in make/cc mode...
Found 1 source file to analyze in /tmp/infer_bug/infer-out
No issues found
avj@platypus /tmp/infer_bug$ grep -A1 "dummy" infer-out/captured/*/*.tenv.debug
dummy: falsestruct st1
fields: {
--
dummy: falsestruct st2
fields: {
--
dummy: falsestruct objc_class
fields: {}
```
(notice that `dummy: false` and `struct objc_class` are on the same line, with no spacing)
## Resolution
Their PR adds an explicit newline at the end of pretty-printing a structured value, such that it is formatted correctly in the `tenv`:
```bash
avj@platypus /tmp/infer_bug$ infer --version
Infer version v1.1.0-bb5a33506
Copyright 2009 - present Facebook. All Rights Reserved.
avj@platypus /tmp/infer_bug$ rm -rf infer-out && infer --debug run -P -- gcc -c test.c
Logs in /tmp/infer_bug/infer-out/logs
Capturing in make/cc mode...
Found 1 source file to analyze in /tmp/infer_bug/infer-out
No issues found
avj@platypus /tmp/infer_bug$ grep -A1 "dummy" infer-out/captured/*/*.tenv.debug
dummy: false
struct st1
--
dummy: false
struct st2
--
dummy: false
struct objc_class
--
dummy: false
```
(*edit*: I forgot to build after committing; now with updated hash)
Signed-off-by: Andrew V. Jones <andrewvaughanj@gmail.com>
Pull Request resolved: https://github.com/facebook/infer/pull/1416
Reviewed By: skcho
Differential Revision: D27264518
Pulled By: jvillard
fbshipit-source-id: 3b86b4c22
Summary:
Before this diff, TOPL had 3 implementations:
1. a post-processing of biabduction summaries
2. a post-processing of pulse summaries
3. a deep embedding in pulse
1 and 2 additionally require instrumenting SIL to generate monitors for
the TOPL properties. 3 is faster than both 1 and 2, by a good lot, and
doesn't require instrumenting the SIL code. Thus, delete 1 and 2!
Also harmonise the CLI so that TOPL is activated by --topl, which
actives it as a checker, like other analyses.
Reviewed By: rgrig
Differential Revision: D27270178
fbshipit-source-id: e86cf972b
Summary:
Changing model for Java `Collection` interface. Every collection has now two internal fields, initially set to `null`. We also keep an extra field to compute emptiness. This model was implemented based on the [preexisting model for HashMap](https://github.com/facebook/infer/blob/master/infer/models/java/src/java/util/HashMap.java).
Existing models (`add`, `remove`, `set` and `is_empty`) have been updated accordingly and new models are provided: `init` and `clear`.
This model is not yet compatible with the `Map` interface but this change will happen very soon.
Reviewed By: ezgicicek
Differential Revision: D27126815
fbshipit-source-id: 79a5fe306
Summary: This diff ignores java.lang.Math method calls since they are all cheap.
Reviewed By: ezgicicek
Differential Revision: D27267282
fbshipit-source-id: ad0a4ef4f
Summary:
There could still be divisions by zero, eg in the "mod" case: consider
"x mod (1/2)" (doesn't matter what x is). Then we'd check "1/2 =? 0" and
since it's false conclude that it's safe to take the modulo... oops!
To make things safer, harden `Z` to not throw anymore.
Also add a layer of defense in depth by wrapping the functions that do
Z/Q operations in another layer of exception catching because we really
don't want to crash the entire analysis due to that.
Reviewed By: martintrojer
Differential Revision: D27262569
fbshipit-source-id: e22187ca0
Summary:
Previously we would only simplify when the term is exactly IsInstanceOf,
and skip sub-terms. Most of the time this is the case but in the future
this could change.
Reviewed By: skcho
Differential Revision: D27156519
fbshipit-source-id: bd10574e0
Summary:
- some editing of the text
- the documentation of NULLPTR_DEREFERENCE was duplicated in
NULL_DEREFERENCE. Make the latter point to the former instead.
Reviewed By: skcho
Differential Revision: D27162785
fbshipit-source-id: 442d6efb9
Summary:
In Pulse, it usually havoc the actual parameters to unknown functions. However, it did not do that when the lengths of actuals and formals mismatch, which may happen when the frontend doesn't have enough information about procedures.
This diff havoc the actual parameters, also when there is mismatch between lengths of actuals and formals.
Reviewed By: ezgicicek
Differential Revision: D27163143
fbshipit-source-id: 1c5e0853a
Summary:
Two methods with identical method names but different number/type of args will have the same hash: e.g. `foo(int x)` and `foo(int x, int y)`. For Config Impact analysis, we assumed this type of hash collusion would never happen when we are comparing config-impact reports, but that assumption is wrong as demonstrated by the modified tests.
To deal with these, in cost analysis, we pick the highest degree among the potential collisions. We follow a similar idea here, picking the highest number of unchecked callees.
That has its own disadvantages:
E.g. giving an example from cost, if we had `foo(int x)` with O(1) before, and after the change, we have also added a linear `foo(int x, int y)`, I think we would introduce a complexity increase.
Still, it is better than picking only the first/last.
Reviewed By: skcho
Differential Revision: D27156722
fbshipit-source-id: c37388f1c
Summary:
10 seems better at no visible CPU cost. Not very scientific as this is
only one data point, but neither was choosing 5 in the first place.
Measurements on OpenSSL using Pulse.ISL:
```
$ time infer --pulse-only --scheduler callgraph -j 2 --pulse-report-latent-issues --pulse-isl
| fuel | user time (s) | under-normalisation | latent issues |
|------+---------------+---------------------+---------------|
| 5 | 163 | 3074 | 160 |
| 10 | 158 | 85 | 160 |
| 15 | 174 | 32 | 160 |
| 20 | 186 | 20 | 160 |
```
Reviewed By: skcho
Differential Revision: D27156497
fbshipit-source-id: 1114b8677
Summary:
This is a refactoring for a later change. This change alters behaviour
slightly to make it less chaotic: instead of normalization doing:
"""
do normalize(phi) until phi doesn't change anymore
normalize(phi):
do normalize_linear_part(phi) until this doesn't change phi anymore
do other normalizations
"""
we now do
"""
do normalize(phi) until phi doesn't change anymore
normalize(phi):
normalize_linear_part(phi)
do other normalizations if linear didn't change
"""
In particular we no longer spend potentially-quadratic amouns of fuel
during normalization.
Reviewed By: skcho
Differential Revision: D26450391
fbshipit-source-id: 9f63e1a04
Summary:
- add a pp_new_eq function to help people who want to printf-debug stuff
- fix one case where new_eqs were reset to `[]` instead of propagated
- do not add to `new_eqs` when nothing changes during normalisation.
This avoids duplicated new_eqs that arise from regenerating the linear
equality relation multiple times during normalisation.
Reviewed By: da319
Differential Revision: D27156042
fbshipit-source-id: 59b093ec8
Summary: To implement nil summaries for unknown calls I would like to reuse functionality from PulseObjectiveCSummary which already depends on PulseOperations causing circular dependencies.
Reviewed By: jvillard
Differential Revision: D27155092
fbshipit-source-id: 1c300ead0
Summary:
See updated tests and code comments: this changes many arithmetic
operations to detect when a contradiction "p|->- * p=0" is about to be
detected, and generate a latent issue instead. It's hacky but it does
what we want. Many APIs change because of this so there's some code
churn but the overall end result is not much worse thanks to monadic
operators.
Reviewed By: skcho
Differential Revision: D26918553
fbshipit-source-id: da2abc652
Summary:
This first commit introduces test cases and the new summary type, in
particular how it is propagated during function calls. We don't yet
actually generate these summary types, this is for the next diff.
The goal is to catch this pattern:
```
foo(p) {
if(p) {}
*p = 42;
}
goo() { foo(NULL); }
```
We went foo(p) to be a latent error when p=0. Right now we detect a
contradiction p|->- * p=0 |- false. The next diff will fix it.
Reviewed By: skcho
Differential Revision: D26918552
fbshipit-source-id: 6614db17b
Summary: Mostly refactoring, get rid of some minor TODOs in the process.
Reviewed By: skcho
Differential Revision: D26916013
fbshipit-source-id: 53c34af05
Summary:
This is to avoid a circular dependency issue in the future when creating
summaries might cause new reports: PulseReport depends on
PulseExecuationDomain so the latter cannot emit reports. Move summary
creation functions to PulseSummary instead, which sits above both of
these modules.
Also limit the responsabilities of PulseLatentIssues to just latent
issues in preparation for another change.
Reviewed By: skcho
Differential Revision: D26915799
fbshipit-source-id: 3275cd514
Summary: Although `Set.contains` could be logarithmic in the worst case, on average, the contains() runs in O(1) time. We rather take the average here, following the most common case.
Reviewed By: ngorogiannis
Differential Revision: D27078794
fbshipit-source-id: 24e3476e8
Summary:
`Initializer` annotation is an advanced feature that should be used
sparingly. Let's remove it from the error message and hence discourage
overuse
Reviewed By: ngorogiannis
Differential Revision: D27047431
fbshipit-source-id: 8eb7ba7ab
Summary: The translation of captured by reference variables has been fixed for ObjC blocks (D26945575 (778c629401)), so we do not need to ignore them in uninit analysis anymore.
Reviewed By: skcho
Differential Revision: D27063663
fbshipit-source-id: 447084d37
Summary:
This diff handles live variables in catch blocks. To do that, this diff adds another metadata,
`CatchEntry`.
Domain change: The domain is changed to
```
(normal:variables) x (exn:try_id->variables)
```
`exn` is a map from try-catch-id to a set of live variables that are live at the corresponding entry
of catch blocks.
Semantics change: It is a backward analysis.
* on `CatchEntry`: It updates `exn` with `try_id` and current `normal`.
* on `Call`: As of now, we assume all function calls can raise an exception. Therefore, it copies
all live variables in `exn` to `normal`.
* on `TryEntry`: It removes corresponding `try_id` from `exn`.
Reviewed By: jvillard
Differential Revision: D26952755
fbshipit-source-id: 1da854a89
Summary: "Please make sure this is an expected change." makes the reports longer. We already ask for feedback on whether it is expected or not at diff time.
Reviewed By: ngorogiannis
Differential Revision: D27009995
fbshipit-source-id: d1bc6e6b1
Summary: This diff adds TryEntry and TryExit statements to the entry and exit of C++ `try` block, in order to handle exceptional control flow better in analyses.
Reviewed By: da319, jvillard
Differential Revision: D26946188
fbshipit-source-id: 33f4ae9e7
Summary:
Update Infer to LLVM (clang) 11.1.0.
Infer/clang now uses the LLVM 'monorepo' release, simplifying the download script.
Some changes done to how/when ASTExporter mangles names, this to avoid the
plugin hitting asserts in the clang code when mangling names.
Reviewed By: jvillard
Differential Revision: D27006986
fbshipit-source-id: 4d4b6ba05
Summary:
We use `procedure_name` which is coming from `Procname.get_method` in explanation of cost issues. For blocks, procedure name includes a prefix `objc_block` and a suffix with `_x` where x is the block counter. However, displaying this name to the user is not pretty. Especially when we have nested blocks, procedure name looks like `objc_blockobjc_blockdirectUIMessageFromContentAndMetadata_10_23`.
This diff drops the block index suffix and replaces `objc_block` with a prettier version `^`(signifying block).
so instead, in the cost report, we will have `^^blockdirectUIMessageFromContentAndMetadata`.
Reviewed By: skcho
Differential Revision: D26945333
fbshipit-source-id: 9d135423c
Summary: Variables captured by reference do not have correct type in objc blocks. They are missing one reference. This diff sets the correct type of captured reference variables inside procdesc, similarly as we already have for cpp lambdas. The translation of block's body will then take into account the type of captured variable from procdesc.
Reviewed By: ezgicicek
Differential Revision: D26945575
fbshipit-source-id: 06a9d9cc6
Summary: This diff adds a test replicating the issue fixed in D26975222 (7110c1ca3f) in [commit 7110c1c](7110c1ca3f)
Reviewed By: ngorogiannis
Differential Revision: D26978562
fbshipit-source-id: 41e41df8c
Summary: We shouldn't report a complexity increase here because `existing_block_here` is a removed function (that doesn't exist in current version)!
Reviewed By: skcho
Differential Revision: D26947439
fbshipit-source-id: 6620804be
Summary: Adding option to suppress errors involving unknown code. If `--pulse-report-ignore-unknown-java-methods-patterns` is provided, reports containing skipped functions not matching at least one of the given regexps are suppressed.
Reviewed By: jvillard
Differential Revision: D26820575
fbshipit-source-id: b6e1df7b2
Summary:
Adapting error messages in Pulse so that they become more intuitive for
developers.
Reviewed By: jvillard
Differential Revision: D26887140
fbshipit-source-id: 896970ba2
Summary:
RacerD needs to analyse the class initialiser in order to establish field properties in its post, such as that certain static fields are synchronized containers.
There was a bug where class initializers were not analysed at all, from the time where there was no analysis of field properties in the post.
We still don't want to report on the class initialiser since it cannot possibly race with itself (JVM guarantees that) and it cannot race with any of the other methods in its class (because it must finish before any other method can be called).
Reviewed By: da319
Differential Revision: D26887151
fbshipit-source-id: 570aff370
Summary: The `NonBlocking` annotation should zero out all domain elements that represent blocking calls. The current implementation only really removes such elements when they are generated by the current method under analysis, leaving such elements from callees unaffected. This diff fixes that.
Reviewed By: jvillard
Differential Revision: D26874704
fbshipit-source-id: 2d4859b30
Summary:
This changes the results. I think it's because we cut short paths to
ISL errors sooner now, before they are duplicated and moved. I could not
really assess what was going on though so could be wrong.
On OpenSSL 1.0.2d:
Before: 106 issues
After: 90 issues
Reviewed By: ezgicicek
Differential Revision: D26822331
fbshipit-source-id: e861e7fc2
Summary:
This will enable further improvements: basically we want to be able to
abort the symbolic execution of a single disjunct whenever an error is
detected. Right now there is only one kind of error, which is now
explicitly called `ReportableError`.
The next diff refactors Pulse.ISL to add its own error type so that we
are able to get rid of the isl_status field (ISLOk/ISLError) inside
abductive states. ISLError states are really `Error _` states but
previously it would have been too much of an API change to expose that.
Now it's all going to be part of `AccessResult.t`.
A further change will add another error type for when a value is found
to be 0 after the fact by the arithmetic.
Reviewed By: ezgicicek
Differential Revision: D26821178
fbshipit-source-id: 2923db8e7
Sungkeun Cho
Loc Le
Gabriela Cunha Sampaio
Jules Villard
Andrew V. Jones
Ezgi Çiçek
Daiva Naudziuniene
Mitya Lyubarskiy
Martin Trojer
Nikos Gorogiannis