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: The diff remove the no-op model for `Cursor.close()` by the frontend-based `Closeable` as resources mechanism where every call of the form `object.close()` removes the file attribute on `object` when `object` is of type `Closeable`.
Reviewed By: sblackshear
Differential Revision: D4519386
fbshipit-source-id: 83633d4
Summary: Those should be treated angelically during the analysis with the same end results
Reviewed By: sblackshear
Differential Revision: D4518930
fbshipit-source-id: ee5bae8
Summary: Not clear why we need to disable this case and in which case is Infer creating too many disjunctions.
Reviewed By: sblackshear
Differential Revision: D4509394
fbshipit-source-id: fbc106d
Summary:
Some classes may have deleted new operator for them. To fix it, run global `new` operator instead
```
struct X {
void* operator new(size_t) = delete;
};
X *p = new X; // compilation error
X *p = ::new X; // no compilation error
```
This change is following same strategy standard headers follow.
Reviewed By: jvillard
Differential Revision: D4500977
fbshipit-source-id: 20babfa
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:
Turns this was needed only because we want infer-out to be models/infer.
Passing `--buck` together with passing `-d models` to `javac` was achieving the
same thing in a more roundabout way.
Reviewed By: jeremydubreil
Differential Revision: D4423185
fbshipit-source-id: 7cafe3b
Summary:
Fixes issue with template argument deduction with enable_shared_from_this as argument
```
#include<memory>
template<class T>
void makeWeak(const std::shared_ptr<T>& x) {}
struct X : public std::enable_shared_from_this<X>{
};
void test() {
X x
makeWeak(x.shared_from_this()); // compilation failed here - it was unable to deduce template parameter of makeWeak
}
```
Reviewed By: jvillard
Differential Revision: D4414788
fbshipit-source-id: 4d19c53
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
Summary: Use the lazy dynamic dispatch by default in prod for the Java analysis
Reviewed By: sblackshear
Differential Revision: D4356872
fbshipit-source-id: 491e92e
Summary:
Turns out that swapping stdout and stderr using a temporary fd 3 was screwing
up with make's jobserver, who also uses fd 3!
Also, infer is partly to blame as it also calls `make`. Unsetting `MAKEFLAGS`
in infer tells `make` that the way infer calls `make` is independent from
parent `make` invocations.
Also, simplify the rules for direct tests and build system tests.
Reviewed By: jberdine
Differential Revision: D4328979
fbshipit-source-id: 96818e8
Summary:
In particular, the method for retrieving an element from the array, this should never be nil. Also added a model for count
similar to the one for NSString length.
Reviewed By: ddino
Differential Revision: D4306655
fbshipit-source-id: 0ecb25a
Summary: Our C++ model magic didn't work when instantiating smart pointers with volatile types. Fix it
Reviewed By: jvillard
Differential Revision: D4313271
fbshipit-source-id: 55ffb98
Summary: Using multicore introduces some flakiness when building the models. This leads to summries for the models that are not always the same after rebuidling from scratch.
Reviewed By: jberdine
Differential Revision: D4306468
fbshipit-source-id: 96933d6
Summary:
The way interfaces are dealt with led to a false positive,
where tryLock() works OK for a Lock but not for a ReentrantLock.
The solution is just to provide the model.
While I am at it I am adding some more standard tests for Lock and ReentrantLock, which were not present.
Reviewed By: sblackshear
Differential Revision: D4204551
fbshipit-source-id: 9b6de28
Summary: clang has very complicated logic what to translate based on `project_root` and filename. Add tests for different situations in regard of symbolic links in path/project_root
Reviewed By: jvillard
Differential Revision: D4168551
fbshipit-source-id: 586b364
Summary:
`install` will not do anything if the file didn't change, which should give
`make` more opportunities to not do work.
Reviewed By: jeremydubreil
Differential Revision: D4161918
fbshipit-source-id: 9b9061a
Summary:
Config.analyze_models, set by the INFER_ANALYZE_MODELS environment
variable, is redundant with Config.models_mode.
Reviewed By: jvillard
Differential Revision: D4047338
fbshipit-source-id: 4522d65
Summary: The Infer builtins can be used in the e2e tests, but those tests should not depend on the Infer models to avoid cyclic dependencies. This diff separates the models and the Infer builtins in two directories so that the test can depend on the builtins without depending on the models
Reviewed By: sblackshear
Differential Revision: D3929478
fbshipit-source-id: 7d0ab79
Summary:
Given that mangling now respects `extern "C" {}` declarations, pnames of C function will have no mangling and we don't need to discard mangled part from procname.
Move `malloc` detection to `get_builtin_pname_opt` function (together with all others)
Reviewed By: dulmarod
Differential Revision: D3804402
fbshipit-source-id: 9ae9991
Summary:
1. models no longer need access to private fields (shared_ptr needed that)
2. create macro for __attribute__((deprecated("__infer_replace_with_deref_first_arg"))) and use it in models
Reviewed By: jberdine
Differential Revision: D3791113
fbshipit-source-id: 532dd33
Summary:
Follow strategy that was done to `std::shared_ptr` model and translate
`std::unique_ptr<T>` as raw pointer `T*`.
As a bonus, model `operator[]` of array overload as dereference
Reviewed By: jvillard
Differential Revision: D3785031
fbshipit-source-id: 2c5b0a4
Summary:
This reverts commit 7e3f8ffc84980ea5bcb4dae63a1588c07460c946.
Workaround is no longer needed since new clang we use has fix for this:
https://llvm.org/bugs/show_bug.cgi?id=28519
Reviewed By: dulmarod
Differential Revision: D3791083
fbshipit-source-id: ec339f1
Summary:
1. Add capability to clang frontend to replace some function calls with another SIL code based on `__deprecated__` attribute.
2. Given this capability, use those attributes for shared_ptr getters to generate `Sil.Load` instruction instead of method call
3. Add test that mimics shared_ptr model, but it doesn't have that much scary C++ templated code
Reviewed By: jvillard, jberdine
Differential Revision: D3729176
fbshipit-source-id: 2a330d5
Summary:
Make std::shared_ptr<T> translated as T* inside infer. This will make reporting better
since smart pointers are really pointers not structs - this form is much easier for the analyzer to understand.
This requires changes to the model of shared_ptr as well.
Reviewed By: jvillard
Differential Revision: D3587255
fbshipit-source-id: b86fb36
Summary:
So far infer had very fragile mechanism to detect smart pointers. It was looking for "std" and "(shared|unique)_ptr" inside name string.
This is easy to trick (like mystd::shared_ptr) and not something we want.
Instead, inside models create models inside infer_std_model namespace. Then just "export" that model into std namespace
via `using shared_ptr = infer_std_model<T>;`
Reviewed By: jvillard
Differential Revision: D3703827
fbshipit-source-id: 9640fc2
Summary:
a) An update to build-infer.sh to avoid downloading/building 4.02.3
ocaml compiler when it is already present.
b) Add the ifdef to avoid this error on glibc systems:
clang -c -w libc_basic.c -o libc_basic.o
libc_basic.c:692:12: error: unknown type name '__WAIT_STATUS'
pid_t wait(__WAIT_STATUS stat_loc) {
^
Closes https://github.com/facebook/infer/pull/429
Reviewed By: akotulski
Differential Revision: D3704604
Pulled By: jvillard
fbshipit-source-id: d557f1b
Summary:
Use Itanium mangling for C++ functions/methods instead of raw type name. This is a step towards removing expensive `ti_raw` field from `type_info`.
For virtual methods, use mangled name of the method from base class in order for dynamic dispatch to work.
Reviewed By: dulmarod
Differential Revision: D3556118
fbshipit-source-id: e45edb5
Summary:
When analyzing C model in C++, we were seeing some SKIP function triggered by generated constructors/operators= for C structs.
In C they weren't present, but in C++ compiler generates them for us. To avoid this (and future) problems
with models, translate all functions that are needed when computing the model
Reviewed By: dulmarod
Differential Revision: D3561873
fbshipit-source-id: f8ad2a0
Summary:
There is nice hook inside glog/logging.h for all CHECK_(GT|LT|GE|...) macros.
This simplifies AST significantly which makes infer way more happy:
cda16b3443/src/glog/logging.h.in (L722-L724)
Reviewed By: jberdine
Differential Revision: D3522110
fbshipit-source-id: 70c94cb
Summary:
Assume that std::vector::resize will always create nonempty vector. While this is clearly
wrong for resize(0), it removes many FPs for `resize(n)` calls, where value of `n` is unknown.
Without it, infer was thinking that `n` could be 0 and reported empty vector access.
Reviewed By: jvillard
Differential Revision: D3424355
fbshipit-source-id: cb476de
Summary:
Reuse data fields of model's superclass (which is actual implementation).
Not very pretty, but makes sizeof shared_ptr same as in actual libraries
Reviewed By: jvillard
Differential Revision: D3398628
fbshipit-source-id: bdb9418
Summary:
Array types where the length is not statically known were represented
using fresh variables. This diff:
- Makes array type length optional, reducing the amount of work needed
for renaming, substitution, and normalization.
- Revises uses of array length so that the length component of a
Tarray type represents only the statically determined constant
length of an array type, and the length component of a Sizeof
expression represents the dynamically determined length of an array
value.
- Restricts the type of static lengths from a general expression
(Sil.exp) to an integer (Sil.Int.t), enforcing that static types are
constant. This in particular ensures that types contain no
variables, and so are invariant under operations such as renaming
and substitution.
- Removes the type substitution and renaming functions typ_sub,
typ_normalize, and typ_captured_ren. Now that array type lengths
are constant integers, all of these functions are the identity.
Reviewed By: cristianoc
Differential Revision: D3387343
fbshipit-source-id: b5db768
Summary:
Specs files can become stale and cause crashes if the serialized format
changes. It is unclear how to add the right dependencies to the
existing targets, so this patch just removes them. Perhaps when the
specs are built we should drop an empty file to time-stamp them, and add
a target that depends on e.g. InferAnalyze and produces that file, and
then add dependencies on it.
Reviewed By: jvillard
Differential Revision: D3358460
fbshipit-source-id: d8aee48
Summary:
Hardcoding `variable@` in Makefiles is Bad™ because it prevents the users from
overwriting them easily with `make variable="my custom value"`. The right way
to do it is thus:
```
variable = variable@
# then use $(variable) everywhere
```
This diff puts all the `variable = variable@` lines in Makefile.config.in, and
changes every occurrence of a `variable@` to `$(variable)` everywhere else.
I mostly automated generating this diff. Here are the steps I did:
- find out which `variable@`s we use:
find . -name 'Makefile*' -exec grep -e '@[^@ []\+@' -o -h \{\} \+ | sort | uniq > config_variables
- write this `replace.sh` script to replace every `variable@` with `$(variable)`:
```
#!/bin/sh
config_vars_file=$1
shift
for line in $(cat $config_vars_file); do
var=$(echo $line | tr -d @)
sed -i -e "s/$line/\$($var)/g" $@ > /dev/null
done
```
- run the script as such:
find . -name 'Makefile.*in' \( -not -wholename './Makefile.config.in' \) -exec ./replace.sh config_variables \{\} \+
- put all the `VARIABLE = VARIABLE@` lines in Makefile.config.in
- move all `Makefile.in` to `Makefile`, since they don't need to be generated by `./configure` anymore:
```
for i in $(find . -name 'Makefile.*in' \( -not -wholename './Makefile.config.in' \)); do \
rm $(dirname $i)/$(basename $i .in) && git mv $i $(dirname $i)/$(basename $i .in) ; \
done
```
- delete all Makefile except Makefile.config from configure.ac
- manually inspect and remove remaining instances of `VAR = $(VAR)` in makefiles, looking at the output of `git grep '^\(\w\+\) = $(\1)'`
Reviewed By: jberdine
Differential Revision: D3358379
fbshipit-source-id: 5d37f02
Summary:
Turns out, analyzer was getting confused with complicated
model and it was reporting empty access in places it
shouldn't. Fixing backend is not trivial (tracing mode is the answer),
but the model can be simplified.
It introduces the problem that get() method doesn't return fresh value
every time, but we should be able to change backend later to deal with it.
Reviewed By: sblackshear
Differential Revision: D3328228
fbshipit-source-id: dddbaf8
Summary:
Build everything at once all the time. This removes the need for multiple
directories, which were a hassle to begin with.
This removes the `java`, `clang`, and `llvm` targets in various Makefiles as
well.
Reviewed By: jberdine
Differential Revision: D3317230
fbshipit-source-id: 8e86140
Summary:
Create model of C++ std::vector to find occurrences when vector which might be empty is accessed. Do it by triggering null dereference every time empty vector access is performed.
Note: model will be used only when c++11 (or c++14) are used.
Reviewed By: sblackshear
Differential Revision: D3276203
fbshipit-source-id: 420a95a
Summary: Create "empty" vector model header. The actual model implementation will come in next diffs to simplify review process.
Reviewed By: dulmarod
Differential Revision: D3240683
fb-gh-sync-id: 03ee002
fbshipit-source-id: 03ee002
Summary:stdlibc++ headers didn't like the fact that hash<unique_ptr> didn't have defined operator() directly.
Do that and provide empty body. Keep inheritance in case it helps compilation to succeed.
Reviewed By: dulmarod
Differential Revision: D3207721
fb-gh-sync-id: 8c950da
fbshipit-source-id: 8c950da
Summary:public
The code:
DataInputStream in = new DataInputStream(new BufferedInputStream(new FileInputStream(file)));
creates a resource with `FileInputStream()` and wraps it twice as a field of `BufferedInputStream` and then as a field of `DataInputStream`. Then calling:
in.close();
needs to go down the wrappers hierachy: `DataInputStream.close()` -> `FilterInputStream.close()` which then calls `BufferedInputStream.close()` -> `FilterInputStream.close()` -> `FileInputStream.close()`.
Going down the wrapper was not working before because `FilterInputStream.close()` was only going further when the type of field `in` was `FileInputStream` wheras it should also continue when the type of the field is any subtype of `FilterInputStream`, e.g. `DataInputStream` and `BufferedInputStream` like in the test example. This diff fixes this last aspect.
Reviewed By: sblackshear
Differential Revision: D3174822
fb-gh-sync-id: 3adbb7e
fbshipit-source-id: 3adbb7e
Summary:public
Add modeling of methods that can raise exceptions when parameters are
nil, and that return nil when passed nil.
Reviewed By: dulmarod
Differential Revision: D3101739
fb-gh-sync-id: 76af5a2
fbshipit-source-id: 76af5a2
Summary:public
clang-format changed filename inside __has_include(<FILENAME>), turn off
clang format for that part of the code.
Reviewed By: jberdine
Differential Revision: D3133593
fb-gh-sync-id: c601514
fbshipit-source-id: c601514
Summary:public
In order to make infer more resiliant to compilation failure,
make static_assert to do nothing.
As a bonus, set _FORTIFY_SOURCE inside same file instead of command line
Reviewed By: jvillard
Differential Revision: D3133446
fb-gh-sync-id: 590f4ad
fbshipit-source-id: 590f4ad
Summary:public
1. Make detection of libc++/stdlibc++ headers more robust
2. Turn on c++11 only for newer versions of stdlibc++
Reviewed By: jvillard
Differential Revision: D3121187
fb-gh-sync-id: c2e5be5
fbshipit-source-id: c2e5be5
Summary:public
This is needed to have a working `opam install infer`. Actual working `opam
install infer` in a follow-up diff.
Reviewed By: jberdine
Differential Revision: D3109308
fb-gh-sync-id: 49d7276
fbshipit-source-id: 49d7276
Summary:public
Conversion constructor of unique_ptr should be explicit. Fix it.
Reviewed By: cristianoc
Differential Revision: D3075661
fb-gh-sync-id: e911d8c
shipit-source-id: e911d8c
Summary:public
Create a model of std::unique_ptr in similar fashion to what was done to std::shared_ptr.
For now, we are modeling it as container of raw pointer (no ownership concept).
This time unique_ptr is not derived from std__unique_ptr (unlike shared_ptr, it was easier to not do that) and so we need to provide implementations for all non-member functions per C++ reference:
http://en.cppreference.com/w/cpp/memory/unique_ptr
Reviewed By: dulmarod
Differential Revision: D3048209
fb-gh-sync-id: a9a6455
shipit-source-id: a9a6455
Summary:public
Create initial model of C++ std::shared_ptr. This means that infer will replace implementation of
shared_ptr and the resulting binary will change. Make sure no one will run it by crashing any binary that includes that code.
Reviewed By: jvillard
Differential Revision: D2999948
fb-gh-sync-id: 5753559
shipit-source-id: 5753559
Summary:public
Create separate specs for C models compiled in C++. It will allow us to tweak behavior/names of certain
functions based on the compilation language (such as adding `std::` namespace in C++).
Reviewed By: jvillard
Differential Revision: D2938992
fb-gh-sync-id: 73902f8
shipit-source-id: 73902f8
Summary:
public
This model does not seem to bring anything anymore. Useless because ...
Reviewed By: cristianoc
Differential Revision: D2920118
fb-gh-sync-id: 7f708d7
shipit-source-id: 7f708d7
Summary:
public
While playing with the type environment for Java, I realised that the types in models.jar where not re-generated when modifying Infer. As a consequence, some changes in Infer where surprisingly having no effect. This diff forces the type environment to be absent when analyzing the models.
Reviewed By: sblackshear
Differential Revision: D2802517
fb-gh-sync-id: 1c2673a
Summary:
public
When building models, make was invoked explicitly with `make -j` rather
than with `$(MAKE)`. This prevented controlling parallelism from the
top-level make invocation.
Reviewed By: akotulski
Differential Revision: D2786812
fb-gh-sync-id: 7f6d58c
Summary: public The properties in those models needed to be made nonatomic.
Reviewed By: akotulski
Differential Revision: D2773650
fb-gh-sync-id: 903e7df
Summary:
public
Lines other than the first of multi-line comments in non-ocaml files
were flush right instead of aligned.
Reviewed By: jvillard
Differential Revision: D2739752
fb-gh-sync-id: c85f56e
Summary:
public
The case when a resource leaks is reported because the the resource was not closed on the execution branch created by the preconditions checks are not very interesting in practice because the exceptions thrown, either `NullPointerException` or `IllegalStateException` are very rarely caught anyway. So the legimate use of preconditions checks is creating spurious resource leak reports.
Reviewed By: sblackshear
Differential Revision: D2707227
fb-gh-sync-id: 6aece73
Summary:
public
Use autoconf's detection of xcode-select to decide whether to build the ObjC
models or not.
Reviewed By: jeremydubreil
Differential Revision: D2703864
fb-gh-sync-id: e6dadca
Summary: public
The classes `java.util.zip.{Inflater/Deflater}` where not modelled as resources. In practice, bad memory leak can happen using these classes and forcing the call to `end()` can help to avoid waisting native memory.
Reviewed By: sblackshear
Differential Revision: D2661249
fb-gh-sync-id: 1e33316
Summary: Add possibility of throwing IOException to model of
java.nio.channels.FileChannel.tryLock, and add test case.
public
Reviewed By: cristianoc
Differential Revision: D2658203
fb-gh-sync-id: 9ca9c02
Summary: Add models of the java.nio.channels.FileChannel.tryLock methods which can
return null according to the java docs.
public
Reviewed By: sblackshear, cristianoc
Differential Revision: D2650050
fb-gh-sync-id: ae6c8ce
Summary: Express model of java.io.File.listFiles using InferUndefined.object_undefined
instead of new File[...].
public
Reviewed By: jeremydubreil, sblackshear
Differential Revision: D2649406
fb-gh-sync-id: 93c5bb5
Summary: public
This should avoid confusion between the toplevel "infer" python script and
inferlib/infer.py.
Reviewed By: jeremydubreil
Differential Revision: D2637087
fb-gh-sync-id: 82ce2a4
Summary: public
This unclutters infer/bin/ and gives more structure to infer/lib/
Reviewed By: jeremydubreil
Differential Revision: D2605809
fb-gh-sync-id: 508fc2c
Summary:
System.getProperty can return null when the property is not found, and expects a non-null argument.
Add models for Infer and Eradicate to reflect that.
Summary: Handler.postDelayed keeps a persistent reference to its Runnable argument that may cause a memory leak if an Activity is reachable from the Runnable.
Summary:
Add a partial copy of TextUtils from Android source for commonly used TextUtils.isEmpty method.
Fixes#141
Closes https://github.com/facebook/infer/pull/143
Github Author: Deniz Türkoglu <deniz@spotify.com>
Summary:
The models for InputStreamReader and OutputStreamWriter are taking into consideration the charset passed as parameter in order to follow the exception branch when the charset is not valid. However, the previsous models were only considering encoding literals with uppercase letters. This diff adds the lowercase encoding names to the list.
Closes https://github.com/facebook/infer/issues/127
Summary: @publicThe first argument of builtin calls in C gets translated twice, which is bad if the argument is a side-effecting expression like a function call.
Test Plan: Attached test previously reported a memory leak because the translation introduces an extra call to malloc(), now reports nothing.
Summary:
@public
Remove setjmp that is causing problems in the models in linux.
Will investigate and add it again later.
Test Plan: All the models are now created. In particular strcpy, strdup and a few others in the beginning of the file.
Summary:
@public
Using InferBuiltins.assume previously caused an assertion failure in the analyzer. Fixed this, and fixed the implementation of the assume builtin to block when the assumed condition cannot hold.
Test Plan: Added several new tests.
Summary:
@public
Modeling bypasses the Closeable as resource assumption for `java.io.StringReader`, `java.io.ByteArrayInputStream` and `java.io.ByteArrayOutputStream`.
Test Plan: Infer CI. Some resource leak should also disappear on Instagram.
Summary:
@public
The models for Java no longer require to keep the original fields since we now make the union of the fields from the models and the fields from the code to analyze.
Test Plan: Infer CI. No functional change intended.
Summary:
@public
Attaching the resource attribute to the object allows to more easily remove this attribute during the symbolic execution when the resource is passed as a argument, e.g. with `res.close()` or when this resource is passed around via a skipped function.
Test Plan: Infer CI.
Jeremy Dubreil
Andrzej Kotulski
Jules Villard
Sam Blackshear
Josh Berdine
Dulma Churchill
Peter O'Hearn
Martino Luca
Raghavendra Prabhu
Lázaro Clapp Jiménez Labora
Dino Distefano
Cristiano Calcagno
Deniz Türkoglu