Summary:
New analysis in foetal form to detect invalid use of C++ objects after their
lifetime has ended. For now it has:
- A domain consisting of a graph of abstract locations representing the heap, a map from program variables to abstract locations representing the stack, and a set of locations known to be invalid (their lifetime has ended)
- The heap graph is unfolded lazily when we resolve accesses to the heap down to an abstract location. When we traverse a memory location we check that it's not known to be invalid.
- A simple transfer function reads and updates the stack and heap in a rudimentary way for now
- C++ `delete` is modeled as adding the location that its argument resolves to to the set of invalid locations
- Also, the domain has a really crappy join and widening for now (see comments in the code)
With this we already pass most of the "use after delete" tests from the
Ownership checker. The ones we don't pass are only because we are missing
models.
Reviewed By: mbouaziz
Differential Revision: D10383249
fbshipit-source-id: f414664cb
Summary:
It avoids raising an exception when unexpected arguments are given to
placement new. We will revert this after fixing the frontend to parse
user defined `new` correctly in the future.
Reviewed By: mbouaziz
Differential Revision: D10378136
fbshipit-source-id: d494f781b
Summary:
Use same code for deciding whether two accesses conflict across java/clang, by adapting that of the clang version.
Eliminate/simplify some code.
Reviewed By: mbouaziz, jberdine
Differential Revision: D10217383
fbshipit-source-id: dc0986d05
Summary:
It unsets `var_exp_typ` of `trans_state` during the translations of
placement parameters, so they are translated independently against the
target variable and class of the `new` function.
Reviewed By: mbouaziz, jvillard
Differential Revision: D10161419
fbshipit-source-id: 7f588a91c
Summary: It enables placement_new to get three parameters, which happens when placement_new is overloaded (e.g. Boost).
Reviewed By: mbouaziz
Differential Revision: D10100324
fbshipit-source-id: 0ecb0a404
Summary:
Fix the logic for computing duplicate symbols. It was broken at some point and some duplicate symbols creeped into our tests. Fix these, and add a test to avoid duplicate symbols detection to regress again.
Also, this removes one use of `Cfg.load`, on the way to removing file-wide CFGs from the database.
Reviewed By: ngorogiannis
Differential Revision: D10173349
fbshipit-source-id: a0d2365b3
Summary: It uses big int, instead of 63bits int of OCaml, in the interval domain in order to get preciser numeric values in the future.
Reviewed By: jvillard
Differential Revision: D10123364
fbshipit-source-id: c217f4366
Summary:
Make distinct reports on strict mode violations.
For now, restrict to direct violations (UI threads calls transitively a violating method).
Will assess impact and enable indirect reports later (via locks).
Reviewed By: mbouaziz
Differential Revision: D10126780
fbshipit-source-id: 9c75930bc
Summary:
New clang in the plugin \o/
Changes that were needed:
- (minor) Some extra AST nodes
- defining a lambda and calling it in the same line (`[&x]() { x = 1; }()`) used to get translated as a call of the literal but now an intermediate variable gets created, which confuses uninit in one test. I added another test to showcase the limitation this is hitting: storing the lambda in a variable then calling it will not get caught by the checker.
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: jeremydubreil
Differential Revision: D10128626
fbshipit-source-id: 8ffd19f3c
Summary: Before this diff, the analysis would only lookup the attributes with the classname appearing in the instruction. However, it would fail to find those attributes for inherited and not overridden methods. With this diff, the attributes are now searched recursively in the super classes.
Reviewed By: mbouaziz
Differential Revision: D10007469
fbshipit-source-id: 77d721cba
Summary: This fixes some cases of false positives where the analysis will compare with the wrong overridden methods. This could later be improved with the possibility to do sub-typing comparison on the parameters.
Reviewed By: ngorogiannis
Differential Revision: D9985249
fbshipit-source-id: 7998d8619
Summary:
Keep `--analyzer` around for now for integrations that depend on it.
Also deprecate the `--infer-blacklist-path-regex`,
`--checkers-blacklist-path-regex`, etc. in favour of
`--report-blacklist-path-regex` which more accurately represents what these do
as of now.
Rely on the current subcommand instead of the analyzer where needed, as most of
the code already does.
Reviewed By: jeremydubreil
Differential Revision: D9942809
fbshipit-source-id: 9380e6036
Summary:
Goal of the stack: deprecate the `--analyzer` option in favour of turning
individual features on and off. This option is a mess: some of the options are
now subcommands (compile, capture), others are aliases (infer and checkers),
and they can all be replicated using some straightforward combination of other
options.
This diff: stop using `--analyzer` in tests. It's mostly `checkers` everywhere,
which is already the default. `linters` becomes `--no-capture --linters-only`.
`infer` is supposed to be `checkers` already. `crashcontext` is
`--crashcontext-only`.
Reviewed By: mbouaziz
Differential Revision: D9942689
fbshipit-source-id: 048281761
Summary: It is common on Android code to recycle the `View` object by nullifying them in the `onDestroy()` or `onDestroyView()` methods. In this case, the outer `Fragment` object structure is preserve while the inner `View` object are set to null for the garbage collect to release the memory. However, if the fields are only set to `null` in the `onDestroy*()` methods, those fields cannot be `null` during the active lifecycle of the `Fragment`, so it is not necessary to annotate those fields with `Nullable`.
Reviewed By: mbouaziz
Differential Revision: D10024458
fbshipit-source-id: b05e538d9
Summary:
The method matcher is now used sufficiently it warrants refactoring out into its own module.
Also, kill dev-android-strict-mode and leave starvation-strict-mode as the stronger option.
Reviewed By: jeremydubreil
Differential Revision: D9990753
fbshipit-source-id: 626a70a19
Summary:
The model for `getcwd` assumes the first argument should be non-null when in fact a NULL pointer is legitimate and results in allocation:
> As an extension to the POSIX.1-2001 standard, glibc's getcwd() allocates the buffer dynamically using mal‐
> loc(3) if buf is NULL. In this case, the allocated buffer has the length size unless size is zero, when buf
> is allocated as big as necessary. The caller should free(3) the returned buffer.
I suggest this glibc extension be used for the getcwd model to reduce false positives.
Pull Request resolved: https://github.com/facebook/infer/pull/925
Reviewed By: mbouaziz
Differential Revision: D9830450
Pulled By: jvillard
fbshipit-source-id: 95c4862b1
Summary: Always read the attributes from the attributes DB instead of trying to read the attributes from the analysis summaries
Reviewed By: mbouaziz
Differential Revision: D9845085
fbshipit-source-id: aef48e6bf
Summary: No longer report inconsistencies with the annotations with subtyping when the super class is in an external packages since those warnings are not necessarily accurate or actionable.
Reviewed By: ezgicicek
Differential Revision: D9845098
fbshipit-source-id: 1f2bcd739
Summary: Sometimes it's very confusing to see why infer believes a method is running on the UI thread. Make a trace out of all the relevant info.
Reviewed By: mbouaziz
Differential Revision: D9781212
fbshipit-source-id: 6d018e400
Summary:
Turn off by default until mature enough.
Also rename the dev-strict-mode test dir to highlight the dev part.
Reviewed By: mbouaziz
Differential Revision: D9775571
fbshipit-source-id: c3a41bbdf
Summary:
First step in writing an analyzer that is meant to run only on Android core library implementation.
This will, when finished, compute the library entrypoints that may lead to a strict mode violation.
The normal analyzer will use those to statically flag strict mode violations in app code.
Strict Mode is an Android debug mode, where doing certain things (like disk read/write or network activity) on the UI thread will raise an exception. We want to statically catch these, as well as indirect versions (the UI thread takes a lock and another thread holding that lock calls a method that would be a strict mode violation).
Reviewed By: mbouaziz
Differential Revision: D9634407
fbshipit-source-id: c30bcedb3
Summary: We had a special case for fixing false positives on constexpr implicitly captured by lambdas. However, we do not report dead stores on constexpr anymore, hence, do not need the special case anymore. Moreover, the special case was not only capturing constexpr in lambdas, but also any variables which type had `const` (see new test `capture_const_bad` which was not being reported before this diff)
Reviewed By: mbouaziz
Differential Revision: D9654848
fbshipit-source-id: 882fd2804
Summary:
It simplifies abstract memory instantiations of function calls. Now it instantiates callee memories by directly evaluating symbol paths, rather than constructing `subst_map`.
main changes are:
- no construction of `subst_map` and `trace_map`
- no symbol table in Inferbo's summary
- no `Symbol_not_found` exception (for when a required symbol was unavailable in `subst_map`)
Reviewed By: mbouaziz
Differential Revision: D9495597
fbshipit-source-id: 18cdcd6f7
Summary:
Separate and rename error reporting functions that use the biabduction state.
No checkers should call these functions.
Reviewed By: da319
Differential Revision: D9633579
fbshipit-source-id: 884fcee66
Summary: We report dead store false positives in template arguments when constexpr is used. To remove the false positives, with the expense of some false negatives, we do not report dead stores on constexpr anymore.
Reviewed By: mbouaziz
Differential Revision: D9608095
fbshipit-source-id: 91b0c71c4
Summary: When a typedef-ed structure is defined in another source file, `tenv` returns a structure with empty fields.
Reviewed By: mbouaziz
Differential Revision: D9629200
fbshipit-source-id: 8859803f9
Summary:
Lambdas can capture references to locals of the enclosing method as long as
they are not propagated outside the method. However to keep things simple
always allow them to capture locals of the enclosing method at the price of
some false negatives.
Reviewed By: da319
Differential Revision: D8974434
fbshipit-source-id: 957ae44bd
Summary:
It returns unknown values on non-const function calls like on unknown
function calls.
Reviewed By: mbouaziz
Differential Revision: D9478862
fbshipit-source-id: 4b795ec55
Summary: `CONDITION_ALWAYS_**` can be introduced by global constants.
Reviewed By: ezgicicek, mbouaziz
Differential Revision: D9478528
fbshipit-source-id: 7b1a46e7a
Summary:
After some testing, it looks like getting the pdesc via
`Ondemand.get_proc_desc` will also load models' proc descs from their
summaries, so this code should not be needed.
Reviewed By: jeremydubreil, mbouaziz, martintrojer
Differential Revision: D9197176
fbshipit-source-id: 1b8603bfa
Summary: The `procedure` field in the final report should use the non-ambiguous fully qualified name containing the Java package declaration and the list of parameter types.
Reviewed By: mbouaziz
Differential Revision: D9237522
fbshipit-source-id: e9b0ff664
Summary: C++17 introduce guaranteed copy elision which omits constructor calls. In ownership analysis, we depended on these constructor calls to acquire ownership. In particular, when a method returns struct, previously, a constructor was used to acquire ownership. In this diff, we acquire ownership of the returned structs directly.
Reviewed By: mbouaziz
Differential Revision: D9244302
fbshipit-source-id: ae8261b99
Summary:
When we see `pthread_create(..., ..., foo, ...)`, we want to call the function
`foo` to check that its precondition is met. The initial goal was to get rid
of the uncouth call to `Summary.get` when what we really want is to analyse
`foo` instead of just betting on the fact that it has been analysed already.
Besides switching to `Ondemand.analyze_proc_name`, this also changes the
matching of the function pointer in the arguments of `pthread_create()` to
detect the common case of a constant function name. I also added tests.
Reviewed By: jeremydubreil
Differential Revision: D9195159
fbshipit-source-id: dfec79f14
Summary: Errors that include temporary variables are difficult to understand. Do not report stack variable address escape on temporary variables.
Reviewed By: jvillard
Differential Revision: D9117517
fbshipit-source-id: 9ebd75ecc
Summary: Treat calls to Thread.sleep as blocking, even when the timeouts are less than the ANR limit.
Reviewed By: da319
Differential Revision: D9027950
fbshipit-source-id: 001409896
Summary:
It adds relational domains to Inferbo: octagon of Apron and polyhedra of Elina.
- Each Mem domain value includes one relational value containing relations among symbols. The relational values are modified by the `Prune` and `Store` commands.
- Each abstract value includes three symbols, which represent integer value, array offset, and array size of an abstract value.
The relational domain is deactivated by default. Use the `--bo-relational-domain {oct, poly}` option for the activation, though Inferbo with the relational domains does not work at this point because some modifications of Apron and Elina we made has not been applied to their opam repositories yet.
Reviewed By: jvillard
Differential Revision: D8874102
fbshipit-source-id: 08e5883cb
Summary:
Guava subclasses Future in ways that make .get() calls safe from the UI thread.
Treat such methods as skip.
Reviewed By: jeremydubreil
Differential Revision: D9013475
fbshipit-source-id: 38373aa5f
Summary:
Infer does the right thing now, make sure it doesn't regress.
https://github.com/facebook/infer/issues/86
Reviewed By: mbouaziz, dulmarod
Differential Revision: D8442855
fbshipit-source-id: 3df29b88c
Summary:
This previous FP does not occur anymore, make sure it doesn't regress.
https://github.com/facebook/infer/issues/49
Reviewed By: dulmarod
Differential Revision: D8442842
fbshipit-source-id: b3fde1ad1
Summary:
Litho uses an idiom as follows. `Component.Builder` has an abstract method `T getThis()` which is supposed to be implemented as `{return this;}`.
I believe the reason it's there is to have covariant return types in subclasses, though this doesn't really matter.
We don't do dynamic dispatch, so instead of summarising `getThis` as having a return ownership of `OwnedIf({ 0 })` we just return `Unowned`.
This is generating many false positives, which are really hard to debug.
Here I'm special-casing any abstract method whose return type is equal to that of it's only argument, to return conditional ownership.
Reviewed By: da319
Differential Revision: D8947992
fbshipit-source-id: 33c7e952d
Summary: Exceptional successors were not meant to be created for return nodes, but they were created if try block had a single return statement.
Reviewed By: jvillard
Differential Revision: D8913371
fbshipit-source-id: 6ac85b21d
Summary:
Currently, some blocking calls are turning up too many false positives. Adjust severities by fix rate/preexisting numbers.
Also, restrict some calls to exact class, as opposed to superclasses.
Reviewed By: mbouaziz
Differential Revision: D8850599
fbshipit-source-id: 47543d04a
Summary:
It adds relational domains to Inferbo: octagon of Apron and polyhedra of Elina.
- Each `Mem` domain value includes one relational value containing relations among *symbols*. The relational values are modified by the `Prune` and `Store` commands.
- Each abstract value includes three *symbols*, which represent integer value, array offset, and array size of an abstract value.
The relational domain is deactivated by default, so this diff should not make any differences in CI.
Use `--bo-relational-domain {oct, poly}` for the activation, though Inferbo with the relational domains does not work at this point because some modifications of Apron and Elina we made has not been applied to their opam repositories yet.
Reviewed By: mbouaziz, jvillard
Differential Revision: D8478542
fbshipit-source-id: 510ff53
Summary:
Do no computation of stability abstract state if not explicitly requested via the command line flag.
Also, simplify the reporting.
Reviewed By: jeremydubreil
Differential Revision: D8614885
fbshipit-source-id: 25dd9de
Summary:
The addresses of global variables do not need initialisation to exist and be valid as they are part of the code or data segment of the program. This means that taking the address of a global is not in itself a danger for SIOF. However, dereferencing such an address would be. In order to avoid false positives but avoid being too unsound, only ignore them when the address is taken only to set another global. The general case would require a more complicated abstract domain.
Fixes#866
Reviewed By: ngorogiannis
Differential Revision: D8055627
fbshipit-source-id: 92307b2
Summary:
These just point to expressions that we know how to translate.
Fixes#950
Reviewed By: mbouaziz
Differential Revision: D8713784
fbshipit-source-id: 9eafa39
Summary: The frontend got better and now `assert` is correctly translated.
Reviewed By: dulmarod
Differential Revision: D8731877
fbshipit-source-id: ba11b0c
Mehdi Bouaziz
Dino Distefano
Jules Villard
Sungkeun Cho
Ezgi Çiçek
Nikos Gorogiannis
Jeremy Dubreil
Julian Sutherland
Martin Trojer
Thomas M. DuBuisson
Daiva Naudziuniene
Martino Luca
Dulma Churchill
Kai Yang