Summary:
For more deduplications of issues, this diff loosens the condition of
similar bounds. The previous condition of similar bounds was too
strict, so [0,0] and [0,+oo] were not similar.
Depends on D10851762
Reviewed By: mbouaziz
Differential Revision: D10866127
fbshipit-source-id: 4ba912a88
Summary: For `operator=(lhs, rhs)` we want to model it as an assignment if rhs is materialized temporary created in the constructor.
Reviewed By: jvillard
Differential Revision: D10462510
fbshipit-source-id: 998341e69
Summary: Do not create a new location for placement new argument if it already exists.
Reviewed By: jvillard
Differential Revision: D12839942
fbshipit-source-id: 758b67a82
Summary:
In order to know whether a global variable is an integral constant
expression in C, this diff adds a field for the results of isInitICE.
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D12838521
fbshipit-source-id: 388bff1f3
Summary:
This may help running the id map bookkeeping on its own in the future
and makes the code slightly more readable in my opinion.
Reviewed By: mbouaziz
Differential Revision: D12858066
fbshipit-source-id: fea4aea63
Summary:
HIL wanted to do its own HTML printing, causing code duplication and hacks to
avoid double opening/closing files. Instead, pass a hook to print SIL
instructions or not.
This also makes the debug HTML be printed even in case of raised exceptions,
which is invaluable to debug crashes or even just reports in the case of
checkers that can raise `Stop_analysis` (pulse only for now).
This also print intermediate abstract states between instructions instead of
only at the start and end of nodes, for moar debugging.
Reviewed By: mbouaziz
Differential Revision: D12857425
fbshipit-source-id: 4ee6c88d6
Summary:
Get rid of `USE_AFTER_LIFETIME`. This could be useful to deploy pulse
alongside the ownership checker too.
Reviewed By: da319
Differential Revision: D12857477
fbshipit-source-id: 8e2a2a37c
Summary: Make the whole type private, introduce constructors for each variant, and deal with the consequences.
Reviewed By: da319
Differential Revision: D12825810
fbshipit-source-id: a01922812
Summary:
Keep `USE_AFTER_LIFETIME` for unclassified errors (for now it contains
vector invalidation too because I can't think of a good name for
them, and maybe it makes sense to wait until we have more types of them
to decide on a name).
Reviewed By: da319
Differential Revision: D12825060
fbshipit-source-id: bd75ef698
Summary:
Getting this right will be long and complex so for now the easiest is to
underreport and only consider as invalid the addresses we know to be invalid on
both sides of a join. In fact the condition for an address to be invalid after
a join is more complex than this: it is invalid only if *all* the addresses in
its equivalence class as discovered by the join are invalid.
Reviewed By: skcho
Differential Revision: D12823925
fbshipit-source-id: 2ca109356
Summary: Similarly as for destructors, we provide an address of an object as a first parameter to constructors. When constructor is called we want to create a fresh location for a new object.
Reviewed By: jvillard
Differential Revision: D10868433
fbshipit-source-id: b60f32953
Summary:
There were several lists constructed unnecessarily -- replaced them with find_maps
and hopefully simplified the logic.
Reviewed By: mbouaziz, jvillard
Differential Revision: D12823559
fbshipit-source-id: 1f06b20f3
Summary:
Sometimes in debug mode, the condition set is too big to print in the
log file. This diff limits the maximum number of conditions to print
as 30.
Reviewed By: mbouaziz
Differential Revision: D12836661
fbshipit-source-id: 8ddfe64a7
Summary: We provide an address of an object as a parameter to destructor. When destructor is called the object itself is invalidated, but not the address.
Reviewed By: jvillard
Differential Revision: D12824032
fbshipit-source-id: 516eebcf8
Summary:
Seems useful to know when we're printing one instruction only, but not when we
print lots of them for readability.
Reviewed By: mbouaziz
Differential Revision: D12823481
fbshipit-source-id: 2beb339f2
Summary:
It terminates narrowing when new and old states are not comparable.
Since current narrowing does not use meet operations guaranteeing
termination of narrowing, it tries to terminate narrowing more
conservatively.
Reviewed By: mbouaziz
Differential Revision: D12815419
fbshipit-source-id: e8b45199e
Summary: It tries division on minmax value approximately, rather than just returning infinities. For example, `[0,2+min(6,s)] / 2` returns `[0,4]`.
Reviewed By: mbouaziz
Differential Revision: D10867091
fbshipit-source-id: d3f49987b
Summary:
This diff preserves values of offset and index separately, rather than
one value of their addition, because premature addition results in
imprecise FPs by the limited expressiveness of the domain.
Reviewed By: mbouaziz
Differential Revision: D10851393
fbshipit-source-id: 1685ead36
Summary:
The time has come to keep track of which tests pass and which are FP/FN
for pulse.
Reviewed By: mbouaziz
Differential Revision: D10854064
fbshipit-source-id: 60938e48f
Summary:
Turns out once a vector array became invalid it stayed that way, instead
of the vector getting a new valid internal array.
Reviewed By: skcho
Differential Revision: D10853532
fbshipit-source-id: f6f22407f
Summary:
Now the domain can reason about `&` and `*` too. When recording `&`
between two locations also record a back-edge `*`, and vice-versa.
Reviewed By: mbouaziz
Differential Revision: D10509335
fbshipit-source-id: 8091b6ec0
Summary: This is more flexible and allows us to give more details when reporting.
Reviewed By: mbouaziz
Differential Revision: D10509336
fbshipit-source-id: 79c3ac1c8
Summary: Just to organise PulseDomain a bit more since it's quite big.
Reviewed By: mbouaziz
Differential Revision: D10509334
fbshipit-source-id: a81b36aa6
Summary: This should stop the bleeding until we get a better solution like shared memory + single writer process.
Reviewed By: mbouaziz
Differential Revision: D10868360
fbshipit-source-id: a4d0b064e
Summary: To avoid reporting on private methods, ignore those starting with underscore. Other cleanups.
Reviewed By: jvillard
Differential Revision: D10558970
fbshipit-source-id: 0572f1e70
Summary:
Invalidating addresses for destructors to catch use after destructor errors.
To pass ownership tests for use after destructor errors, we still need to:
(1) fix pointer arithmetic false positives
(2) add model for placement new to fix false positives
(3) add model for operator= to fix false positives
(4) support inter-procedural analysis for destructor_order_bad test
Reviewed By: jvillard
Differential Revision: D10450912
fbshipit-source-id: 2d9b1ee68
Summary:
It uses platform-dependent integer type widths information when
constructing Sizeof expressions which have a field(`nbytes`)
representing the static results of the evaluation of `sizeof(typ)`.
Reviewed By: mbouaziz
Differential Revision: D10504715
fbshipit-source-id: 0c79d37d8
Summary: Reports will now be issued for the class loads of the methods specified by the option `--class-loads-roots`.
Reviewed By: jvillard
Differential Revision: D10466492
fbshipit-source-id: 91456d723
Summary:
Instead of the non-sensical piecewise join we had until now write
a proper one. Hopefully the comments explain what it does. Main one:
```
(* high-level idea: maintain some union-find data structure to identify locations in one heap
with locations in the other heap. Build the initial join state as follows:
- equate all locations that correspond to identical variables in both stacks, eg joining
stacks {x=1} and {x=2} adds "1=2" to the unification.
- add all addresses reachable from stack variables to the join state heap
This gives us an abstract state that is the union of both abstract states, but more states
can still be made equal. For instance, if 1 points to 3 in the first heap and 2 points to 4
in the second heap and we deduced "1 = 2" from the stacks already (as in the example just
above) then we can deduce "3 = 4". Proceed in this fashion until no more equalities are
discovered, and return the abstract state where a canonical representative has been chosen
consistently for each equivalence class (this is what the union-find data structure gives
us). *)
```
Reviewed By: mbouaziz
Differential Revision: D10483978
fbshipit-source-id: f6ffd7528
Summary:
Instead of propagating a partial state give up the analysis of the
function entirely on error. The state after an error is mostly
non-sensical so until we know better just giving up makes sure the
analysis remains sensible and produce fewer spurious warnings.
Reviewed By: mbouaziz
Differential Revision: D10483979
fbshipit-source-id: 171ec8469
Summary: Since we only care about reachability, drop the interpreter and just fold over all instructions in the procdesc.
Reviewed By: mbouaziz
Differential Revision: D10461783
fbshipit-source-id: 3e0b42a48
Summary: We don't need the machinery of HIL, or its complexity for this analysis.
Reviewed By: ddino
Differential Revision: D10461641
fbshipit-source-id: 2e7d3ab8e
Summary: First version of an analyzer collecting classes transitively touched.
Reviewed By: mbouaziz
Differential Revision: D10448025
fbshipit-source-id: 0ddfefd46
Summary: Even though we recognize the lock/unlock methods of various classes in C++, to report we insist that the class must have a `mutex` member. Equalize the two sets of types recognized.
Reviewed By: da319
Differential Revision: D10446527
fbshipit-source-id: f42ae1a35
Summary:
It avoids checking integer overflow when it definitely cannot happen.
For example, it does not check integer overflow of addition when one
of parameters is a negative number, or underflow of subtraction when
its first parameter is a positive number.
Reviewed By: mbouaziz
Differential Revision: D10446161
fbshipit-source-id: b8c86e1b2
Summary: We assume multiplication of 1 is safe. It happens sometimes by multiplying `sizeof(char)`.
Reviewed By: mbouaziz
Differential Revision: D10444680
fbshipit-source-id: 2f33be280
Summary: This diff changes pp of binary operation condition in order to avoid a `make test` failure. For the same `uint64_t` type, it is translated to `unsigned long long` in 64bit mac, but `unsigned long` in 64bit linux, which made a `make test` failure.
Reviewed By: mbouaziz
Differential Revision: D10459466
fbshipit-source-id: 449ab548e
Summary:
`Location` was clashing with the `Location` module, so use `Address`
instead.
When invalidating an address, remember the "actor" of its invalidation,
i.e. the access expression leading to the address and the source
location of the corresponding instruction.
When checking accesses, also pass the actor responsible for the access,
so that when we raise an error we know:
1. when and why a location was invalidated
2. when and why we tried to read it after that
Reviewed By: mbouaziz
Differential Revision: D10446282
fbshipit-source-id: 3ca4fb3d4
Summary:
Model `x[y]` and `x.push_back(i)` to catch the classic bug of "take
reference inside vector, invalidate, then use again".
Reviewed By: da319
Differential Revision: D10445824
fbshipit-source-id: 21ffd9677
Summary:
Do the intersection of the heap and stack domains, and the union of the
invalid location sets. This forgets invalid locations that appear only
in one heap, unfortunately. We can start with this and improve later.
Reviewed By: mbouaziz
Differential Revision: D10445825
fbshipit-source-id: cc24460af
Summary:
It gets built-in integer type widths of C from the clang plugin. For Java, it uses fixed widths.
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D10397409
fbshipit-source-id: 73958742e
Summary:
Store the correct version of the proc desc into the DB when specialising
it. This doesn't seem to be used but is useful for investigating after
the fact (eg, if we could print individual cfgs).
Reviewed By: mbouaziz
Differential Revision: D10380708
fbshipit-source-id: fd72dbfc2
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:
In some error paths we may end up querying the state for the instruction
being executed, but that is only populated by biabduction. Now it's
populated by AI checkers too.
Reviewed By: jberdine
Differential Revision: D10381068
fbshipit-source-id: dca1325d7
Summary:
When the backend crashes we print which instruction/file/... we were analysing,
but because of recursion we can end up repeating that information all
the way to the toplevel call.
This makes sure we only print the innermost one, we don't care about the
calling context because the analysis is compositional.
Reviewed By: mbouaziz
Differential Revision: D10381141
fbshipit-source-id: 1c92bb861
Summary:
Trace events would crash when infer subprocesses were spawned by the build
system because they didn't detect if the file was already initialised
correctly.
Also trace the clang capture.
Reviewed By: mbouaziz
Differential Revision: D10380745
fbshipit-source-id: 76e1d4d7e
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:
Using debugging on uninit raised an exception. A file was opened twice and closed twice.
This happened because the two abstract interpreters (SIL, LowerHIL) conflicted.
Let's use the LowerHIL-AI directly
Reviewed By: jvillard
Differential Revision: D10126442
fbshipit-source-id: 113c9e131
Summary:
Load proc descs from the "procedures" sqlite table instead of from
file-wide cfgs stored in the "source_files" table. This removes the need
for a cache of these file-wide CFGs, which was needed because loading
them is expensive and potentially needed in case we need to load the
proc descs of several procedures in the same file. Now we can just load
the proc descs one by one and not worry about caching.
Reviewed By: jberdine
Differential Revision: D10173355
fbshipit-source-id: 665636121
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:
First step: record the proc desc of each procedure in the "procedures"
table. Update them according to the attributes logic. Bonus: this
proc-desc for a procedure is now always in sync with its attributes.
For now nothing uses these per-procedure cfgs. Later diffs make more and
more use of them and eventually kill off file-wide CFGs from the
database.
Reviewed By: jberdine
Differential Revision: D10173350
fbshipit-source-id: b6d222bee
Summary:
There's nothing to analyse for declared procedures, and if there is then
that's because they are defined outside the source file and should not
be analysed unless ondemand needs them.
Reviewed By: ngorogiannis
Differential Revision: D10173353
fbshipit-source-id: 39c42eb7a
Summary:
In a future commit `Attributes` will depend on `Procdesc` and that
creates a cycle for the functions concerned with specialising proc
descs, which need `Attributes`.
Reviewed By: jberdine
Differential Revision: D10173354
fbshipit-source-id: 6c4ff82f0
Summary: The Nullsafe checker integration is filtering out the pre-existing warnings based on the bug hash only. However, there was a typo in the regexp and the bug hash for methods in anonymous classes was then depending on the name (in the bytecode) of the anonymous class, i.e. depending on the `N` in `ClassName$N.methodName()` where `N` is the occurrence of the anonymous class in `ClassName`. As a consequence, introducing a new anonymous class in a file was leading to all the reports in the subsequent anonymous classes to be marked as introduced.
Reviewed By: jberdine
Differential Revision: D10186651
fbshipit-source-id: 42e27c132
Summary:
An order constraint (A,B) means we take lock A and before releasing it we perform B (whatever that is).
Previously if a method call crossed class boundaries, we removed the callee's order constraints before integrating the callee's summary to that of the caller. The reasoning was that this may lead to reports blaming a caller for something they are very far from, plus a proliferation of reports with the same bad endpoint.
The first reason still applies, but this is a general problem. It may be better to report and let developers deal with it.
The second reason is moot, since in differential mode most of these reports are hidden.
Reviewed By: jberdine
Differential Revision: D10173200
fbshipit-source-id: 9afbf292c
Summary:
Instead of many successive implicit transactions to write each
attributes of the procedures in a file, write them all in a single
transaction.
Reviewed By: jberdine
Differential Revision: D10173351
fbshipit-source-id: 5f2a5ffb5
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:
Before storing attributes to disk, we fix their location information if needed.
Ideally we wouldn't be creating bogus attributes but sometimes the frontends
are built in a way that makes it difficult to do otherwise, thus we have to
live with this. However, what's aggravating is that attributes are also saved
in the proc descs of these procedures but in their wrong version. This makes
the two versions (inside the procedures sqlite table and inside the procdesc in
the cfg of the source_files table) agree.
Reviewed By: jeremydubreil
Differential Revision: D10084708
fbshipit-source-id: 5bfd5da3a
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: Option is not needed, just set `default` record to agree with function default arguments.
Reviewed By: da319
Differential Revision: D10050463
fbshipit-source-id: e7d13bbd5
Summary:
The 2nd iteration of analysis of the Android core implementation did not yield actionable models, so delete those.
Turn on strict-mode reporting by default, when doing starvation analysis (which is disabled by default).
Reviewed By: jvillard
Differential Revision: D9991448
fbshipit-source-id: 67504591d
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: We may want to use these traces more generally, so put them into their own module.
Reviewed By: mbouaziz
Differential Revision: D10084404
fbshipit-source-id: 8f87c17f4
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:
Sometimes the default timeout of 10s is not enough(!). Make it
configurable while we work on not hitting it anyway.
Reviewed By: da319
Differential Revision: D10083772
fbshipit-source-id: ab949039f
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: If we get to that point, it means we already want to run the analysis so no need for this check.
Reviewed By: mbouaziz
Differential Revision: D9942702
fbshipit-source-id: e89e22c91
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: Use the value of other options instead since we're trying to get rid of it. This should be equivalent.
Reviewed By: jeremydubreil
Differential Revision: D9943274
fbshipit-source-id: 055e1bdd2
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:
This allows infer devs to see the effects their changes have on the infer manuals.
Check in the manuals for each subcommand + the output of `--help-full` to get a
complete picture. If this is too annoying we can also check in only
`--help-full`.
Reviewed By: mbouaziz
Differential Revision: D9916404
fbshipit-source-id: b981e2c33
Summary:
When a deprecated option is found in .inferconfig, we change it to `--<long>`
on the command line, but that string can be empty. Plumb things through so that
some non-empty string is selected in that case.
Reviewed By: mbouaziz
Differential Revision: D9989189
fbshipit-source-id: c0f46bca9
Summary:
We can fill the gaps in the trace now: they correspond to processes waiting on
pipes. This suggests a more efficient protocol would help perf, at least on the
small example I tried. Anyhow, it shows it's useful to trace pipe operations.
Some small gaps remain but they look like they could be explained by rounding errors.
Reviewed By: mbouaziz
Differential Revision: D9934437
fbshipit-source-id: 1d5f53a6d
Summary: They actually don't take very much time at all but it's good to know that they don't.
Reviewed By: mbouaziz
Differential Revision: D9832277
fbshipit-source-id: 7486fb40c
Summary: Use `PerfEvent` to record the execution time of individual checkers.
Reviewed By: jeremydubreil, mbouaziz
Differential Revision: D9832102
fbshipit-source-id: 678fca155
Summary:
This adds an option `--trace-events` that generates a Chrome trace event[1] to
quickly visualise the performance of infer.
Reviewed By: mbouaziz
Differential Revision: D9831599
fbshipit-source-id: 96a33c627
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:
Now we see which file/procedure/instruction is responsible for a crash in the
backend. Biabduction and eradicate not supported yet for the instruction-level
debug.
Reviewed By: mbouaziz, da319
Differential Revision: D9915666
fbshipit-source-id: 279472305
Summary:
Previously we wouldn't flush the formatter hence the error message would
generally not make it to the log file. Add the backtrace too, although only the
first few lines appear for some reason...
Reviewed By: ngorogiannis
Differential Revision: D9915499
fbshipit-source-id: 43cd9e36e
Summary: This fixes a flaky test where some issues would disappear and re-appear.
Reviewed By: da319
Differential Revision: D9027686
fbshipit-source-id: 5ac314096
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: This allows Eradicate to detect more issues related to inconsistent annotations with sub-typing.
Reviewed By: ngorogiannis
Differential Revision: D9807306
fbshipit-source-id: 159d5d4e8
Summary: Display the errors report as red in the termninal, warnings as yellow, advice as blue and the like as green.
Reviewed By: mbouaziz
Differential Revision: D9828302
fbshipit-source-id: 30315eac2
Summary:
First version of differential for costs, based on polynomial's degree's variation. The rule is very simple:
For a given polynomial that is available before and after a diff, `if degree_before > degree_after`, then the issue becomes `fixed`. Instead, `if degree_before < degree_after`, then the issue becomes `introduced`.
Reviewed By: ezgicicek
Differential Revision: D9810150
fbshipit-source-id: d08285926
Summary: Buck is allowing compiler commands with no source files and skipping them when using the in-memory complier mode. However, those commands are not skipped when using an external compiler. Simulating this behavior at the level of Infer.
Reviewed By: mbouaziz, ngorogiannis
Differential Revision: D9795043
fbshipit-source-id: e80cfa453
Summary: There may be several reasons why we think a method is on the UI thread. Choose to keep the shortest via join.
Reviewed By: jeremydubreil
Differential Revision: D9806944
fbshipit-source-id: 89d27456d
Summary:
Callsites of `Reporting.log_error/warning` always use `Exceptions.Checkers`, let's simplify the API.
Under the hood it still creates an exception, but this can be cleaned up later.
Reviewed By: jeremydubreil
Differential Revision: D9799860
fbshipit-source-id: 6492a60b4
Summary: This feature is not currently used and crashes when enabled.
Reviewed By: ngorogiannis
Differential Revision: D9805110
fbshipit-source-id: db405c79e
Summary:
For some unexplained reason, some of the functions registered in the Epilogues would sometimes be executed several times. I could not figure out why.
This diff fixes that, but also has more explainable benefits:
- Do not run epilogues registered in the parent in the children. Previously it
would do so, but probably only if the children registered some epilogue given
that `at_exit` must be called again once on the child (but the value of the ref
in `Pervasives` would not have been reset).
- Unified behaviour for early and late epilogues given that we now handle both of these directly
We already have all the control needed to run epilogues when needed: we know
when infer exits, and we know when children processes exit.
Reviewed By: mbouaziz
Differential Revision: D9752046
fbshipit-source-id: 13af40081
Summary:
The constructor `` `Typ`` is never used to build values. Removing type
substitutions from Sil.ml had knock-on effect on Typ.ml etc., resulting in more
deleted code around type substitutions \o/
Reviewed By: mbouaziz
Differential Revision: D9769340
fbshipit-source-id: 509cbd284
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:
It detaches the Summary module from BufferOverrunDomain.
Depends on D9194130
Reviewed By: jvillard
Differential Revision: D9194375
fbshipit-source-id: 30392b5ce
Summary: It simplifies instantiataion of `ret_alias`. While it got `ret_alias` values by iterating caller's and callee's memory, now it gets `ret_alias` by evaluating symbol paths included in location values.
Reviewed By: mbouaziz
Differential Revision: D9569606
fbshipit-source-id: a3326bb81
Summary: Now that the def file is stored in the issue type (hence in the issue desc), no need for it here any more.
Reviewed By: martinoluca
Differential Revision: D9654109
fbshipit-source-id: 0b3c413bf
Summary:
- Let's call `IssueType.from_string` once only
- Use properly defined issue types for builtin linters
Reviewed By: martinoluca
Differential Revision: D9654105
fbshipit-source-id: 947b50a51
Summary: This list is built once only, let's avoid exposing it.
Reviewed By: jeremydubreil
Differential Revision: D9654091
fbshipit-source-id: d92f91329
Summary:
Now that we got rid of dummy nodes used non-dummily (biabduction state, reporting), `pname` don't need to be an option anymore.
Let's save a boxing on all nodes.
Reviewed By: jeremydubreil
Differential Revision: D9654152
fbshipit-source-id: 83b00f239
Summary:
Using a dummy node here made the whole reporting wrong because it didn't fail getting a `node_key` when reporting issues from checkers not using the biabduction state.
Now that it's fixed, let's fail hard if someone ever tries again.
Reviewed By: jeremydubreil
Differential Revision: D9654137
fbshipit-source-id: c00273e53
Summary: No dummy node key, as a consequence the option `--skip-duplicated-types` will have no effect on issues with no node key, i.e. issues reported by non-biabduction non-eradicate checkers.
Reviewed By: martinoluca
Differential Revision: D9633564
fbshipit-source-id: 9ff8abf21
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:
- Was not used by the caller
- Gives smaller summaries
- Will allow adding a intra-proc info, e.g. `node` for reporting (not sure yet)
Reviewed By: skcho
Differential Revision: D9373763
fbshipit-source-id: 322001b53
Summary: The pattern matching could previously be missing some valid cases (in theory).
Reviewed By: mbouaziz, jberdine
Differential Revision: D9491441
fbshipit-source-id: 2bc1fc1aa
Summary:
In SIL, (1) some program variables (e.g., array parameter) are used as pointers to heap addresses and (2) the other program variables (e.g., local array) are used as addresses themselves. So, the values of (1) are retrieved by the `Load` command, while that of (2) are by `Exp.Lvar` expressions directly.
To address them differently, we had managed two maps (`Mem.Stack` and `Mem.Heap`), but which introduced function duplications on abstract memory and increased complexity. This diff merges the two maps, and instead a location set is used for distinguishing two types of abstract locations during analysis.
Reviewed By: mbouaziz
Differential Revision: D9420388
fbshipit-source-id: 13f824850
Summary: It is easier to filter out those reports in `.inferconfig` if we want them that modifying a boolean value ddirectly in the code
Reviewed By: mbouaziz
Differential Revision: D9494082
fbshipit-source-id: 9fb042313
Summary: There no clear alternative to using models of the standard library at this point so we can simplify the code a little bit
Reviewed By: mbouaziz
Differential Revision: D9491062
fbshipit-source-id: 9e5a6eeea
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:
Not all clang commands are happy with all arguments, but the driver is usually
the place we want to add arguments to.
Reviewed By: martinoluca
Differential Revision: D9421403
fbshipit-source-id: fa6d39a9b
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:
`Errlog` will merge similar issues (same severity, name, description) reported at the same location, so let's make sure the locaiton is mandatory.
Issues:
- errors happening in `Ondemand` still use the `State` which makes sense only for biabduction and eradicate
- a case of `NullabilitySuggest` didn't have a location, I did my best to patch it but I'm sure the location could be more precise
Reviewed By: jvillard
Differential Revision: D9332840
fbshipit-source-id: ee7898146
Summary: Keeping pushing arguments higher in the stack, `node_id_key` is not used in calls to `log_warning/error`
Reviewed By: jvillard
Differential Revision: D9332826
fbshipit-source-id: e5c48c686
Summary:
Before we would convert it to string in `Reporting` and pass it to `Errlog` which would use it only to 'log events'.
I guess the reason is that there was a cyclic dependency between `Errlog` and `clang_method_kind` defined in `ProcAttributes`.
This diff:
- moves it to its own module
- defers the conversion to string
Reviewed By: jvillard
Differential Revision: D9332819
fbshipit-source-id: 43a028b61
Summary:
- abstracted the type for a node key
- moved it to its own module with an ugly `compute` to avoid cyclic dependencies...
- renamed `node_id` to `node_id_key` where needed
- moved key computation from `State` to `Procdesc.Node`
Reviewed By: jvillard
Differential Revision: D9332803
fbshipit-source-id: fe1ae8c1c
Summary:
- made arguments of `Errlog.log_issue` mandatory
- pushed some arguments of `log_issue_from_errlog` higher in the stack, the goal is to make sure `State` is only used in analyses that update it (biabduction and eradicate, if I'm correct)
Reviewed By: jvillard
Differential Revision: D9332773
fbshipit-source-id: ce79df21c
Summary: The procedure id is likely to be trucated with random CRC values so we should probably not use it to match Java names
Reviewed By: mbouaziz
Differential Revision: D9183518
fbshipit-source-id: b450c7869
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:
Useful tips and tricks to debug infer's OCaml code. Also emit a developer
warning when the database is not initialised (since it's only expected to
happen when running infer from the toplevel).
Reviewed By: mbouaziz
Differential Revision: D9295782
fbshipit-source-id: 09b7b9a02
Summary:
To keep up with the times. Changes consist of new features and moving modules
around so shouldn't change anything on our side.
Depends on D9239803
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: da319
Differential Revision: D9239817
fbshipit-source-id: d02a2076a
Summary: This test was not re-run when the Java dependencies were changing
Reviewed By: mbouaziz
Differential Revision: D9238288
fbshipit-source-id: 65cc9c03c
Summary:
Use `ignore` instead, as this will warn if the argument is an arrow type,
unlike `let _ = ...`. This makes the code more future-proof: if an argument is
added to a function called in `let _ = f x` then the compiler will complain
instead of silently turning a value into a partial evaluation.
Also got rid of particularly irksome `let _ = <stuff returning unit> in` where I could.
Reviewed By: mbouaziz
Differential Revision: D9217176
fbshipit-source-id: 3be463405
Summary: It moves the functions that constructs array values from BufferOverrunSemantics to ArrayBlk and Val modules.
Reviewed By: mbouaziz
Differential Revision: D9194130
fbshipit-source-id: bf040a01a
Summary:
It removes the sizeof function because most of the cases on static types are addressed in the clang frontend.
Depends on D9193802
Reviewed By: mbouaziz
Differential Revision: D9213876
fbshipit-source-id: 0ce2f3749
Summary:
The internal concept of "kind" should in fact be named "severity" to match the convention used by many other tools, whereas the internal concept of "severity", i.e "HIGH", "MEDIUM" and "LOW" was never used and in any case redundant with the concept of "info", "warning", "error".
This diff maps both the "kind" and "severity" fields to value of the form "advice", "info", "warning", and "error" to be able to progressively migrate the code using the "kind" field.
Reviewed By: mbouaziz, jvillard
Differential Revision: D9187978
fbshipit-source-id: 447d89f51
Summary:
- changes the `Ondemand` callbacks to take the execution environment instead of a `get_proc_desc` function.
- removes all the cases passing `get_proc_desc` as parameter to use `Ondemand.get_proc_desc` instead.
Reviewed By: jvillard
Differential Revision: D9200583
fbshipit-source-id: d16c218b5
Summary:
Some paths are hardcoded in infer as being relative to the current executable,
for instance the directory where to find the models. By copying infertop.bc to
infer/bin like we do for `infer` these relative paths lead to the expected
place, which means models can be loaded in the toplevel like they would be in a
normal infer execution. This is more useful for debugging than previously.
Reviewed By: jeremydubreil, mbouaziz
Differential Revision: D9197142
fbshipit-source-id: 48c4f82fb
Summary:
Using the proc name directly should be equivalent and does not rely on
`Summary` caching summaries for us.
Reviewed By: mbouaziz
Differential Revision: D9196503
fbshipit-source-id: dea67999a
Summary:
We want to kill the cache in `Summary`, and calling `Summary.get` relies on
that cache existing for efficiency. However in this case it's not needed
because we can pass the summary from above instead.
Reviewed By: mbouaziz
Differential Revision: D9195234
fbshipit-source-id: 5b7023242
Summary:
Exposing the in-memory cache seems dangerous. There were only 2 uses anyway:
eradicate and biabduction. I think the biabduction one is safe to remove. The
eradicate one I do not really understand as we return a different summary than
the one we cache... the tests pass though.
Reviewed By: jeremydubreil
Differential Revision: D9150167
fbshipit-source-id: cf30af232
Summary:
This makes sure that the javac_jar is disabled when setting the external compiler option to point at the Infer wrapper
Closes#976
Reviewed By: jvillard
Differential Revision: D9193336
fbshipit-source-id: abafb51fc
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: This should be functionally equivalent but removes one call to `Summary.get`
Reviewed By: jvillard
Differential Revision: D9153924
fbshipit-source-id: d49789d2f
Summary:
Because of the custom formatting we do on top of `Cmdliner.Manpage` in
`CommandLineOption`, the long `$(b,strings inside bold block)` would sometimes
end up split across several lines and then `Cmdliner` complains about the line
ending before having seen the closing parenthesis `)`.
Instead of allowing these long strings, remove then from the help message as
they don't seem very useful to me. Replace them with just a mention of
`--test-determinator` so all the options related to Test Determinator are easy
to look up.
Also change the relevant options from `CLOpt.mk_string` to `CLOpt.mk_path`
because they look like they are supposed to be paths.
Reviewed By: ddino
Differential Revision: D9133505
fbshipit-source-id: d82acf5bf
Summary: This code is no longer necessary because the bug hash does not depend on the name of the anonymous classes
Reviewed By: mbouaziz
Differential Revision: D9176205
fbshipit-source-id: 9a8e9c9f8
Summary: It uses a SymbolPath map to Symbol in Inferbo's summary instead of an entry memory of callee, which is used for instantiations of the abstract memories on function calls.
Reviewed By: mbouaziz
Differential Revision: D9081631
fbshipit-source-id: 478cda0de
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:
Print the following for each source file to analyse in non-interactive mode:
```
path/to/source_file.c starting
[...]
path/to/source_file.c DONE in <time>
```
This should help diagnose when infer is stuck. It also logs this information to
the log file regardless of the form of the progress bar.
Also add a `--progress-bar-style` option to allow the user to force a
particular rendering: plain (as above), multiline (The Glorious One), or auto
(selection depends on whether infer is connected to a TTY on stdin *and*
stderr).
Reviewed By: mbouaziz
Differential Revision: D9120509
fbshipit-source-id: 4b43b7464
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:
The current message is confusing when the current class is marked UIThread.
See picture attached to task.
Reviewed By: jeremydubreil
Differential Revision: D8996593
fbshipit-source-id: cf52ee5d6
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: This avoid cases where the type definition is not found when trying to lookup the type definition for the class starting from the method name.
Reviewed By: ngorogiannis
Differential Revision: D9027983
fbshipit-source-id: 1add7692f
Summary: Added variant type for statement node to make it cleaner to match a particular statement node.
Reviewed By: mbouaziz
Differential Revision: D8997124
fbshipit-source-id: e19f6eacd
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:
`ANSITerminal` flushes after almost every operation. As a result, there is a
lot of flicker in the task bar. Going through `Format` instead reduces the
flicker.
Also erase end-of-lines when outputting logging stuff to the console. This makes sure the output is not garbled with stuff from the task bar.
Reviewed By: ngorogiannis
Differential Revision: D8931294
fbshipit-source-id: e89c87f84
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: This integration rolls out its own parallelisation using `Parmap` and its own error handling. Make it use `Tasks` instead.
Reviewed By: mbouaziz
Differential Revision: D8807109
fbshipit-source-id: ac09b3791
Summary: `IntLit.to_int` could raise, was not documented until recently and was not named `_exn`. Switch to option type and fix uses.
Reviewed By: jeremydubreil
Differential Revision: D8865525
fbshipit-source-id: f5ec2f221
Summary: `ownership_of_expr` is already doing some of the work in `propagate_return`. Also, split and move into abstract domain, where it belongs.
Reviewed By: jeremydubreil
Differential Revision: D8855257
fbshipit-source-id: 7756552d8
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
Sungkeun Cho
Ezgi Çiçek
Dino Distefano
Daiva Naudziuniene
Jules Villard
Mehdi Bouaziz
Nikos Gorogiannis
Jeremy Dubreil
Martin Trojer
Josh Berdine
Julian Sutherland
Vincent Siles
Thomas M. DuBuisson
dWiGhT
Martino Luca
Andrey Epin
Katie Ots