Summary:
Before this diff, the nullable checker would not be able to find annotations involving methods annotated in the protocols
update-submodule: facebook-clang-plugins
Reviewed By: sblackshear
Differential Revision: D6534893
fbshipit-source-id: 39bd3dd
Summary: This is to allow the bi-abduction analysis and the nullable checker for Clang languages to run together without stepping on each other toes.
Reviewed By: sblackshear
Differential Revision: D6567934
fbshipit-source-id: a318c33
Summary: There was a back and forth conversion between `string` and `IssueType.t` which was not necessary.
Reviewed By: sblackshear
Differential Revision: D6562747
fbshipit-source-id: 70b57a2
Summary: As Dulma pointed out, adding or removing paramters in a method in Objective C is changing the name of the method. Such changes should not make pre-exisiting issues reported as introduced. This diff is to prevent this by only keeping in the bug hash the part of the name that is before the first colon.
Reviewed By: dulmarod
Differential Revision: D6491215
fbshipit-source-id: 3c00fae
Summary:
Our model of unique_ptr and shared_ptr relied on the fact that we could C-style cast a pointer to the internal pointer type used in the smart pointer.
This is wrong when the smart pointer is used with a custom deleter that declares its own pointer type whose is not constructible from just a single pointer.
Reviewed By: dulmarod
Differential Revision: D6496203
fbshipit-source-id: 1305137
Summary: Local `CKComponentScope`'s are often created purely for their side effects, so it's fine for them to be unread.
Reviewed By: jeremydubreil
Differential Revision: D6475475
fbshipit-source-id: 17e869a
Summary: This would allow the checker to detect indirect nullable violations, i.e. violations that are involving intermediate method calls on potentially `nil` values.
Reviewed By: sblackshear
Differential Revision: D6464900
fbshipit-source-id: 3663729
Summary: NSDictionary initialization will crash when using `nil` as a key or as a value
Reviewed By: dulmarod
Differential Revision: D6466349
fbshipit-source-id: 57bb012
Summary: This will avoid collisions when the inner classes are implementing the same methods. For example, the previous version of the bug hash could conflate the issues when several annonymous inner classes are implementing the same method, e.g. a annonymous subclass of `Runnable` implementing `run()`.
Reviewed By: sblackshear
Differential Revision: D6461594
fbshipit-source-id: 2bb8545
Summary:
Simpler bug hash that is more independent of the underlying analysis. This now computes the hash based on:
- the bug type.
- the base filename: i.e for my/source/File.java, just keep File.java. So the hash will not change when moving files around.
- the simple method name: i.e. without package information and list of parameters. So changing the list of parameters will not affect the bug hash.
- the error message were the line numbers have been removed. So moving code or reformatting will not affect the hash.
Reviewed By: jberdine
Differential Revision: D6445639
fbshipit-source-id: 82e3cbe
Summary: To avoid false positives, we treat `operator[]` in cpp as container read. Moreover, if a container `c` is owned, we make all accesses `c[i]` to be also owned.
Reviewed By: sblackshear
Differential Revision: D6396574
fbshipit-source-id: 94aabff
Summary:
It seems that the abstraction instructions were not previously added the the CFG.
This is a functional changes to make sure that the abstraction state is always added. We can simplify the code later and just run this step before storing the CFG instead of after loading them.
Reviewed By: sblackshear, jvillard
Differential Revision: D6383672
fbshipit-source-id: cedcb8a
Summary:
As da319 points out, we did not handle this case correctly before. There were a few reasons why:
(1) An assignment like `struct S s = mk_s()` gets translated as `tmp = mk_s(); S(&s, tmp)`, so we didn't see the write to `s`.
(2) We counted uses of variables in destructors and dummy `_ = *s` assignments as reads, which meant that any struct values were considered as live.
This diff fixes these limitations so we can report on dead stores of struct values.
Reviewed By: da319
Differential Revision: D6327564
fbshipit-source-id: 2ead4be
Summary:
The diff is very big but it's mostly removing code. It was inspired by the fact that we were getting Dead Store FPs because we were modeling some functions from CoreFoundation and CoreGraphics directly as alloc in the frontend, which caused the parameters of the function to be seen as dead. See the new test.
To deal with this, if we are going to skip the function, we model it as malloc instead. Given how many models we had for those "model as malloc" functions, I removed them to rely solely on the new mechanism.
The modeling of malloc and release was still based on the old retain count implementation, even though all we do here is a malloc/free kind of analysis. I also changed
that to be actually malloc/free which removed many Assert false in the tests. CFRelease is not exactly free though, and it's possible to use the variable afterwards. So used a custom free builtin that only cares about removing the Memory attribute and focuses on minimizing Memory Leaks FPs.
Otherwise we were translating CFBridgingRelease as a special cast, and this wasn't working. To simplify this as well, I removed all the code for the special cast, and just modeled CFBridgingRelease and CFAutorelease also as free_cf, to avoid Memory Leak false positives. I also treated the cast __bridge_transfer as a free_cf model. This means we stopped trying to report Memory Leaks on those objects.
The modeling of CoreGraph release functions was done in the frontend, but seemed simpler to also simplify that code and model all the relevant functions.
Reviewed By: sblackshear
Differential Revision: D6397150
fbshipit-source-id: b1dc636
Summary:
- Plug model checkers
- Add alloc size safety condition on alloc of negative, zero or big size
Reviewed By: sblackshear
Differential Revision: D6375144
fbshipit-source-id: bbea6f3
Summary: This option was for compatibility with the command line options of the previous, but is no longer used. This diff removes the option and the deprecated code.
Reviewed By: sblackshear, mbouaziz
Differential Revision: D6351097
fbshipit-source-id: 0e4cfc5
Summary: Adding a null key or a null value will cause a runtime exception.
Reviewed By: sblackshear
Differential Revision: D6378618
fbshipit-source-id: 8bd27c6
Summary:
This resolves#796 . Effectively it adds file specific suffix to name of all global initializers (so initializersof two global variable of the same name will have unique Typ.Procname). which is the same rule as currently used by constructing Procname for the static functions. However this change applies to initializers of all global variables and not just static (arguably it's a right thing. since GCC used to allow multiple global variables with the same name).
Consequences of this change that it becomes impossible to know name of generated initialization function of global ('extern') variables. However get_initializer_pname function is only referenced by the frontend (when creating initializer for the defined global variables) and by the SIOF checker.
Closes https://github.com/facebook/infer/pull/801
Reviewed By: jvillard
Differential Revision: D6335034
Pulled By: dulmarod
fbshipit-source-id: 1a92c08
Summary: Adding a nil object to an NSArray will crash. Adding this case to the checker.
Reviewed By: sblackshear
Differential Revision: D6346241
fbshipit-source-id: 3fe6f20
Summary: The clang compiler introduces a materialized temporary expression which should be treated similarly to the Infer internal temporary variables.
Reviewed By: sblackshear
Differential Revision: D6331237
fbshipit-source-id: 81d8196
Summary:
We would previously skip any function that had one of these.
A no-op translation is sufficient to fix this issue (see new E2E test).
Reviewed By: mbouaziz
Differential Revision: D6317323
fbshipit-source-id: 0855bd8
Summary: Just changing ClangTrace to actually look at the different sanitizer kinds.
Reviewed By: jeremydubreil
Differential Revision: D6325086
fbshipit-source-id: 5da236d
Summary: In a thread safety report we used the access path from the final sink. This diffs change the report to include the expanded access path from the initial sink.
Reviewed By: sblackshear
Differential Revision: D6297848
fbshipit-source-id: 2386063
Summary:
This diff adds a new way of executing blocks when they are passed as parameters to a method. So far we just skipped the block in this case.
Now we can execute it. Let's demonstrate with an example. Say we have
//foo has a block parameter that it executes in its body
foo (Block block) { block();}
// bar calls foo with a concrete block
bar() {
foo (^(){
self->x = 10;
});
};
Now, when we call the method foo with a concrete block, we create a copy of foo instantiated with the concrete block, which in itself is translated as a method with a made-up name.
The copy of foo will get a name that is foo extended with the name of the block parameter, the call to the block parameter will be replaced to a call to the concrete block, and the captured variables
of the concrete block (self in this case), will be added to the formals of the specialized method foo_block_name.
This is turned on at the moment for ObjC methods with ObjC blocks as parameters, and called with concrete blocks. Later on we can extend it to other types of methods, and to C++ lambdas, that are handled similarly to blocks.
Another extension is to check when the block has been called with nil instead of an actual block, and raise an error in that case.
After this diff, we can also model various methods and functions from the standard library that take blocks as parameters, and remove frontend hacks to deal with that.
Reviewed By: ddino
Differential Revision: D6260792
fbshipit-source-id: 0b6f22e
Summary: The checker should not report unitinialzed values on the throw branch.
Reviewed By: ddino
Differential Revision: D6267019
fbshipit-source-id: 05768f1
Summary: We were conflating reads/writes with container reads/writes that created false positives.
Reviewed By: sblackshear
Differential Revision: D6232768
fbshipit-source-id: 39159cb
Summary: This is a hack to removes most of the false positives of this checker in Objective C.
Reviewed By: sblackshear
Differential Revision: D6239914
fbshipit-source-id: 1cf05de
Summary:
This confuses the SIOF checker and causes false positives. This dummy deref is
generated for constructors of classes that are modeled as being pointer types
instead of the actual class in infer, typically for smart pointers. I do not
understand how this works.
The biabduction also analyses this code, so might now get confused itself.
Reviewed By: jberdine
Differential Revision: D6221817
fbshipit-source-id: 050c5a9
Summary:
The issue is with classes defining static data members:
```
$ cat foo.h
struct A {
static int foo;
};
$ cat foo.cpp
#include "foo.h"
int A::foo = 12;
int f() { return A::foo; // should see A::foo as defined in this translation unit
$ cat bar.cpp
#include "foo.h"
void g() { return A::foo; // should see A::foo defined externally
```
Previously, both foo.cpp and bar.cpp would see `A::foo` as defined within their
translation unit, because it comes from the header. This is wrong, and static
data members should be treated as extern unless they're defined in the same
file.
This doesn't change much except for frontend tests. SIOF FP fix in the next diff.
update-submodule: facebook-clang-plugins
Reviewed By: da319
Differential Revision: D6221744
fbshipit-source-id: bef88fd
Summary: This only works for Java at the moment but we can re-organise the code later to add the Objective C equivalent of these assertion methods.
Reviewed By: mbouaziz
Differential Revision: D6230588
fbshipit-source-id: 46ee98e
Summary: The checker should not report nullable violations on repeated calls
Reviewed By: sblackshear
Differential Revision: D6195471
fbshipit-source-id: 16ff76d
Summary: The Java bytecode does not contain information about the location of abstract of interface methods. Before this diff, the analysis trace was tuncated and the file where the abstract or interface method was not included in the trace, which makes it harder to understand the Infer report, especially when the method is on a generated file that is not checked in the repository.
Reviewed By: sblackshear
Differential Revision: D6223612
fbshipit-source-id: c80c6f2
Summary: More general version of the fix in D6138749. This diff moves RacerD's lock modeling into a separate module and uses the module in the HIL translation to check when a function has lock/unlock semantics.
Reviewed By: jberdine, da319
Differential Revision: D6191886
fbshipit-source-id: 6e1fdc3
Summary: Functions that do not belong to a class or a struct are translated to c-style functions even in the context of cpp. We need to add ownership to locals for c-style functions too.
Reviewed By: sblackshear
Differential Revision: D6196882
fbshipit-source-id: 715f129
Summary:
vector::data returns a pointer to the first value of the vector.
- The size of the (array) pointer should be the same with the vector.
- The pointer should point to the same abstract value with the vector.
Reviewed By: mbouaziz
Differential Revision: D6196592
fbshipit-source-id: cc17096
Summary: `std::unique_lock` constructor allows to create a unique lock without locking the mutex. `std::unique_lock::try_lock` returns true if mutex has been acquired successfully, and false otherwise. It could be that an exception is being thrown while trying to acquire mutex, which is not modeled.
Reviewed By: jberdine
Differential Revision: D6185568
fbshipit-source-id: 192bf10
Summary:
Code often uses std::unique_lock::owns_lock to test if a deferred lock
using the 2-arg std::unique_lock constructor actually acquired the
lock.
Reviewed By: sblackshear
Differential Revision: D6181631
fbshipit-source-id: 11e9df2
Summary:
Use a distinct issue type for the Java and C++ concurrency analyses,
as the properties they are checking are significantly different.
Reviewed By: sblackshear
Differential Revision: D6151682
fbshipit-source-id: 00e00eb
Summary:
In a summary, you never want to see a trace where non-footprint sources flow to a sink.
Such a trace is useless because nothing the caller does can make more data flow into that sink.
Reviewed By: jeremydubreil
Differential Revision: D5779983
fbshipit-source-id: d06778a
Summary:
Due to limitations in our Buck integration, the thread-safety analysis cannot create a trace that bottoms out in a Buck target that is not a direct dependency of the current target.
These truncated traces are confusing and tough to act on.
Until we can address these limitations, let's avoid reporting on truncated traces.
Reviewed By: jeremydubreil
Differential Revision: D5969840
fbshipit-source-id: 877b9de
Summary:
:
Make both buck capture and compilation database handle buck command line arguments and invoke buck query the same way.
Plus allow:
- target patterns `//some/dir:` and `//some/dir/...`. However since `//some/dir:#flavor` and `//some/dir/...#flavor` are not supported, they need to be expanded before adding the infer flavor.
- target aliases (defined in `.buckconfig`)
- shortcuts `//some/dir` rewritten to `//some/dir:dir`
- relative path `some/dir:name` rewritten to `//some/dir:name`
Reviewed By: jvillard
Differential Revision: D5321087
fbshipit-source-id: 48876d4
Summary:
If you write
```
boolean readUnderLockOk() {
synchronized (mLock) {
return mField;
}
}
```
it will be turned into
```
lock()
irvar0 = mField
unlock()
return irvar0
```
in the bytecode. Since HIL eliminates reads/writes to temporaries, it will make the above code appear to perform a read of `mField` outside of the lock.
This diff fixes the problem by forcing HIL to perform all pending reads/writes before you exit a critical section.
Reviewed By: jberdine
Differential Revision: D6138749
fbshipit-source-id: e8ad9a0
Summary: In HIL, allow deref'ing a magic address like `0xdeadbeef` for debugging purposes. Previously, we would crash on code like this.
Reviewed By: mbouaziz
Differential Revision: D6143802
fbshipit-source-id: 4151924
Summary: This check is deprecated and will be replaced by a dedicated checker to detect unitialized values.
Reviewed By: mbouaziz
Differential Revision: D6133108
fbshipit-source-id: 1c0e9ac
Summary: Previously, this would incorrectly classify types like `map<std::string, int>` as a buffer
Reviewed By: mbouaziz
Differential Revision: D6125530
fbshipit-source-id: c8564de
Summary:
This is in the spec for clang compilation databases.
Also improves error messages when we fail to parse the compilation database.
closes#771
Reviewed By: dulmarod
Differential Revision: D6123832
fbshipit-source-id: 070f70f
Summary: In preparation for making `-a checkers` the default (when no analyzer is specified), let's test `-a checkers` by default.
Reviewed By: mbouaziz
Differential Revision: D6051177
fbshipit-source-id: d8ef611
Summary:
Refactor `RegisterCheckers` to give a record type to checkers instead of a tuple type.
Print active checkers with their per-language information.
Improve the manual entries slightly.
Reviewed By: sblackshear
Differential Revision: D6051167
fbshipit-source-id: 90bcb61
Summary:
Whenever we see a use of a lock, infer that the current method can run in a multithreaded context. But only report when there's a write under a lock that can be read or written without synchronization elsewhere.
For now, we only infer this based on the direct usage of a lock; we don't assume a caller runs in a multithreaded context just because its (transitive) callee can.
We can work on that trickier case later, and we can work on smarter inference that takes reads under sync into account. But for now, warning on unprotected writes of reads that occur under sync appears to be too noisy.
Reviewed By: jberdine
Differential Revision: D5918801
fbshipit-source-id: 2450cf2
Summary: This commit adds unsigned symbol for preciser analysis results with less number of uses of min/max operators.
Reviewed By: mbouaziz
Differential Revision: D6040437
fbshipit-source-id: 999ca4c
Summary:
This was a crutch from the days before ownership analysis.
We shouldn't need it anymore, and it was actually causing FP's because we were skipping analysis of `ImmutableList.builder()` and not understanding that the return value is owned.
Reviewed By: jeremydubreil
Differential Revision: D6035631
fbshipit-source-id: afa0ade
Summary:
This generates `--resource-leak-only` automatically, and make the other
checkers' `-only` option work as expected with respect to `--resource-leak` too
(eg, `--resource-leak --biabduction-only` disables resource leak).
Reviewed By: jeremydubreil
Differential Revision: D6051134
fbshipit-source-id: 2d4a2ba
Summary:
1. Mark some Makefile targets as depending on `MAKEFILE_LIST` so they get rebuilt on Makefile changes
2. Do not show boolean options with no documentation in the man pages (like we do for other option types).
3. Default to Lazy dynamic dispatch for the checkers.
4. In the tests, use `--<checker>-only` instead of relying on `--no-default-checkers`
5. `--no-filtering` is redundant if `--debug-exceptions` is passed
Reviewed By: jeremydubreil
Differential Revision: D6030578
fbshipit-source-id: 3320f0a
Summary: We will then be able to merge the tests for the other checkers without affecting these lab tests
Reviewed By: jvillard
Differential Revision: D6039433
fbshipit-source-id: e575ce9
Summary:
Another step toward running the biabduction analysis as a checker.
Depends on D6038210
Reviewed By: jvillard
Differential Revision: D6038682
fbshipit-source-id: fed45bf
Summary: Stack-allocated variables cannot be raced on in cpp as every thread has its own stack. At the beginning of the analysis we add ownership to the local variables.
Reviewed By: jberdine
Differential Revision: D6020506
fbshipit-source-id: 0a90a97
Summary: Now that we report write-write races involving more than one write, we need to improve the traces accordingly.
Reviewed By: jberdine
Differential Revision: D6026845
fbshipit-source-id: b1366dd
Summary:
Next step to issue deduplication: do not keep safety conditions that are subsumed by others.
Only do it if they do not have infinite bound: replacing `0 < size` by `1 < size` is ok, but replacing it by `+oo < size` is not because it looks much more like a lack of precision.
Reviewed By: skcho
Differential Revision: D5978455
fbshipit-source-id: acc2384
Summary:
A specific type of alias is added for the vector::empty() result and it is used at pruning.
Now, there are two types of aliases:
- "simple" alias: x=y
- "empty" alias: x=v.empty() and y=v.size
So, if x!=0, y is pruned by (y=0). Otherwise, i.e., x==0, y is pruned by (y>=1).
Reviewed By: mbouaziz
Differential Revision: D6004968
fbshipit-source-id: bb8d50d
Summary:
Attempting to translate these will not go well as the declaration still depends
on some template arguments. Added a test that was previously crashing the
frontend.
Also extend the catching of "Unimplemented" and other errors to `translate_one_decl` as it was useful to debug this issue. In particular, reraise all exceptions and log some additional context when doing so.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5976357
fbshipit-source-id: fca8e38
Summary:
Previously, annotating something ThreadSafe meant "check that it is safe to run all of this procedure's methods in parallel with each other" (including self-parallelization).
This makes sense, but it means that if the user writes no annotations, we do no checking.
I'm moving toward a model of inferring when an access might happen on a thread that can run concurrently with other threads, then automatically checking that it is thread-safe w.r.t to all other accesses to the same memory (on or off the current thread thread).
This will let us report even when there are no `ThreadSafe` annotations.
Any method that is known to run on a new thread (e.g., `Runnable.run`) will be modeled as running on a thread that can run in parallel with other threads, and so will any method that is `synchronized` or acquires a lock.
In this setup, adding `ThreadSafe` to a method just means: "assume that the current method can run in parallel with any thread, including another thread that includes a different invocation of the same method (a self race) unless you see evidence to the contrary" (e.g., calling `assertMainThread` or annotating with `UiThread`).
The key step in this diff is changing the threads domain to abstract *what threads the current thread may run in parallel with* rather than *what the current thread* is. This makes things much simpler.
Reviewed By: jberdine
Differential Revision: D5895242
fbshipit-source-id: 2e23d1e
Summary:
Indicate if read or write is protected, and do not print only the
field but also the object involved in the race.
Reviewed By: sblackshear
Differential Revision: D5974250
fbshipit-source-id: 351a576
Summary:
Move Inferbo safety conditions to their own file.
Split the old `Condition.t` to a condition together with a trace.
This will ease having: different kind of condition and several traces for the same condition (see following diff)
Reviewed By: jvillard
Differential Revision: D5942030
fbshipit-source-id: d74a612
Summary:
Inject a marker using a global variable in <iostream>, and whitelist it so that
the frontend translates it.
Use the marker in the SIOF checker to tell whether a file includes <iostream>.
If so, start the analysis of its methods assuming that the standard streams are
initialised.
Reviewed By: sblackshear
Differential Revision: D5941343
fbshipit-source-id: 3388d55
Summary:
The previous domain for SIOF was duplicating some work with the generic Trace
domain, and basically was a bit confused and confusing. A sink was a set of
global accesses, and a state contains a set of sinks. Then the checker has to
needlessly jump through hoops to normalize this set of sets of accesses into a
set of accesses.
The new domain has one sink = one access, as suggested by sblackshear. This simplifies
a few things, and makes the dedup logic much easier: just grab the first report
of the list of reports for a function.
We only report on the fake procedures generated to initialise a global, and the
filtering means that we keep only one report per global.
Reviewed By: sblackshear
Differential Revision: D5932138
fbshipit-source-id: acb7285
Summary:
This adds more structure to the SQL schema backing attributes. With that, we
can transfer the logic for updating attributes in SQLite, instead of doing
optimistic concurrency in the client.
Reviewed By: jberdine
Differential Revision: D5891038
fbshipit-source-id: 6577ba2
Summary: The case of closures was not considered for the convertion of SIL instructions into HIL instructions
Reviewed By: sblackshear
Differential Revision: D5929675
fbshipit-source-id: bb6920a
Summary: The tests are slower when running in debug mode, and it creates a lot of html outputs
Reviewed By: sblackshear
Differential Revision: D5916511
fbshipit-source-id: 07c90b7
Summary:
This diff does two things:
# Infer no longer add the contrains that the return value of a skip function is never null. This was leading to false negatives and is not necessary as those return value are treated angelically
# Infer now support `Nonnull` on the return value of skip functions.
Reviewed By: jberdine, sblackshear
Differential Revision: D5840324
fbshipit-source-id: bbd8d82
Summary: Example of combination between annotating fields with nullable and the biabduction analysis in Objective C
Reviewed By: dulmarod
Differential Revision: D5906016
fbshipit-source-id: b95c6e0
Summary:
The interval bound of the abstract domain is extended.
`[min|max](int, symbol)` => `int [+|-] [min|max](int, symbol)`
As a result, `vector::empty` can be modelled to return a more precise value: `1 - min(1, size)`.
Reviewed By: mbouaziz
Differential Revision: D5899404
fbshipit-source-id: c8c3f49
Summary:
We take it into account to not report bugs inside the available block. This requires a plugin change.
update-submodule: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D5891511
fbshipit-source-id: 21a02ad
Summary:
With this change and the previous facebook-clang-plugins change, infer no
longer exhausts the biniou buffer when reading the serialized C++ AST.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5891081
fbshipit-source-id: cf48eac
Summary: Infer can then detect the resource leak when resources are stored into a container
Reviewed By: sblackshear
Differential Revision: D5887702
fbshipit-source-id: 6106cfb
Summary: The point of the tracing mode is to compute all the possible path leading to an error state. However, within a method, many of those paths are not feasibile in practice. This leads to many false alarms for the resource leak analysis.
Reviewed By: sblackshear
Differential Revision: D5888695
fbshipit-source-id: 2dbc57b
Summary: Model the `remove(...)` and the `clear()` method of `HashMap`.
Reviewed By: sblackshear
Differential Revision: D5887674
fbshipit-source-id: c3f40ee
Summary: Handling the utility functions for asserting that we're on background thread.
Reviewed By: jberdine
Differential Revision: D5863435
fbshipit-source-id: 3ad95b5
Summary:
Previously, we just tracked a boolean representing whether we were possibly on the main thread (true) or definitely not on the main thread (false).
In order to start supporting `Thread.start`, `Runnable.run`, etc., we'll need something more expressive.
This diff introduces a lattice:
```
Any
/ \
Main Background
\ /
Unknown
```
as the new threads domain. The initial value is `Unknown`, and we introduce `Main` in situations where we would have introduced `true` before.
This (mostly) preserves behavior: the main difference is that before code like
```
if (*) {
assertMainThread()
} else {
x.f = ...
}
```
would have recorded that the access to `x.f` was on the main thread, whereas now we'll say that it's on an unknown thread.
Reviewed By: peterogithub
Differential Revision: D5860256
fbshipit-source-id: efee330
Summary:
It's useful to be able to disable de-duplication on the command line with `--no-filtering`.
Gate de-duplication with `Config.filtering` and move the de-duplication tests to a new directory under the build systems tests.
Reviewed By: jeremydubreil
Differential Revision: D5865329
fbshipit-source-id: 5094f5b
Summary:
Since D5381239, infer is careful not to delete directories that do not "look
like" results directories on startup, in case the user passed, eg, `-o /`.
In our repo, lots of results dir are created by build/test of infer, and when
the version of infer changes and the expected contents of results directories
change then it might start refusing to delete the results directories created
with another version of infer.
Add an option to force infer to delete the results directory no matter how
dodgy it looks, and use it in our repo by adding the option in every
.inferconfig.
Reviewed By: mbouaziz
Differential Revision: D5870984
fbshipit-source-id: 09412de
Summary: Will then be easier to understand if some changes in the test results is legit or not.
Reviewed By: sblackshear
Differential Revision: D5863961
fbshipit-source-id: 7eb3f33
Summary:
Suggesting to add `_Nullable` on the fields checked for, or assigned to, `nullptr` will allow the biabduction analysis to report null dereferences that are related to the lifetime of objects.
Depends on D5832147
Reviewed By: sblackshear
Differential Revision: D5836538
fbshipit-source-id: c1b8e48
Summary: The prune nodes where translated as `prune (expr = false)` and `prune ( expr != false)`. This case is a bit tricky to deconstruct in HIL. This diff translates the prune instructions as just `prune !expr` for the true branch and `prune expr` for the false branch.
Reviewed By: dulmarod
Differential Revision: D5832147
fbshipit-source-id: 2c3502d
Summary:
We need to make sure that destructors of virtual base classes are called only once. Similarly to what clang does, we have two destructors for a class: a destructor wrapper and an inner destructor.
Destructor wrapper is called from outside, i.e., when variables go out of scope or when destructors of fields are being called.
Destructor wrappers have calls to inner destructors of all virtual base classes in the inheritance as their bodies.
Inner destructors have destructor bodies and calls to destructor wrappers of fields and inner destructors of non-virtual base classes.
Reviewed By: dulmarod
Differential Revision: D5834555
fbshipit-source-id: 51db238
Summary:
The "placement new" operator `new (e) T` constructs a `T` in the pre-allocated memory address `e`.
We weren't translating the `e` part, which was leading to false positives in the dead store analysis.
Reviewed By: dulmarod
Differential Revision: D5814191
fbshipit-source-id: 05c6fa9
Summary:
Simple instance of the problem: analyzing the following program times out.
```
#include <tuple>
void foo() {
std::tuple<std::tuple<int>> x;
}
```
Replacing `std::tuple<std::tuple<int>>` by `std::tuple<int>` makes the analysis
terminate.
In the AST, both tuple<tuple<int>> and tuple<int> have the same template
specialization type: "Pack" (which means we're supposed to go look into the
arguments of the template to get their values). This is not information enough
and that's the plugin fault.
On the backend side, this means that two types have the same Typ.Name.t, namely
"std::tuple<_>", so they collide in the tenv. The definition of
tuple<tuple<int>> is the one making it into the tenv. One of the fields of the
corresponding CxxRecord is of type "tuple<int>", which we see as the same
"tuple<_>", which causes the loop.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5775840
fbshipit-source-id: 0528604
Summary:
Sort the complete set of warnings by everything except procname, then de-duplicate.
This scheme prevents reporting identical error messages on the same line/same file.
This is important for avoiding duplicate reports on multiple instantiations of the same template.
Reviewed By: jberdine
Differential Revision: D5819467
fbshipit-source-id: 984f47f
Summary: The resolution was previously only happening for constructors, but calls to private methods or to `super` are also neither static calls nor virtual calls. In this case, the resolution logic should be the same as for constructors.
Reviewed By: sblackshear
Differential Revision: D5830376
fbshipit-source-id: 9b56f80
Summary: Destroying local variables that are out of scope after `continue`.
Reviewed By: jberdine
Differential Revision: D5804120
fbshipit-source-id: 638cff5
Summary: Destroying local variables that are out of scope after `break`.
Reviewed By: jberdine
Differential Revision: D5764647
fbshipit-source-id: a7e06ae
Summary: The successor node of `continue` was not correct inside the `do while`.
Reviewed By: sblackshear
Differential Revision: D5769578
fbshipit-source-id: d7b0843
Summary: With this diff, the analysis trace will jump to the definition of the skipped methods when the location is known. This is especially useful when the analysis is relying on the method annotations.
Reviewed By: sblackshear
Differential Revision: D5783428
fbshipit-source-id: 561b739
Summary:
We supported globals as sources before, but we did so by allowing ClangTrace etc. to match against any access path in the footprint of the trace.
This is very powerful/flexible, but it's ultimately not a good idea because it leads to traces that are hard to read.
This is because a footprint source doesn't have any information about its provenance: we might know that the value came from a global, but we don't know where the read occurred.
The mechanism for handling procedure calls as sources already knows how to solve this problem.
This diff implements globals as sources as a special case of procedure call sources instead.
This will give us much nicer traces with full provenance of the read from the global.
Reviewed By: mbouaziz
Differential Revision: D5772299
fbshipit-source-id: 491ae81
Summary: The list of fields of a Java object in SIL is the list of fields declared in the class plus all the fields declared in the the super classes. It turns out that we were missing the fields declared in the implemented interfaces.
Reviewed By: sblackshear
Differential Revision: D5720386
fbshipit-source-id: d65c9de
Summary:
When a lambda has an `auto` parameter, the inferred type of the parameter because part of the name.
Our heuristic for identifying lambda was checking if the lambda's name was exactly `operator()`, which won't catch this case.
Reviewed By: jvillard
Differential Revision: D5753323
fbshipit-source-id: 85ff75a
Summary:
Not translating these properly was causing false positives for the dead store analysis in cases like
```
int i = 0;
return [j = i]() { return j; }();
```
Reviewed By: da319
Differential Revision: D5731562
fbshipit-source-id: ae79ac8
Summary: We inject destructor calls of base classes inside destructor bodies after the destructor calls of fields.
Reviewed By: dulmarod
Differential Revision: D5745499
fbshipit-source-id: 90745ec
Summary: We used to crash whenever we hit these. The simple translation implemented here is not particularly inspiring, but it is better than crashing.
Reviewed By: jvillard
Differential Revision: D5702095
fbshipit-source-id: 3795d43
Summary: This adds an option to only translate the body of a method when the file matches the give pattern. This is especially intended to be use for generated files.
Reviewed By: jvillard
Differential Revision: D5729120
fbshipit-source-id: 1e28469
Summary: With this, we can now get now get inter-procedural issues involving native methods.
Reviewed By: sblackshear
Differential Revision: D5730638
fbshipit-source-id: 3bdbdbd
Summary: Atoms of the form `identifier = footprint var` naturally occurs with the angelic analysis mode. So it is not clear to me why we should drop those.
Reviewed By: sblackshear
Differential Revision: D5654754
fbshipit-source-id: 9dd2eb5
Summary: This makes the traces more readable when involving skipped functions.
Reviewed By: sblackshear
Differential Revision: D5731683
fbshipit-source-id: 49d363b
Summary: This new tests outlines that Infer does not detect inter-target issues involving native methods.
Reviewed By: sblackshear
Differential Revision: D5720873
fbshipit-source-id: cce8193
Summary:
This simplifies the jbuild files: no need to list these files explicitly
anymore, nor to exclude them explicitly from the main `InferModules` library
(due to their different compilation flags).
Isolate common parts into jbuild.common do `cat`-based code inclusion into
jbuild files to factorize code.
Reviewed By: jberdine
Differential Revision: D5678328
fbshipit-source-id: 6d7d925
Summary:
A function can both be a sink and propagate source info, but we currently ignore the summary for any function that is also a sink.
This will cause us to under-report for (e.g.) `src1 = source(); src2 = strcpy(dest, src1); exec(src2)`.
This is both a potential buffer overflow and a potential shell injection, but we won't report the second issue.
Reviewed By: jberdine
Differential Revision: D5676167
fbshipit-source-id: 232ab2f
Summary:
In looking at summaries that Quandary took a long time to compute, one thing I notice frequently is redundancy in the footprint sources (e.g., I might see `Footprint(x), Footprint(x.f), Footprint(x*)`).
`sudo perf top` indicates that joining big sets of sources is a major performance bottleneck, and a large number of footprint sources is surely a big part of this (since we expect the number of non-footprint sources to be small).
This diff addresses the redundancy issue by using a more complex representation for a set of sources. The "known" sources are still in a set, but the footprint sources are now represented as a set of access paths (via an access trie).
The access path trie is a minimal representation of a set of access paths, so it would represent the example above as a simple `x*`.
This should make join/widen/<= faster and improve performance
Reviewed By: jberdine
Differential Revision: D5663980
fbshipit-source-id: 9fb66f8
Summary:
The previous widening operator added stars to the *end* of paths that existed in `next` but not `prev`. This is not enough to ensure termination in the case where the trie is growing both deeper and wider at the same time.
The newly added test demonstrates this issue. In the code, there's an ever-growing path of the form `tmp.prev.next.prev.next...` that wasn't summarized by the previous widening operator. The new widening is much more aggressive: it replaces *any* node present in `next` but not `prev` with a `*` (rather than trying to tack a star onto the end). This fixes the issue.
This issue was causing divergence on tricky doubly-linked list code in prod.
Reviewed By: jeremydubreil
Differential Revision: D5665719
fbshipit-source-id: 1310a92
Summary:
This is a check for when an unavailable class is being allocated.
This diff also adds a check for the context to remove false positives: If the class is not available but the method calls are wrapped in a check whether the class is available, then don't report.
Reviewed By: jvillard
Differential Revision: D5631191
fbshipit-source-id: 2082dfe
Summary:
Calling exit at the end of a proc does not create unreachable code, prior to this commit inferbo reports that it does.
We extend collect_instrs to detect when we're at the end of a procedure in C and not report on unreachable code if we call a procedure there.
Reviewed By: mbouaziz
Differential Revision: D5623637
fbshipit-source-id: 0d5f326
Summary: This check is not possible in Java as it natirally happens in the totally legit case of the `try ... finally`.
Reviewed By: sblackshear
Differential Revision: D5568802
fbshipit-source-id: 24ca074
Summary:
Instead of a whitelist and blacklist and default issue types and default
blacklist and filtering, consider a simpler semantics where
1. checkers can be individually turned on or off on the command line
2. most checkers are on by default
3. `--no-filtering` turns all issue types on, but they can then be turned off again by further arguments
This provides a more flexible CLI and is similar to other options in the infer
CLI, where "global" behaviour is generally avoided.
Dynamically created checkers (eg, AL linters) cause some complications in the
implementation but I think the semantics is still clear.
Also change the name of the option to mention "issue types" instead of
"checks", since the latter can be confused with "checkers".
Reviewed By: jberdine
Differential Revision: D5583238
fbshipit-source-id: 21de476
Summary:
The `-index-store-path` argument does not exist in Clang 5.0 and it gets discarded without any error message. The problem is that its argument, a folder, is not discarded, and Clang considers it as a source file. This leads to the following errors:
- `cannot specify -o when generating multiple output files` (this happens if a `-o` argument is passed)
- `error reading '<PATH>'` (this can be observed when running the "normalized" version of the clang command, generated via the -### flag)
This weird case can be observed when `-index-store-path` is passed in a sequence like the following: `-x c -index-store-path <PATH> -c`.
With this change, we remove the `index-store-path` option, and its argument, from the original clang command.
Reviewed By: jvillard
Differential Revision: D5601808
fbshipit-source-id: 4200308
Summary:
We previously lumped ownership predicates in with all other predicates. That limited us to a flat ownership domain.
This diff separates out the ownership predicates so we can have a richer lattice of predicates with each access path.
This lets us be more precise; for example, we can now show that
```
needToOwnBothParams(Obj o1, Obj o2) {
Obj alias;
if (*) { alias = o1; } else { alias = o2; }
alias.f = ... // both o1 and o2 need to be owned for this to be safe
}
void ownBothParamsOk() {
needToOwnBothParams(new Obj(), new Obj()); // ok, would have complained before
}
```
is safe.
Reviewed By: jberdine
Differential Revision: D5589898
fbshipit-source-id: 9606a46
Summary:
This makes it easier to test a single checker.
Also refactor the code to make it harder to mess up the list of default/all checkers.
Reviewed By: sblackshear
Differential Revision: D5583209
fbshipit-source-id: 7c919b2
Summary:
Use jbuilder to build infer instead of ocamlbuild. This is mainly to get faster builds:
```
times in 10ms, ±differences measured in speedups, 4 cores
| | ocb total | jb | ±total | ocb user | jb | ±user | ocb cpu | jb | ±cpu | ocb sys | jb | ±sys |
|-----------------------------------+-----------+------+--------+----------+------+-------+---------+-----+------+---------+------+------|
| byte from scratch | 6428 | 2456 | 2.62 | 7743 | 6662 | 1.16 | 138 | 331 | 2.40 | 1184 | 1477 | 0.80 |
| native from scratch | 9841 | 4289 | 2.29 | 9530 | 8834 | 1.08 | 110 | 245 | 2.23 | 1373 | 1712 | 0.80 |
| byte after native | 29578 | 1602 | 18.46 | 4514 | 4640 | 0.97 | 170 | 325 | 1.91 | 543 | 576 | 0.94 |
| change infer.ml byte | 344 | 282 | 1.22 | 292 | 215 | 1.36 | 96 | 99 | 1.03 | 040 | 066 | 0.61 |
| change infer.ml native | 837 | 223 | 3.75 | 789 | 174 | 4.53 | 98 | 99 | 1.01 | 036 | 47 | 0.77 |
| change Config.ml byte | 451 | 339 | 1.33 | 382 | 336 | 1.14 | 97 | 122 | 1.26 | 056 | 80 | 0.70 |
| change Config.ml native | 4024 | 1760 | 2.29 | 4585 | 4225 | 1.09 | 127 | 276 | 2.17 | 559 | 644 | 0.87 |
| change cFrontend_config.ml byte | 348 | 643 | 0.54 | 297 | 330 | 0.90 | 96 | 67 | 0.70 | 038 | 102 | 0.37 |
| change cFrontend_config.ml native | 1480 | 584 | 2.53 | 1435 | 906 | 1.58 | 106 | 185 | 1.75 | 136 | 178 | 0.76 |
#+TBLFM: $4=$2/$3;f2::$7=$5/$6;f2::$10=$9/$8;f2::$13=$11/$12;f2
50 cores
| | ocb total | jb | ±total | ocb user | jb | ±user | ocb cpu | jb | ±cpu | ocb sys | jb | ±sys |
|---------------------+-----------+------+--------+----------+------+-------+---------+----+------+---------+------+------|
| byte from scratch | 9114 | 2061 | 4.42 | 9334 | 5133 | 1.82 | | | 0/0 | 2566 | 1726 | 1.49 |
| native from scratch | 13481 | 3967 | 3.40 | 12291 | 7608 | 1.62 | | | 0/0 | 3003 | 2100 | 1.43 |
| byte after native | 3467 | 1476 | 2.35 | 5067 | 3912 | 1.30 | | | 0/0 | 971 | 801 | 1.21 |
#+TBLFM: $4=$2/$3;f2::$7=$5/$6;f2::$10=$9/$8;f2::$13=$11/$12;f2
```
Menu:
1. Write a jbuild file, autogenerated from jbuild.in because we need to fill in
some information at build-time (really, at configure time, but TODO), such as
whether or not clang is enabled.
2. Nuke lots of stuff from infer/src/Makefile that is now in the jbuild file
3. The jbuild file lives in infer/src/ so it can see all the sources. If we put it somewhere else, eg, infer/, then `jbuilder` scans too many files (all irrelevant) and takes 2.5s to start instead of .8s. Adding irrelevant directories to jbuild-ignore does not help.
4. jbuilder does not support subdirectories, so resort to listing all the
source files in the generated jbuild (only source directories need to be
manually listed in jbuild.in though). Still, the generated .merlin is wrong
and makes merlin find source files in _build, so manually tune it to get
good merlin support. We also lose some of merlin for unit tests as it
cannot see their build artefacts anymore.
5. checkCopyright gets its own jbuild because it's standalone. Also, remove
some deprecation warnings in checkCopyright due to the new version of Core from
a while ago.
6. Drop less-used Makefile features (they had regressed anyway) such as
building individual modules. Also, building mod_dep.pdf now takes all the
source files available so they better build (before, it would only take the
source files from the config, eg with or without clang) (that's pretty minor).
7. The toplevel is now built as a custom toplevel because that was easier. It
should soon be even easier: https://github.com/janestreet/jbuilder/issues/210
8. Move BUILTINS.mli to BUILTINS.ml because jbuilder is not happy about
interface files without implementations.
In particular, I did not try to migrate too much of the Makefile logic to jbuilder,
more can be done in the future.
Reviewed By: jberdine
Differential Revision: D5573661
fbshipit-source-id: 4ca6d8f
Summary:
Previous version was hard to understand because it was doing many things within same code. New version has different code for Arrays, Structs and others.
There is some copy-paste, but it's easier to follow code (open to suggestions though)
Reviewed By: dulmarod
Differential Revision: D5547999
fbshipit-source-id: 77ecb24
Summary: It wasn't using code from `std::vector::empty` which recently was improved. Instead of inlining `std::vector::empty`, call it to know whether vector is empty or not.
Reviewed By: jvillard
Differential Revision: D5573379
fbshipit-source-id: e024a42
Summary: Useful for identifying user-controlled array accesses that could lead to buffer overflows
Reviewed By: mbouaziz
Differential Revision: D5520985
fbshipit-source-id: 92984f6
Summary:
When a sink name is specified in `.inferconfig` or in OCaml, it might conflict with a function of the same name that has a different number of args.
We shouldn't try to create a sink in this case, and we definitely shouldn't crash.
Reviewed By: jeremydubreil
Differential Revision: D5561216
fbshipit-source-id: fa1859b
Summary:
Replace `inferTraceBugs` with `infer-explore` with a similar CLI. Some options changed:
- --max-level -> --max-nesting, and "max" is the default value instead of a possible value
- --no-source -> --no-source-preview
Reviewed By: mbouaziz
Differential Revision: D5526651
fbshipit-source-id: 8383f37
Summary:
In some cases we normalize expressions to check some facts about them. In these
cases, trying to keep as much information as possible in the expression, such
as the fact it comes from a `sizeof()` expression, is not needed. Doing
destructive normalization allows us to replace `sizeof()` by its
statically-known value.
closes#706
Reviewed By: mbouaziz
Differential Revision: D5536685
fbshipit-source-id: cc3d731
Summary: That was too noisy. Propagate `--quiet` to the Python reporting hook so as to still emit bugs.txt and so on.
Reviewed By: martinoluca
Differential Revision: D5501106
fbshipit-source-id: 63b6451
Summary:
This allows the user to specify a different command to build the current version of the project vs to build the previous version of the project. Here's an example:
```
infer diff --gen-previous-build-command-script "echo clang -c hello.c" ... -- clang -c hello2.c
```
By default the two build commands are the same. If the script is to be used for both the current and the previous versions, then it's on the user to run it once first, eg:
```
infer diff --gen-previous-build-command-script "./myscript.sh previous" ... -- $(./myscript.sh current)
```
Reviewed By: martinoluca
Differential Revision: D5500989
fbshipit-source-id: 7374b44
Summary:
Do not use the deprecated (and slower) `#infer` flavor. Instead, `infer-run`
runs capture with the `#infer-capture-all` flavor, followed by merging targets,
followed by the analysis.
Move the call to `MergeCapture` around to make this change easier.
Reviewed By: mbouaziz
Differential Revision: D5547199
fbshipit-source-id: 53c9996
Summary:
With current model, there are issues with cxx range loop. It looks like
it comes from std::vector::size model.
example of such FP:
```
int t = vec.size();
for(auto& elem : vec) {
auto x = elem
}
```
Reviewed By: jvillard
Differential Revision: D5545914
fbshipit-source-id: fbe55b3
Summary: Those are not particularly relevant for the biabduction analysis. It would be easy to have a dedicated checker for this if we happen to need one day.
Reviewed By: sblackshear
Differential Revision: D5530834
fbshipit-source-id: 316e60f
Summary:
The Eradicate `Nullable` checker should now be run using:
infer -a checkers --eradicate ...
Reviewed By: mbouaziz
Differential Revision: D5529226
fbshipit-source-id: 0de2956
Summary:
This is unsound but will help the analysis to report less false alarms with the common pattern:
if (a.get() != null) {
a.get().foo();
}
Reviewed By: sblackshear
Differential Revision: D5528227
fbshipit-source-id: 750db4a
Summary: This was reusing the side effects of the `add_constraints_on_retval` for the final purpose of being angelic and just assigning a fresh value to the lhs of the load.
Reviewed By: sblackshear
Differential Revision: D5507037
fbshipit-source-id: ec1c89c
Summary:
Bumps facebook-clang-plugins to a version that outputs sizeof() info in bytes and not bits.
update-submodule: facebook-clang-plugins
Reviewed By: akotulski
Differential Revision: D5526747
fbshipit-source-id: 6019542
Summary:
Cosmetic changes to the output of `make clean` and `make test-replace`.
Run the inferTraceBugs tests from tests/build_systems/ like other such e2e
tests, so that they can run in parallel with other tests instead of
sequentially at the end.
Reviewed By: dulmarod
Differential Revision: D5526599
fbshipit-source-id: 57231bd
Summary:
Works the same way as read/write races on fields, except that are more relaxed (er, unsound) in deciding whether two containers may alias.
This is needed to avoid reporting a ton of FP's; full explanation in comments.
Reviewed By: da319
Differential Revision: D5493404
fbshipit-source-id: 0a5d8b1
Summary:
This was an oversight. The prologue has to be run before capture and analysis
(see eg Infer.run). This sets up a bunch of things that are useful for the
diff analysis as well.
Reviewed By: jeremydubreil
Differential Revision: D5499405
fbshipit-source-id: 8f339cf
Summary: The `--failures-allowed` was doing for the Clang frontend what `--keep-doing` was doing for the backend. This revision merges the two options to simplify the Infer CLI and our tests.
Reviewed By: jvillard
Differential Revision: D5474347
fbshipit-source-id: 09bcea4
Summary:
update-submodule: facebook-clang-plugins
Moving to a newer version of clang, see ffb5dd0114
Reviewed By: jvillard
Differential Revision: D5452529
fbshipit-source-id: 28bc215
Summary:
The way we represented container writes before was pretty hacky: just use a dummy field for the name of the method that performs the container write.
This diff introduces a new access kind for container writes that is much more structured.
This will make it easier to soundly handle aliasing between containers and support container reads in the near future.
Reviewed By: da319
Differential Revision: D5465747
fbshipit-source-id: e021ec2
Summary: Using a dedicated abstract domain, like Quandary does, is more suitable for taint analysis.
Reviewed By: sblackshear
Differential Revision: D5473794
fbshipit-source-id: c917417
Summary:
Both `stringWithUTF8String` and `stringWithString` implements copy semantics that copies the content of their parameter into a newly allocated buffer. We modeled this as pointer assignment in the past, which means that once we write
```
NSString* foo() {
char buf[...];
...
return [NSString stringWithUTF8String:buf];
}
```
We are going to get a spurious stack variable address escape report because local pointer `buf` is assigned to the newly created string and the string gets returned.
This diff tries to address the issue by heap-allocating a buffer and `memcpy` the contents in `stringWithUTF8String` and `stringWithString`. But this change will create another problem: the allocated buffer will be reported as leaked by the backend, while in reality those buffers won't actually be leaked as they are allocated in a region that will be periodically autoreleased. To suppress spurious memory leak FPs, I added another attribute `Awont_leak` that will suppress the leakage report on any expressions that get tagged with it.
Reviewed By: jeremydubreil
Differential Revision: D5403084
fbshipit-source-id: df6de7f
Summary:
Pretty basic: warn when we see an assignment instruction `x = ...` and `x` is not live in the post of the instruction.
Only enabled for Clang at the moment because linters already warn on this for Java. But we can enable it later if we want to (should be fully generic).
Reviewed By: jeremydubreil
Differential Revision: D5450439
fbshipit-source-id: 693514c
Summary: We get the wrong answer on most of them for now, but that is expected
Reviewed By: ngorogiannis
Differential Revision: D5429242
fbshipit-source-id: 4899079
Summary:
This commit avoids precision loss on pruning.
// x -> [s$1, s$2]
if(x) { ... }
// x -> ?
before: x -> [min(0, s$1), max(0, s$2)]
because two x values, [0, 0] (true case) and [s$1, s$2] (false case), were joined after the if branch.
after: x -> [s$1, s$2]
Reviewed By: mbouaziz
Differential Revision: D5431009
fbshipit-source-id: 14a9efe
Summary:
This just makes the warnings silent for now. We may improve the analysis to check if the null check on the captured fields are consistent with the annotation on the corresponding parameters.
Eradicate also has the same issue. I added a test to outline this. The biabduction analysis will also probably fail on the same of annotation lookup. We may want implement the proper fix at the level of `Annotation.field_has_annot`.
Reviewed By: sblackshear
Differential Revision: D5419243
fbshipit-source-id: 6460de8
Summary:
First steps towards implementing diff analysis functionalities inside infer
itself. What works: run infer, checkout parent, re-run infer, checkout top
revision, compute the reportdiff (but no final surfacing on the console). Lots
of TODO still, inlined in the code.
Reviewed By: jberdine
Differential Revision: D5364226
fbshipit-source-id: 5b7f9a5
Summary: CFG nodes were not connected and some instructions ended up in wrong place. Fix those issues
Reviewed By: dulmarod
Differential Revision: D5406720
fbshipit-source-id: 2a70e1a
Summary:
Problem: The analyzer did not know that the value of `v.size()` is an alias of `v.infer_size`, so `v.infer_size` is not pruned by the if condition. As a result it raises a false alarm.
void safe_access(std::vector<int> v) {
if (v.size() >= 10) {
v[9] = 1; // error: BUFFER_OVERRUN Offset: [9, 9] Size: [5, 5]
}
}
void call_safe_access_Good() {
std::vector<int> v(5, 0);
safe_access(v);
}
Solution: Adding alias for return value to the abstract domain.
Now Inferbo can prune `v.infer_size` because it knows that the value of `v.size()` is an alias of `v.infer_size`. There is already an alias domain in Inferbo, so we added a specific room for the retrun value.
Reviewed By: jvillard, mbouaziz
Differential Revision: D5396988
fbshipit-source-id: 4a4702c
Summary:
Add support for converting without actually converting. To be used as
a landing pad for rebasing over the conversion.
Reviewed By: jvillard
Differential Revision: D5379967
fbshipit-source-id: 1bd932d
Summary: Adding to the Quandary tests, the list of tests that are already working for the bi-abduction based taint analysis.
Reviewed By: sblackshear
Differential Revision: D5395734
fbshipit-source-id: c4f2e79
Summary:
Fixes#683
Disclaimer: I don't know what XML namespaces/namespace prefixes are, but this seems to do the trick.
Reviewed By: mbouaziz
Differential Revision: D5381480
fbshipit-source-id: 3da16e7
Summary:
:
because otherwise people would believe they can use the internal representation of these std lib but it fails for our models.
Reviewed By: jvillard
Differential Revision: D5368671
fbshipit-source-id: 4e53d5a
Summary: So that it get rebuilt. Test is clang_compilation_db_relpath
Reviewed By: mbouaziz
Differential Revision: D5364561
fbshipit-source-id: 8cae984
Summary:
:
Get rid of model location in reports.
The goal is to avoid changing `issues.exp` whenever a model is updated.
Reviewed By: jvillard
Differential Revision: D5356608
fbshipit-source-id: 88ecaba
Summary:
Indexing into a string literal expression would generate a fresh
variable on every application of a transformer. This violated
finiteness of the domain, and caused divergence.
Reviewed By: da319
Differential Revision: D5342951
fbshipit-source-id: e95e84e
Summary: It was passing the argument multiple times rather than once with all the accepted values
Reviewed By: jvillard
Differential Revision: D5338509
fbshipit-source-id: 22b86a5
Summary:
It instantiates fields of structures when a pointer to which is given
as a function parameter, e.g., `foo(&s);`.
Reviewed By: mbouaziz, jvillard
Differential Revision: D5337645
fbshipit-source-id: c06da29
Summary:
We keep track of both `beginPtr` and `endPtr` but the modelling was mostly
about `beginPtr` as some sort of approximation I guess. This shouldn't change
much but will be useful later when doing more iterator stuff.
Reviewed By: mbouaziz
Differential Revision: D5255772
fbshipit-source-id: 0f6e3e8
Summary: This seems to move in the right direction. Also, `const operator[]` did not do an `access_at`, which I fixed.
Reviewed By: mbouaziz
Differential Revision: D5320427
fbshipit-source-id: c31c5ea
Summary: Unknown library returns the unknown pointer as well as the top interval.
Reviewed By: mbouaziz, jvillard
Differential Revision: D5282669
fbshipit-source-id: 34c7e18
Summary:
Once the fixed/preexisting/introduced sets have been computed, they endure
further filtering which may decide that more of them are equal. These bugs just
get dropped on the floor. Put these into preexisting as well instead, at least
in the case of the "skip_duplicated_types_on_filenames" filter.
Reviewed By: martinoluca
Differential Revision: D5274248
fbshipit-source-id: 99b3f3d
Summary:
This diff tries to achieve the followings: if we have the following C++ codes:
```
bool foo(int x, int y) {
return &x == &y;
}
```
We want the C++ frontend to emit Sil as if the input is written as
```
bool foo(int x, int y) {
if (&x == &y) return 1; else return 0;
}
```
This matches the behavior of our Java frontend.
The reason why we prefer an explicit branch is that it will force the backend to eagerly produce two different specs for `foo`. Without the explicit branch, for the above example the backend would produce one spec with `return = (&x == &y)` as the post condition, which is not ideal because (1) we don't want local variables to escape to the function summary, and (2) with the knowledge that no two local variables may alias each other, the backend could actually determines that `&x == &y` is always false, emitting a more precise postcondition `return = 0`. This is not possible if we do not eagerly resolve the comparison expression.
Reviewed By: akotulski
Differential Revision: D5260745
fbshipit-source-id: 6bbbf99
Summary:
:
There are throw wrapper functions like `std::__throw_bad_alloc()` defined in both libstdc++ (https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/functexcept.h) and libc++ (e.g. 907c1196a7/include/new (L145)). Folly actually exports some of them as well (diffusion/FBS/browse/master/fbcode/folly/portability/BitsFunctexcept.h). The function body of those wrappers merely throws the corresponding exception. My understanding is that the primary purpose of the wrappers is to throw the exception if everything goes well and to fall back to something reasonable when exception is disabled (e.g. when `-fno-exceptions` is passed to the compiler).
The problem is that infer doesn't really understand what those functions do, and I've seen some false positives get reported as a result of it. So to remove those FPs we need to either model them or handle them specially. Modeling those wrappers by either whitelisting them or overriding the include files turns out to be difficult, as those wrappers are only declared but not defined in the STL headers. Their implementations are not available to Infer so whitelisting them does nothing, and if I provide custom implementations in the headers then normal compilation process will be disrupted because the linker would complain about duplicated implementation.
What I did here is to replace functions whose name matches one of the throw wrapper's name with a `BuiltinDecls.exit`. I have to admit that this is a bit hacky: initially I was trying to do something more general: replacing functions with `noreturn` attribute with `BuitinDecls.exit`. That did not work because, CMIIW, the current frontend only exports function attributes for functions with actual bodies, not declaration-only functions. I'd love to be informed if there are better ways to handle those wrappers.
Reviewed By: jeremydubreil
Differential Revision: D5266030
fbshipit-source-id: 4580227
Summary:
:
What is relevant for the Buck integration is not the list of bugs that we find in a single target, which is essentially identical to testing `infer -- javac ...`, but to make sure that we still find the issues that are involving several Buck targets, and later other things like the caching mechanism.
This should also make the tests faster.
Reviewed By: jberdine, jvillard
Differential Revision: D5250205
fbshipit-source-id: 7f66b68
Summary: This change introduces the a new argument that lets you restrict the results of a differential report to only certain files.
Reviewed By: mbouaziz
Differential Revision: D5236626
fbshipit-source-id: 52711e9
Summary: We had a model for `Pools.SimplePool`, but were missing models for `Pools.Pool`. Since `SimplePool` and `SynchronizedPool` both extend `Pool`, modeling it should cover all of the cases.
Reviewed By: ngorogiannis
Differential Revision: D5236280
fbshipit-source-id: 9bbdb25
Summary:
:
No longer use deprecated reporting function for the suggest nullable checker
Depends on D5205009
Reviewed By: grievejia
Differential Revision: D5205843
fbshipit-source-id: f6dd059
Summary:
Read/write race errors should always show one trace for a read and one trace for a write.
We forget to pass the conflicting writes to the reporting function in one case, which prevented us from showing a well-formed trace.
Fixed it by making the `conflicts` parameter non-optional
Reviewed By: jberdine
Differential Revision: D5209332
fbshipit-source-id: 05da01a
Summary:
This messes up with the Buck cache: if infer recaptures a file with only trivial changes that shouldn't affect the capture Buck will still believe it has to reanalyze everything that depends on that file if there's non-deterministic data in infer's output.
Re-use the `--buck` flag used by the Java Buck integration for mostly the same purposes. Add a few special cases for the flavours integration (eg: keep capture data).
Change perf stats registration to take `Config.buck_cache_mode` into account instead of relying on each call site to handle that peculiarity correctly. Also, there's no need to create the perf stats directories before calling the registration function since it will do that too.
Reviewed By: jeremydubreil
Differential Revision: D5192311
fbshipit-source-id: 334ea6e
Sam Blackshear
Jeremy Dubreil
Jules Villard
Mehdi Bouaziz
Daiva Naudziuniene
Peter O'Hearn
Dulma Churchill
Dino Distefano
Sungkeun Cho
Vivek Dhiman
Dominic Cooney
Josh Berdine
Anders Papitto
Kyriakos Nikolaos Gkorogiannis
Roy Li
Greg Nisbet
Martino Luca
Jia Chen
Meng Zhang
Andrzej Kotulski
azhdar
Kihong Heo
Don Stewart