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: 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: 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:
:
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:
:
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 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:
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: 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: We were almost always using `~report_reachable:true`, and in the cases where we weren't it is fine to do so. In general, a sink could read any state from its parameters, so it makes sense to complain if anything reachable from them is tainted.
Reviewed By: mbouaziz
Differential Revision: D5169067
fbshipit-source-id: ea7d659
Summary:
For now, we just support clearing the taint on a return value.
Ideally, we would associate a kind with the sanitizer and only clear taint that matches that kind.
However, it's fairly complicated to make that work properly with footprint sources.
I have some ideas about how to do it with passthroughs instead, but let's just do the simple thing for now.
Reviewed By: jeremydubreil
Differential Revision: D5141906
fbshipit-source-id: a5b8b5e
Summary:
- model `exit` as `Bottom`
- model `fgetc` as returning `[-1; 255]` rather than `[-1; +oo]`
- reduced the number of model functions for simple models
Reviewed By: KihongHeo
Differential Revision: D5137485
fbshipit-source-id: 943eeeb
Summary:
This is a minimal change to (poorly) recognize and model std::mutex
lock and unlock methods, and to surface all thread safety issues for
C++ based on the computed summaries with no filtering.
This ignores much of the Java analysis, including everything about the
Threads domain. The S/N is comically low at this point.
Reviewed By: sblackshear
Differential Revision: D5120485
fbshipit-source-id: 0f08caa
Summary:
This diff fixes unintentional bottoms in pointer arithmetic of inferbo.
The pointer arithmetic on addresses of variables (not array) just returns
the operand.
Reviewed By: jvillard
Differential Revision: D5060424
fbshipit-source-id: 495d8b8
Summary: Using Conjunction for thread join has known false negatives. Finer grained recording of threading information fixes this.
Reviewed By: sblackshear
Differential Revision: D5111161
fbshipit-source-id: aab483c
Summary: This fixes a couple of false positives as objects of BufferedReader don't need to be closed if the wrapped reader resource gets closed correctly.
Reviewed By: sblackshear
Differential Revision: D5106596
fbshipit-source-id: 725fb80
Summary: Gflags is a popular library used to create command line arguments. Flags shouldn't flow directly to `exec` etc.
Reviewed By: jvillard, mbouaziz
Differential Revision: D5058393
fbshipit-source-id: ab062f8
Summary: Useful to have Eradicate and Biabduction agree on how to inform that the analysis that some objects are not null.
Reviewed By: sblackshear
Differential Revision: D5075127
fbshipit-source-id: 9e56981
Summary: String are very important for taint analysis, have to make sure that we have the right models/the right behaviors for unknown code.
Reviewed By: jvillard
Differential Revision: D5054832
fbshipit-source-id: 7e7ee07
Summary:
Previously all knowledge of the dynamic length of such arrays was lost to infer:
```
void foo(int len) {
int a[len];
}
```
The translation of this program would make no reference to `len` (except as a
param of `foo`).
Translate this "initialization" using the existing `__set_array_length` infer
builtin, as:
```
# Declare local a[_]
n$0 = len;
__set_array_length(a, len);
```
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4969446
fbshipit-source-id: dff860f
Summary:
This commit fixes a problem that the buffer overrun checker incorrectly
stops when a global variable (bottom) is involved in control flow.
In the new version, abstract memories return Top for unanalyzed abstract
variables.
Reviewed By: mbouaziz
Differential Revision: D5016447
fbshipit-source-id: 5132448
Summary:
The code was pretty fragile, it's less fragile now. We should probably just
delete that and start outputting proper class/package info directly in the
report.json instead of reverse-engineering them.
Fixes#640
Reviewed By: jeremydubreil
Differential Revision: D4905973
fbshipit-source-id: 1590067
Summary:
An array has a static or dynamic length (number of elements), but it also has a
stride, determined by the type of the element: `sizeof(element_type)`. We don't
have a good `sizeof()` function available on SIL types, so record that stride
in the array type.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4969697
fbshipit-source-id: 98e0670
Summary: In particular, the heuristics for propagating taint via unknown code needs to be aware of the frontend's trick of introducing dummy return variables.
Reviewed By: mbouaziz
Differential Revision: D5046345
fbshipit-source-id: da87665
Summary:
HIL had only been tested in Java, and it made some assumptions about what array expressions look like (the LHS is always resolvable to an access path) and assignments (the LHS is always an access path) that aren't true in C.
Fixed the code so we won't crash in this case.
Thanks to jeremydubreil for catching this.
Reviewed By: jeremydubreil
Differential Revision: D5047649
fbshipit-source-id: e8484f4
Summary:
There are two pointer-related operations you can do in C++ but not Java that we need to support in taint analysis:
(1) `*formal_ptr = ...` when `formal_ptr` is a formal that's a pointer type. Java doesn't have raw pointers, so we didn't need to handle this case.
(2) Passing by reference, which Java also doesn't have (everything is pass-by-value).
Reviewed By: mbouaziz
Differential Revision: D5041246
fbshipit-source-id: 4e8f962
Summary:
Introduce `infer-<command>` for each command, except for internal commands
(only `infer-clang` for now) which are not exported. Install these executables
(which are just symlinks to `infer`) on `make install`. The main executable
looks at the name it was invoked with to figure out if it should behave as a
particular command.
Get rid of `InferClang`, `InferAnalyze`, and `InferPrint`. As a bonus, we now
only need to build one executable: `infer`, which should be a few seconds
faster (less link time).
`InferAnalyze` is now `infer-analyze` and `InferPrint` is `infer-print`. To run
`InferClang`, use a symlink named `clang`, `clang++`, etc. to `infer`. There
are such symlinks available in "infer/lib/wrappers/" already.
I also noticed that the scripts in xcodebuild_wrappers/ don't seem useful
anymore, so use wrappers/ instead, as for `make`.
Reviewed By: mbouaziz
Differential Revision: D5036495
fbshipit-source-id: 4a90030
Summary: Same as D5026082, but allowing specification in JSON rather than harcoded in Infer.
Reviewed By: jeremydubreil
Differential Revision: D5030042
fbshipit-source-id: 8a6cfee
Summary: A lot of C++ library functions look like this, so it's important to have.
Reviewed By: mbouaziz
Differential Revision: D5026082
fbshipit-source-id: 6f421b6
Summary: Making sure simple passthroughs like the identity function work in C++.
Reviewed By: mbouaziz
Differential Revision: D5024031
fbshipit-source-id: ce48ead
Summary:
Now,
infer -a infer -- ...
and
infer -a checkers --biabduction -- ...
will return the same list of errors
Reviewed By: sblackshear
Differential Revision: D5023223
fbshipit-source-id: f52ce5d
Summary:
Needed because this is how the Clang frontend translates returns of non-POD, non pointer values (I think)?
Will handle the more general case of pass by reference soon.
Reviewed By: jvillard
Differential Revision: D5017653
fbshipit-source-id: 1fbcea5
Summary:
`[a, b] < [a, _]` and `[_, a] < [b, a]` are most probably false (it comes from size < size)
Mark definitely unsatisfied conditions as B1, others as B2+
Reviewed By: KihongHeo, jvillard
Differential Revision: D4962107
fbshipit-source-id: ba8f469
Summary:
The bufferoverrun checkers can now be run with:
infer -a checkers --bufferoverrun -- ...
Reviewed By: jeremydubreil
Differential Revision: D5010689
fbshipit-source-id: 2eaa396
Summary:
The Siof checkers can now be run with:
infer -a checkers --siof -- ...
and also runs by default using:
infer -a checkers -- ...
Reviewed By: jberdine
Differential Revision: D5009731
fbshipit-source-id: e0e2168
Summary:
First step to be able to enable and disable the checkers to run in the following form:
> infer -a checkers --checker1 --checker2 --checker3 -- ...
and have a predefined list of checkers that are run by default with:
> infer -a checkers -- ...
Reviewed By: sblackshear
Differential Revision: D5007377
fbshipit-source-id: d7339ef
Summary:
This gives the option to run the biabduction analysis together with the other Clang-based checkers with the command:
infer -a checkers --biabduction -- ...
The filtering does not work yet because the filtering for the biabduction analysis matches the analyzer `Infer`, and does not filter much when the analyzer is `Checkers`, which is the case here.
Reviewed By: sblackshear
Differential Revision: D4773834
fbshipit-source-id: 16300cc
Summary:
Don't store redundant information in C++ template Type.Name.t.
New signature:
```
| CppClass (qual_name, template_args)
```
For example, for `std::shared_ptr<int>`, will look like this:
```
| CppClass (["std", "shared_ptr"], Template [int])
```
While it used to be:
```
| CppClass (["std", "shared_ptr<int>"], Template (["std", "shared_ptr"], [int]))
```
Reviewed By: jberdine, mbouaziz
Differential Revision: D4834512
fbshipit-source-id: cb1c570
Summary:
Before we understood ownership, we needed this to avoid a mountain of Builder-related FP's.
Now that we have fairly sophisticated understanding of ownership, we can kill this hack.
Reviewed By: jaegs
Differential Revision: D4940238
fbshipit-source-id: 8d86e57
Summary:
Title.
The way types are printed is completely valid, but little weird for some C++ programmers:
`int const` - same as `const int`
`int * const` - pointer is `const`, value under it is not
`int const *` - pointer is not `const`, but the value is
`int const * const` - both pointer and value are const
Reviewed By: jberdine
Differential Revision: D4962180
fbshipit-source-id: dcb02e3
Summary:
Modify the type of `Exp.Sizeof ...` to include the value that the expression
evaluates to according to the compiler, or None if it cannot be known
statically.
Use this information in inferbo.
Mostly unused in the BiAbduction checker for now, although it could be useful
there too.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4953634
fbshipit-source-id: be0999d
Summary:
- Use Logging for logging
- Add a few extra debug (not ON by default)
- No space before colon
Depends on D4961957
Reviewed By: KihongHeo
Differential Revision: D4961989
fbshipit-source-id: 7c8697d
Summary: The purpose of the annotation reachability analysis is to report when a method annotated with `X` never calls, directly or indirectly, another method annotated with `Y`. However, there can be different call stacks following different execution paths from `X` to `Y`. Reporting more than one call stack ending with the same annotated procedure does not bring more signal to the end user. So the purpose of this diff is to avoid those duplicated reports and report at most one annotation reachability issue per end of call stack.
Reviewed By: sblackshear
Differential Revision: D4942765
fbshipit-source-id: 46325a7
Summary:
- Bottom-lift abstract memory domain to express unreachable node
- Two cases to make a node unreachable
+ constant: when an evaluation result of condition expression is
bottom or false, e.g., "prune(0)".
+ alias: when the same structure e is compared to itself with "<",
">", and "!=", e.g., "prune(e < e)".
- Add test for the new prune (prune_constant.c, prune_alias.c)
- Debug the semantics of comparison
Reviewed By: mbouaziz
Differential Revision: D4938055
fbshipit-source-id: d0fadf0
Summary:
As an interprocedural checker, SIOF should not run unless explicitly required.
Make it a new type of analyzer like other similar checkers.
Reviewed By: mbouaziz
Differential Revision: D4937820
fbshipit-source-id: a9e2d38
Summary: `bufferoverrun` is not part of `checkers` yet so bufferoverrun tests require their own Makefile
Reviewed By: jvillard
Differential Revision: D4937558
fbshipit-source-id: 20380f1
Summary: Sawja assigns them on multiple control-flow paths, so they're not SSA.
Reviewed By: peterogithub
Differential Revision: D4896745
fbshipit-source-id: c805216
Summary:
There are false positives in the current analysis due to the
use of conjunction in the treatment of threaded. Changing conjunction to disjunction
removes these false positives. Some new false negatives arise, but all the old tests pass.
This is a stopgap towards a better solution being planned.
Reviewed By: sblackshear
Differential Revision: D4883280
fbshipit-source-id: c2a7e6e
Summary: The flakiness is exercised by upgrading the OCaml compiler to 4.04.0.
Reviewed By: jberdine
Differential Revision: D4859904
fbshipit-source-id: 0dd7ff6
Summary:
If we couldn't project the callee access path into the caller during summary application, we still added the corresponding trace to the caller state.
This was wasteful; it just bloats the caller with state it will never look at.
Fixed it by making `get_caller_ap_node` return `None` when the state won't be visible in the caller.
Reviewed By: jeremydubreil
Differential Revision: D4727937
fbshipit-source-id: 87665e9
Summary: This should make the reports much easier to understand. We can generalize to reporting a stack trace for all of the writes in the future if we wish.
Reviewed By: peterogithub
Differential Revision: D4845641
fbshipit-source-id: 589fdbc
Summary:
Make it possible to write one model which will be used by all template instantiations.
There is one big missing piece: infer never tries to do template instantiation by itself. With current code, it's possible to use generic models
as long as header contains `__infer_generic_model` annotation (see the test as an example).
This is not viable to modify all headers with this annotation hence infer will try to do template instantiation for generic models in later diffs.
Reviewed By: jberdine
Differential Revision: D4826365
fbshipit-source-id: 2233e42
Summary: If two public methods touch the same state and only one is marked `ThreadSafe`, it's reasonable to report unsafe accesses on both of them.
Reviewed By: peterogithub
Differential Revision: D4785038
fbshipit-source-id: 5a80da4
Summary: `__attribute__((annotate("")))` is better way of passing information to the frontend
Reviewed By: jberdine
Differential Revision: D4818805
fbshipit-source-id: 6e8add2
Summary:
*Unless* the unprotected write runs on the main thread and the read doesn't.
Otherwise, we'll already report on the unprotected write, and we don't want to duplicate.
Reviewed By: peterogithub
Differential Revision: D4798357
fbshipit-source-id: 5de06a0
Summary:
This is step further simplify the code to avoid cases where the summary of the procedure being analyzed can exist in two different versions:
# one version is the summary passed as parameter to every checker
# the other is a copy of the summary in the in-memory specs table
This diff implements:
# the analysis always run through the `Ondemand` module (was already the case before)
# the summary of the procedure being analyzed is created at the beginning of the on-demand analysis call
# all the checkers run in sequence, update their respective part of the payload and log errors to the error table
# the summary is store at the end of the on-demand analysis call
Reviewed By: sblackshear
Differential Revision: D4787414
fbshipit-source-id: 2d115c9
Summary:
Adds a new type and branching for a missing path of execution.
closes#575
Reviewed By: jvillard
Differential Revision: D4738681
fbshipit-source-id: f72344c
Summary:
It can be useful when debugging infer or the Makefiles themselves to see what
`make` is doing. Instead of editing Makefiles to remove `@` now you can `make
VERBOSE=1`.
This is just `git ls-files | grep -e Makefile -e '.*\.make' | xargs sed -e 's/^\t@/\t$(QUIET)/' -i`, and adding the definition of `QUIET` to Makefile.config.
Reviewed By: jeremydubreil
Differential Revision: D4779115
fbshipit-source-id: e6e4642
Summary:
No new functionality here; mostly `FN_` tests documenting our current limitations.
Will start chipping away at the false negatives in follow-up diffs.
Reviewed By: peterogithub
Differential Revision: D4780013
fbshipit-source-id: 7a0c821
Summary: Bringing the logic back to where it was before the big refactoring of the reporting logic.
Reviewed By: peterogithub
Differential Revision: D4774541
fbshipit-source-id: afeaaf8
Summary:
Move all of the reporting on top of the aggregation functionality.
This lets us delete lots of code
Reviewed By: peterogithub
Differential Revision: D4772223
fbshipit-source-id: 47cc51a
Summary:
This was the one type of races we were not yet reporting (besides ones that use the wrong synchronization :)).
Wrote new utility function to aggregate all accesses by the memory they access.
This makes it easy to say which accesses we should report and what their conflicts are.
Eventually, we can simplify the reporting of other kinds of unsafe accesses using this structure.
Reviewed By: peterogithub
Differential Revision: D4770542
fbshipit-source-id: 96d948e
Summary:
For collections whose type does not express that the collection is thread-safe (e.g., `Collections.syncrhonizedMap` and friends).
If you annotate a field holding one of these collections, we won't warn when you mutate the collection.
Reviewed By: jeremydubreil
Differential Revision: D4763565
fbshipit-source-id: 58b487a
Summary: It seems that we were not really using the `Bottom` part of the domain as a pair of (empty call map, empty tracking var map) was already acting as bottom.
Reviewed By: sblackshear
Differential Revision: D4759757
fbshipit-source-id: 53dedfe
Summary:
Most of our tests work by comparing the output of some command to a baseline
expected output. It's often needed to update that baseline.
Previously, that was crudely done by attempting to move every foo.exp.test file
to foo.exp. This does not work terribly well, in particular because
foo.exp.test might be stale.
Instead, add a `replace` target to every test that knows how to update the
baseline. This allows custom behaviours too, eg in the differential tests.
Most of the tests include base.make or differential.make, so add a replace target
there. A few tests are completely custom, add a replace target to them too.
Reviewed By: jeremydubreil
Differential Revision: D4754279
fbshipit-source-id: ec34273
Summary:
This issue was spotted in the wild. There may be more of those, unfortunately it's hard to predict
More general problem is that types in infer frontend diverge from clang's types for DerivedToBase cast.
Then, infer uses types from clang anyway and that confuses backend. Getting it always right is very hard
Reviewed By: jvillard
Differential Revision: D4754081
fbshipit-source-id: 5fb7069
Summary:
If I read off the main thread and write on the main we
could have a race. (Writes off main are already reported.)
Reviewed By: sblackshear
Differential Revision: D4746138
fbshipit-source-id: 8b6e9c5
Summary:
All tests were redirecting `stderr` into duplicates.txt which made it much harder to see other error messages in stderr (such as uncaught exceptions).
To mitigate it, write duplicates to separate file and don't redirect `stderr` to another file.
Reviewed By: jvillard
Differential Revision: D4728938
fbshipit-source-id: 8ad2fc8
Summary:
One limitation of Eradicate is that certain nullability patterns are not expressible using simply the `Nullable` annotation.
One such pattern is using the knowledge that a function returns null when passed null, but returns an object otherwise.
The annotation `PropagatesNullable` is a variant of `Nullable` applied to parameters when their value propagates to the return value.
A method annotated
```
B m(PropagatesNullable A x) { return x == null ? x : B(x); }
```
indicates that `m` returns null if `x` is null, or an object of class `B` if the argument is not null.
Examples with multiple parameters are in the test cases.
This diff builds some infrastructure for annotation transformers: the example above represents the identity function on nullability annotations.
Reviewed By: jvillard
Differential Revision: D4705938
fbshipit-source-id: 9f6194e
Summary:
Before, `trace_of_pname` only grabbed unprotected writes from the summary, so the traces ending in an unprotected read were truncated.
We now look at reads too when appropriate.
Reviewed By: peterogithub
Differential Revision: D4719740
fbshipit-source-id: 28f6e63
Summary: Now, running `infer -a checkers -- ...` will also run the ThreadSafety checker
Reviewed By: sblackshear
Differential Revision: D4691330
fbshipit-source-id: 04fc781
Summary: Run all the checkers one after each other, which allows the Infer AI framework to run several checkers together, including the possibility for them to collaborate.
Reviewed By: sblackshear
Differential Revision: D4621838
fbshipit-source-id: e264d67
Summary:
This makes sure that one can run `./build-infer.sh` then `make`. Otherwise it's
not always clear what one should do to recompile infer, eg when `make` will
work and when `./build-infer.sh` should be used instead, in particular when the
user doesn't have opam configured for her terminal.
Reviewed By: jberdine
Differential Revision: D4698159
fbshipit-source-id: 5df8059
Summary:
We were including hex of empty string if mangled name was not empty (so for all C++ functions).
Instead, include hex of a source file only if it's not empty
Reviewed By: mbouaziz
Differential Revision: D4705388
fbshipit-source-id: 55b6587
Summary:
Procnames files are now reversed qualifier lists with `#` as separator (instead of `::` which needs to be escaped in bash).
Because of the mechanism that is used to obtain qualifiers, it also affects naming for ObjC classes.
Examples:
```
std::unique_ptr<int>::get -> get#unique_ptr<int>#std#__MANGLED,...__ // C++ method
folly::split -> split#folly#__MANGLED,..._ // function within namespace
NSNumber numberWithBool: -> numberWithBool:#NSNumber#class // ObjC method
```
Reviewed By: jvillard
Differential Revision: D4689701
fbshipit-source-id: c3acfc6
Summary:
We want to stop supporting '-multiletter' options altogether in the future as
they are not standard.
For boolean switches, we used to have '-short' for true and '-nshort' for
false, but that second form is necessarily > 1 letters, which doesn't work in
this scheme. Replace these by '-s' for true and '-S' for false.
Reviewed By: jberdine
Differential Revision: D4674079
fbshipit-source-id: 95bacfe
Summary:
When both an unprotected write and a read/write race emanate from the same line,
undoubtedly because of interprocedurality, strip the read/write report (for now).
Perhaps report the info in more succinct form later, but keep to one report/line.
Reviewed By: sblackshear
Differential Revision: D4685102
fbshipit-source-id: 291cf20
Summary:
There was a bug where we allowed ourselves to project local variables from the callee summary into an access path in the caller.
We should only be able to project callee variables that are in the footprint.
Reviewed By: jeremydubreil
Differential Revision: D4684868
fbshipit-source-id: 53a2b9d
Summary:
Updated version of the plugin exports some missing `VarDecls`. To make sure it doesn't break again,
add a test that didn't work before.
Reviewed By: dulmarod
Differential Revision: D4674123
fbshipit-source-id: 0c1677a
Summary:
Eradicate detects circular field initializations (e.g. a field initialized with itself)
by checking in the typestate at the end of the constructor whether the origin
of the field is a field name in the current class.
This has the problem that the following initialization pattern is not recognized as correct:
C(C x) { this.field = x.field }
To fix the issue, the origin information for field accesses x.f is extended
with the origin information of the inner object x.
Circularities are detected if the origin of x is "this".
Reviewed By: jberdine
Differential Revision: D4672472
fbshipit-source-id: 9277bdd
Summary: Previously, we wouldn't report races where the write was under synchronization.
Reviewed By: peterogithub
Differential Revision: D4658850
fbshipit-source-id: e9f4c41
Summary: I encountered cases where the class name part of the method name was passed as `(None, "package.Class")` instead of `("package", "Class")` and therefore incorrectly failing some inequality checks
Reviewed By: sblackshear
Differential Revision: D4662617
fbshipit-source-id: 98ee3e3
Summary:
Stop multiple reports per line happening. These come about
because of interprocedural access to multiple fields. Present one trace,
and summary information about other accesses.
Reviewed By: sblackshear
Differential Revision: D4636232
fbshipit-source-id: 9039fea
Summary:
All intermediate `.exp` files used for tests can be generated with custom info, based on what is needed for the tests purposes.
This customisation happens via command-line argument `--issues-fields`.
Reviewed By: cristianoc, jvillard
Differential Revision: D4628062
fbshipit-source-id: feaa382
Summary:
- The package declaration was wrong
- There was a leftover copy-pasted resource leak test from `CursorLeak.java`.
Reviewed By: sblackshear
Differential Revision: D4612687
fbshipit-source-id: 42c1a35
Summary: Rather than having three separate annotations related to checking/assuming thread-safety, let's just have one annotation instead.
Reviewed By: peterogithub
Differential Revision: D4605258
fbshipit-source-id: 17c935b
Summary: distinguish writes via method calls (e.g., add) from writes via assignment in the error messages
Reviewed By: sblackshear
Differential Revision: D4611748
fbshipit-source-id: 7594d3b
Summary: Report at most one read/write race or unprotected write per access path per method
Reviewed By: sblackshear, jvillard
Differential Revision: D4590815
fbshipit-source-id: 3c3a9d9
Summary:
To address a common source of false positives observed in D4494901.
We don't do anything with `release` yet, but can model it as releasing ownership in the future if we want to enforce correct usage of `SynchronizedPool`'s.
Reviewed By: peterogithub
Differential Revision: D4593635
fbshipit-source-id: 621e937
Summary: Reports on reads that have one or more conflicting writes. When you report, say which other methods race with it.
Reviewed By: sblackshear
Differential Revision: D4538793
fbshipit-source-id: 47ce700
Summary: Thread-local variables can't be shared between threads, so it's safe to mutate them outside of synchronization
Reviewed By: jeremydubreil
Differential Revision: D4568316
fbshipit-source-id: 0634cad
Summary:
Enrich the domain of SIOF to contain, as well as the globals needed by a
procedure, the globals that the procedure initializes. Also add the possibility
to model some procedures as initializing some variables. Use that mechanism to
teach the checker about `std::ios_base::Init`.
Reviewed By: mbouaziz
Differential Revision: D4588284
fbshipit-source-id: d72fc87
Summary:
This gives a way for users to flag safe methods regarding SIOF, for instance if
the problematic paths in the method cannot happen before `main()` has started.
Reviewed By: akotulski
Differential Revision: D4578700
fbshipit-source-id: 6542dcf
Summary:
I couldn't figure out why, but from within an infer release the traces we get
for this test are different than the expected ones. This is even consistent
across osx and linux.
In order to restore sanity, let's just hide this incomprehensible fact. Let's
come back to it if more tests exhibit this, maybe traces are not guaranteed to
be exactly the same across runs.
Reviewed By: sblackshear
Differential Revision: D4559405
fbshipit-source-id: dd88c59
Summary: Should stop us from reporting on benign races of fields that are caching resources.
Reviewed By: peterogithub
Differential Revision: D4538037
fbshipit-source-id: 15236b4
Summary: This annotation can then be used to suppress the warnings on non-android Java projects.
Reviewed By: sblackshear
Differential Revision: D4544858
fbshipit-source-id: 8a0b8fa
Summary:
The Java models for resources are way to complex. The main issue I am facing with these models is that small changes in the analysis can affect the generation of the models in some weird ways. For instance, I get different specs for some of the models between my devserver and my devvm, which seems to be mostly related with the backend treatment of `instanceof`.
The objective here is to simplify the models as much as possible in order to:
1) make debugging regressions easier
2) get simpler specs and less modeled methods shipped in `models.jar`
Reviewed By: sblackshear
Differential Revision: D4536115
fbshipit-source-id: 577183a
Summary: Better documentation, and could perhaps be checked instead of trusted later if the analysis understands threads better.
Reviewed By: jaegs
Differential Revision: D4537463
fbshipit-source-id: 4323c78
Summary: In some cases where a function is called directly on a formal (e.g, `def foo(o) { callSomething(o) }`, we were failing to propagate the footprint trace to the caller.
Reviewed By: jeremydubreil
Differential Revision: D4502404
fbshipit-source-id: d4d632f
Summary: This case was already working but there was no tests for it
Reviewed By: sblackshear
Differential Revision: D4529473
fbshipit-source-id: ca3ff02
Summary: This will be important for maintaining ownership of `View`'s, which involve a lot of casting.
Reviewed By: peterogithub
Differential Revision: D4520441
fbshipit-source-id: fdef226
Summary:
Previously, we would lose track of ownership in code like
```
Obj owned = new Obj();
Obj stillOwned = id(owned); // would lose ownership here
stillOwned.f = ... // would report false alarm here
```
This diff partially addresses the problem by adding a notion of "unconditional" (always owned) or "conditional" (owned if some formal at index i is owned) ownership.
Now we can handle simple examples like the one above.
I say "partially" because we still can't handle cases where there are different reasons for conditional ownership, such as
```
oneOrTwo(Obj o1, Obj o2) { if (*) return o1; else return o2; } // we won't understand that this maintains ownership if both formals are owned
Obj stillOwned = oneOrTwo(owned1, owned2);
stillOwned.f = ... // we'll report a false alarm here
```
This can be addressed in the future, but will require slightly more work
Reviewed By: peterogithub
Differential Revision: D4520069
fbshipit-source-id: 99c7418
Summary: This fixes a wrong level of indirection when performing the type substitution.
Reviewed By: sblackshear
Differential Revision: D4521008
fbshipit-source-id: 7324ea6
Summary:
This fixes false positives we had in fields written by callees of a constructor (see new E2E test).
This is also a bit cleaner than what we did before; instead of special-casing constructors, we just use the existing ownership concept.
Reviewed By: peterogithub
Differential Revision: D4505161
fbshipit-source-id: a739ebc
Summary:
Constants are always "owned" in the sense that no one can mutate them.
In code like
```
Obj getX(boolean b) {
if (b) {
return null;
}
return new Obj();
}
```
, we need to understand this in order to infer that the returned value is owned.
This should fix a few FP's that I've seen.
Reviewed By: peterogithub
Differential Revision: D4485452
fbshipit-source-id: beae15b
Summary:
Infer used to report null dereference when field was accessed later:
```
vector<int> v;
int& a = v[0]; // should be EMPTY_VECTOR_ACCESS here, but it wasn't reported
int b = a; // was NULL_DEREFERENCE here
```
To avoid this problem, model all accesses to vector as dereference of its internal `beginPtr` field.
Reviewed By: jberdine
Differential Revision: D4481942
fbshipit-source-id: 2142894
Summary: This should fix the issue with broken invariants when the method specialization on pointer ends up doing a substitution on non pointer types
Reviewed By: sblackshear
Differential Revision: D4487232
fbshipit-source-id: f3fce84
Summary: The method `junit.framework.TestCase.setUp()` is always run before the other methods by the JUnit testing framework. So the method act as a class initializer.
Reviewed By: sblackshear
Differential Revision: D4487371
fbshipit-source-id: 1998801
Summary: Just adding some more test cases on how Infer handles dynamic dispatch.
Reviewed By: jberdine
Differential Revision: D4486529
fbshipit-source-id: d90ef42
Summary:
This modifies `CTrans_utils.dereference_value_from_result` function. Reasons:
1. Type in Sil.Load tuple is supposed to be the type of Ident.t field (result), not Exp.t (source)
2. Inside `dereference_value_from_result` we used wrong type when `~strip_pointer:true` - it used original type instead of the resulting one
3. Fixing (2), uncovered similar issue in `CTrans_utils.cast_operation` - it should have used resulting type, not original one
Changed tests are expected:
1. `deprecated_hack.cpp` test was how I discovered this issue (it was very wrong)
2. the other test also had issues, now it's correct
Sadly, there was no logic I followed when writing this change - I stuffed changes in a way that fits, but it may be breaking something somewhere else.
Reviewed By: jberdine
Differential Revision: D4481895
fbshipit-source-id: b139c59
Summary:
This diff adds a set of access paths holding a value returned from a method annotated with Functional to the domain.
If a "functional" value is written to a field, we won't count that right as an unprotected access.
The idea is to be able to use the Functional annotation to get rid of benign race false positive, such as:
```
Functional T iAlwaysReturnTheSameThing();
T mCache;
T memoizedGetter() {
if (mCache == null) {
mCache = iAlwaysReturnTheSameThing();
}
return mCache;
}
```
Although there is a write-write race on `mCache`, we don't care because it will be assigned to the same value regardless of which writer wins.
Reviewed By: peterogithub
Differential Revision: D4476492
fbshipit-source-id: cfa5dfc
Summary:
We warn on unsafe accesses to fields that occur in a public method (or are reachable from a public method).
We ought not to consider VisibleForTesting methods as public, since they are only public for testing purposes.
Reviewed By: peterogithub
Differential Revision: D4477648
fbshipit-source-id: 5f58914
Summary: Simple model for List methods that write to the collection.
Reviewed By: peterogithub
Differential Revision: D4453381
fbshipit-source-id: 19edc51
Summary:
Sometimes some instructions were never part of any CFG node and were not written to CFG at all. Add a mechanism that will create node and add them to CFG when they reach compoundStmt translation.
This is step forward to make `*x;` instruction actualy dereference x (it works in C already in C++ AST looks different though)
Reviewed By: dulmarod
Differential Revision: D4469139
fbshipit-source-id: b03b11c
Summary:
Previously, we would correctly be silent on code like `x = new T(); x.f = ...`, but would wrongly warn on code like `x = makeT(); x.f = ...`.
The reason is that we only allowed ownership through direct allocation.
This diff adds a boolean that specifies whether the return value is owned as part of the summary.
This allows us to correctly handle many common cases of (transitively) returning a freshly allocated object, but still won't work for understanding that ownership is maintained in examples like
`x = new T(); y = id(x); y.f = ...`.
Reviewed By: jvillard
Differential Revision: D4456864
fbshipit-source-id: b5eec02
Summary:
Eradicate currently considers a field initialized if it's simply accessed (not written to),
or initialized with another initialized field.
This fixes the issue.
Reviewed By: jvillard
Differential Revision: D4449541
fbshipit-source-id: 06265a8
Summary:
If we have code like
```
o.setF(source())
sink(o)
```
and `setF` is an unknown method, we probably want to report.
Reviewed By: jeremydubreil, mburman
Differential Revision: D4438896
fbshipit-source-id: 5edd204
Summary:
In code like
```
foo(o) {
iWriteToF(o)
}
```
, the condtional write to `f` in `iWriteToF` should become a conditional write for `foo`.
Reviewed By: peterogithub
Differential Revision: D4429160
fbshipit-source-id: f111ac4
Summary:
In code like
```
foo() {
Object local = new Object();
iWriteToAField(local);
}
```
, we don't want to warn because the object pointed to by `local` is owned by the caller, then ownership is transferred to the callee.
This diff supports this by introducing a notion of "conditional" and "unconditional" writes.
Conditional writes are writes that are rooted in a formal of the current procedure, and they are safe only if the actual bound to that formal is owned at the call site (as in the `foo` example above).
Unconditional writes are rooted in a local, and they are only safe if a lock is held in the caller.
Reviewed By: peterogithub
Differential Revision: D4429131
fbshipit-source-id: 2c6112b
Summary:
Races on volatile fields are less concerning than races on non-volatile fields because at least the read/write won't result in garbage.
For now, let's de-prioritize these writes by ignoring them.
Reviewed By: peterogithub
Differential Revision: D4434023
fbshipit-source-id: 05043ba
Summary:
Also make sure we don't introduce deprecated options in our repo, eg when
calling infer from infer.
Reviewed By: jeremydubreil
Differential Revision: D4430379
fbshipit-source-id: 77ea7fd
Summary: Just cleanup; gives us slightly less test code to maintain.
Reviewed By: jeremydubreil
Differential Revision: D4429265
fbshipit-source-id: d43c308
Summary:
Similar to marking classes ThreadConfined, we want to support marking fields as well.
The intended semantics are: don't warn on writes to the marked field outside of syncrhonization, but continue to warn on accesses to subfields.
Reviewed By: peterogithub
Differential Revision: D4406890
fbshipit-source-id: af8a114
Summary: Generalized the CppTrace into a Clang trace because we don't currently have separate checkers for Obj-C and Cpp. Happy to separate them later if there is a good reason
Reviewed By: akotulski
Differential Revision: D4394952
fbshipit-source-id: e288761
Summary:
Adding models that allow us to warn on unguarded accesses to subclasses of `Map`, but not on accesses of threadsafe containers like `ConcurrentMap`.
Lots more containers to model later, but stopping at `Map`s for now to make sure the approach looks ok.
Reviewed By: jvillard
Differential Revision: D4385306
fbshipit-source-id: d791eee
Summary: Need to upgrade in order to specify some taint properties on a more recent `WebView` API.
Reviewed By: cristianoc
Differential Revision: D4382590
fbshipit-source-id: 0925742
Martino Luca
Jia Chen
Sam Blackshear
Jeremy Dubreil
Mehdi Bouaziz
Sungkeun Cho
Dulma Churchill
Andrzej Kotulski
azhdar
Josh Berdine
Kihong Heo
Daiva Naudziuniene
Jules Villard
Dino Distefano
Kyriakos Nikolaos Gkorogiannis
Don Stewart
Peter O'Hearn
Paco Estevez Garcia
Cristiano Calcagno
Kihong Heo