Summary: Instrument the ProcLocker to get the aggregated systime that is used by `lock` and `unlocks`.
Reviewed By: ngorogiannis
Differential Revision: D19814554
fbshipit-source-id: 5fd928b9c
Summary:
This was introduced in D19770219, and skcho caught it, but for some
reason I seemed to have a blind spot and confused the semantics of `instanceof`.
Reviewed By: skcho
Differential Revision: D19813031
fbshipit-source-id: d939b981b
Summary:
This diff fixes the clang translation for switch statement. It assumed that `default:` comes always
at last, which introduced some unreachable nodes inadvertently, e.g. when `default:` comes at first.
Reviewed By: dulmarod
Differential Revision: D19793138
fbshipit-source-id: 1e8b52c0d
Summary: After looking at some reports with blocks inside blocks, it seemed more obvious that adding which method we are talking about makes more clear which block we are talking about.
Reviewed By: mityal
Differential Revision: D19789285
fbshipit-source-id: 20e0e6804
Summary:
The domain has a notion of lock state (taken/not taken) and any access occurring will remember the current lock state.
Methods in Java can be `synchronized` meaning the lock is taken automatically at method start and release on return.
Currently, instead of starting analysis of a `synchronized` method with an initial lock state of "lock taken", the summary would be computed as if there is no lock, and then a caller would peek at whether the callee is `synchronized` and change the callee summary accordingly before taking it into account. Also, when an access happens (not a call) the analysis always consults the pdesc of the current method to check whether the method is `synchronized`.
Do things the right way: if method is `synchronized`, start with the lock taken. When the method exits, the lock-state postcondition is computed by releasing once the lock.
This is a behaviour-preserving change.
Reviewed By: mityal
Differential Revision: D19742559
fbshipit-source-id: 1d0fce3f6
Summary: This diff makes the taint analysis in Inferbo inter-procedural: adding symbolic taint values and substitute them at function call statements.
Reviewed By: ezgicicek
Differential Revision: D19411022
fbshipit-source-id: 4ff9a590a
Summary:
We already warn about lack of nullable annotations in `equals()`, and even have a specialized error message for that.
But lack of an annotation is not as severe as direct dereference: the
latter is a plain bug which is also a time bomb: it will lead to an NPE not immediately.
This is widespread enough to be reported separately.
Reviewed By: dulmarod
Differential Revision: D19719598
fbshipit-source-id: a535d43ea
Summary:
Since we fixed a bug in implementation of FalseOnNull (see stack below),
we can finally ship this change.
Side note: this change is essential for the follow up diff (which adds extra check
for user-defined implementations of equal()), without it the follow
up change would introduce a lot of false positives.
Reviewed By: ngorogiannis
Differential Revision: D19771057
fbshipit-source-id: 7d7cf1ef7
Summary:
If we managed to whitelist a function as TrueOnNull, we should teach
nullsafe the nullability of its arguments, otherwise it will ask not to
pass null here.
This fixes a silly FP warning, see the test.
Reviewed By: dulmarod
Differential Revision: D19770341
fbshipit-source-id: 0f861fae1
Summary:
Yay, the previous refactoring finally makes it possible to do some actual
changes to the code in `TypeCheck.ml`!
Changes in this diff:
1. Fixes the bug: TrueOnNull and FalseOnNull were working only for
static methods. Surpsingly nobody noticed that. It is because the first
argument for non-static method was `this`.
2. Behavior change: TrueOnNull/FalseOnNull were not working correctly
where there are several argumens. See the task attached for the example
of the legit usecase. Now the behavior is the following: if there are
several Nullable arguments infer nullability for all of them.
Reviewed By: skcho
Differential Revision: D19770219
fbshipit-source-id: 7dffe42cd
Summary:
This diff proceeds clean up the mess in TypeState.ml
This refactoring unblocks the change in the logic for true on null and
allows to fix a bug, see follow up diffs.
The current code is trying to do two things at once: processing boolean
results (which normally need manipulations with the argument of a
function + additional logic for containsKey), and comparisons witn null
(which requires manipulation with return value in typestate plus check
for redundancy).
All this was done in sort of generic fashion, which, among other, lead
to weird situations for edge cases, e.g. in processing containsKey we used to override nullability
already inferred as a non-null twice with losing information about
original nullability; which made logs weird).
Reviewed By: ngorogiannis
Differential Revision: D19768795
fbshipit-source-id: c928e2cff
Summary: The main job the schedulers do is building their work queues. That's being performed before the workers are forked which means they get copied into all of them. These changes push the initialization of the schedulers just after the forking takes place.
Reviewed By: ngorogiannis
Differential Revision: D19769741
fbshipit-source-id: 0b20ddd5c
Summary:
This diff is part of cleaning up of Typestate.ml mess to make it
somewhat maintainable.
This method is always called with `default` equivalent to (exp,
typestate); also there is no semantical need to return typestate because it never
gets changed.
Reviewed By: ngorogiannis
Differential Revision: D19767366
fbshipit-source-id: 173dcbbca
Summary:
This refactoring is made possible by previous stack, which ensured we
don't do two completely different things in one code anymore (processing
results of functions returning booleans and objects in generic fashion).
Follow up diffs will clean up the code for Prune(a != zero) case.
Reviewed By: dulmarod
Differential Revision: D19745050
fbshipit-source-id: 61d3d02ad
Summary:
This refactoring unblocks the changes in follow up diffs (plus fixes a
bug).
So what was happening?
Each comparison with null leads to CFG being splitted into two branches, one branch
is PRUNE(a == null) and another is PRUNE(a != null).
PRUNE(a != null) is where most of logic happens, it is the place where
we infer non-null nullability for a, and this is a natural place to
leave a check for redundancy.
Before this diff we effectively checked the same thing twice, and used
`true_branch` (only one of 2 instruction will have it set to true) as a symmetry breaker.
This diff removes the `true_branch` checks, but leaves only one call out
of two, hence breaking symmetry in a different way.
## Bug fix
The code around the removed check was (crazily) doing two things at
once: it processed results of (returning booleans!)
TrueOnNull-annotated functions AND
results of (returning Objects!) other functions, using the fact that all
of them are encoded as zero literals (sic!).
Not surprisingly that lead to a bug where we accidentally call the check
for non intended places (arguments of trueOnNull functions), which lead
to really weird FP.
This diff fixes it.
Reviewed By: dulmarod
Differential Revision: D19744604
fbshipit-source-id: fe4e65a8f
Summary:
These two methods are called in processing prune instructions, when
instruction is Prune(expr == null) and Prune(expr != null), to correctly
infer nullability in corresponding branches.
Typechecking underlying expr makes little sense for two reasons:
1. In practice, expr it is as simple as a temporary SIL variable
2. If the idea is defensively typecheck everything for case when SIL
produces crazy expressions, well, that is not going to work: the code
around ignores many other forms of expressions, e.g. everything where
expr = <something not equal to null literal>. So this is inconsistent.
This will simplify further cleanup, see follow up diffs
Reviewed By: ngorogiannis
Differential Revision: D19743826
fbshipit-source-id: 319a80ee7
Summary:
The whole TypeCheck.ml is exceptionally hard to read and maintain, lets
clean up it a bit.
Reviewed By: ngorogiannis
Differential Revision: D19743632
fbshipit-source-id: c24c21a85
Summary:
The goals are:
- Increase precision in C-languages by ditching access paths.
- Help with eventually sharing the abstract address module with RacerD.
- Reports are now language-mode specific (eg `->` in clang vs `.` in Java).
It's not exactly access expressions used here. Instead the pattern `(base, access list)` is used where `access` is `HilExp.Access.t`. This is done to ease the way `deriving` is used for creating two comparison functions, one that cares about the root variable and one that doesn't; and also because the main function that recurses over accesses (`normalise_access_list`) visits the accesses from innermost to outermost.
Also, kill some dead code.
Reviewed By: skcho
Differential Revision: D19741545
fbshipit-source-id: 013bf1a89
Summary: We don't use allocation costs in prod at the moment. There is no plan to do so in the near future. Let's not report them anymore and also save some space in `costs-report.json`.
Reviewed By: skcho
Differential Revision: D19766828
fbshipit-source-id: 06dffa61d
Summary:
This diff introduces two issue types: `BUFFER_OVERRUN_T1` and `INFERBO_ALLOC_IS_TAINTED`, which
denotes tainted values are used in array accesses and memory allocations, respectively.
Note that the taint analysis is intra-procedural for now.
Reviewed By: ezgicicek
Differential Revision: D19410536
fbshipit-source-id: af85148ec
Summary:
This diff adds a taint domain in Inferbo. The taint value will be used to find vulnerable array
accesses in the following diffs.
Reviewed By: ezgicicek
Differential Revision: D19391028
fbshipit-source-id: 566b4c0fe
Summary: Fixing bugs in the report_unchecked_strongself_issues_on_args function: we were not recursing if the first arg wasn't a var, and we were not removing the annotations from self in instance methods. Here we fix those issues.
Reviewed By: ngorogiannis
Differential Revision: D19662886
fbshipit-source-id: 03820961e
Summary:
The RestartSchedulerTests were failing because they are run in parallel by OUnit and all share the same output
directory. This makes the different tests collide since they are using the same file locks directory.
Reviewed By: jvillard
Differential Revision: D19765647
fbshipit-source-id: 5390ad14e
Summary:
This test tests PropagatesNullable and TrueOnNull/FalseOnNull
annotations.
Both tests suites grew big so it is hard to observe them at glance and
make changes.
I could not figure out better name for TrueFalseOnNull.java, it is sort
of silly but I optimized for searchability, "FalseOnNull" will be
directly searched and "TrueOnNull" will be searched in IDEs that are
smart enough.
Reviewed By: skcho
Differential Revision: D19724512
fbshipit-source-id: 703961342
Summary:
The problem with is_override that it can be misleading: it does not
check that that class of the first method is a subtype of the second; in
fact it totally ignores the classes.
This method is publicly exposed so lets just call what it does.
Reviewed By: jvillard
Differential Revision: D19724295
fbshipit-source-id: dc0919193
Summary: This diff returns non-symbolic value (top) for unknown external function calls because the symbolic values sometimes make it hard to understand costs.
Reviewed By: ezgicicek
Differential Revision: D18685715
fbshipit-source-id: 1b39c718b
Summary:
Pulse has an extra invalidation mechanism (introduced in D18726203) to prevent something invalid (e.g. `null`) to be passed by reference to an initialisation function. Therefore, it havocs formals passed by reference to skipped functions. However, I don't think this makes sense in Java. So, let's turn it off.
A nice consequence of this is that in impurity analysis, we do not consider functions that call skipped library calls with object arguments as writing to their formals.
Reviewed By: skcho
Differential Revision: D19697110
fbshipit-source-id: 6e3a71f2a
Summary:
To emulate the `ThreadSafe` contract in C++/ObjC, reporting was gated behind a check that ensured a C++/ObjC class has a `std::mutex` member (plus other filters). This is reasonable, but it has some drawbacks
- other locks may be used, and therefore must be added to the member check;
- locking mechanisms that use the object itself as a monitor cannot be modelled (`synchronized` in ObjC)
RacerD already has `ThreadsDomain` which models our guess on whether a method is expected to run in a concurrent context, and which in C++/ObjC boils down to whether the method non-transitively acquires a lock. This should be a good enough indicator that the class should be checked regardless of whether the locks are member fields. This diff gates the C++/ObjC check on that abstract property.
Reviewed By: dulmarod
Differential Revision: D19558355
fbshipit-source-id: 229d7ff82
Summary: This diff removes a dead field, `is_cpp_nothrow` and `is_cpp_noexcept_method`.
Reviewed By: jvillard
Differential Revision: D19489417
fbshipit-source-id: 971a7f533
Summary:
The ProcLocker uses files as locks and relies on the guarantees of the `Unix.open_file` function when using `O_CREAT` and `O_EXCL` simultaneously.
- `setup`: creates a directory for the lock files inside `infe-out` and deletes its content if it already existed.
- `clean`: does nothing for now. Any file locks that may have been left unlocked are removed by the `setup` in the next run. This way the user can see what locks were taken if the program crashes.
- `lock_exn`: try to lock the `Procname` and if it can't releases all the locks that is currently holding.
- `unlock`: removes the corresponding file.
Reviewed By: ngorogiannis
Differential Revision: D19639402
fbshipit-source-id: e02f277ff
Summary:
Revert incomplete/incorrect translation of `synchronized` in ObjC.
The current translation is incomplete because
```
syncrhonized(foo){
return;
}
```
should be translated as
```
__set_locked_attribute(foo);
__delete_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
but instead we get
```
__set_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
The same applies for `break`/`continue` etc
Reviewed By: skcho
Differential Revision: D19718882
fbshipit-source-id: fc49ef529
Summary:
- Thread the two types into one instead of having a record where the `path` field doesn't always make sense (`Class` case).
- Improved pretty printing of class objects (java only).
- Move starvation-specific stuff out of `AbstractAddress` (eg `make_java_synchronized`).
- Slight optimisation of `apply_subst` for when a parameter is used without additional accesses inside a method (then, the substitution need not modify the term substituted for the parameter in any way).
Reviewed By: ezgicicek
Differential Revision: D19639922
fbshipit-source-id: 1cebecf5d
Summary:
`String` and `StringBuilder` both implement `CharSequence`. Let's generalize the model for `String` to `CharSequence` wherever possible and add missing models for
- `StringBuilder.append`
- `StringBuilder.toString`
Reviewed By: skcho
Differential Revision: D19558009
fbshipit-source-id: 0dfdb21af
Summary:
Java's String models were broken for
- initializing a String object with a locally defined constant string (which is an `Object*` in SIL).
- initializing a String object with a `char`/`byte` array
This diff fixes them and also adds models for `new String ()`.
Reviewed By: skcho
Differential Revision: D19662180
fbshipit-source-id: 23968d0aa
Summary:
When the expression resolving to a function to be called could not be
translated to either a proc name or at least an access path, the HIL
translation code would crash. However, this is perfectly possible. Moreover, no
one actually uses the payload of the `Indirect` datatype so no complication
arises from generilising its type to `HilExp.t`, as done in this diff.
Reviewed By: ngorogiannis
Differential Revision: D19691127
fbshipit-source-id: 4c0400ab7
Summary:
`replace_make_shared` is finding a constructor by iterating all fields of type structs, which may be
expensive if there are lots of fields or `replace_make_shared` is called many times with the same parameters.
Reviewed By: ngorogiannis
Differential Revision: D19446260
fbshipit-source-id: c5c279ca5
Summary: This diff fixes the array access checking function for nested global arrays. We had assumed that RHS of `store` statement in SIL does not include array access expression, but that is not true: for global arrays, SIL can have statements like `*LHS = GlobalArray[n][m]`.
Reviewed By: ezgicicek
Differential Revision: D19300153
fbshipit-source-id: 256325642
Summary:
This diff gives semantics of `std::make_shared` as simple constructor, i.e., it changes function
call of `std::make_chared<C>(i)` to the constructor `C(i)`.
Reviewed By: ngorogiannis
Differential Revision: D19432338
fbshipit-source-id: 0d838e555
Summary:
Add a field to LLAIR variables to indicate whether they are global or
local. Update the LLVM semantics for constant expression evaluation to
be relational, so that it doesn't have to have an answer for references
to undefined globals.
Reviewed By: jberdine
Differential Revision: D19446312
fbshipit-source-id: 9bbfd180e
Summary: This adds a check for when developers use weakSelf (and maybe strongSelf) in a block, when there is no need for it, because it won't cause a retain cycle. In general there is an annotation in Objective-C for methods that take blocks, NS_NOESCAPE, that means that the passed block won't leave the scope, i.e. is "no escaping". So we report when weakSelf is used and the block has the annotation.
Reviewed By: ngorogiannis
Differential Revision: D19662468
fbshipit-source-id: f5ac695aa
Summary:
This attribute is given to parameters of methods that take Objective-C blocks to show that they will be used only in the current context and won't "escape" the context.
We translate it here, with the goal to use it in a new check later. The check is about not using weakSelf in non-escaping blocks, because retain cycles are not possible.
The translation is a bit complex because the annotation comes in the parameter of a method, but in the checker we will need it in the block. So we pass it around in the frontend from the translation of the method call to the translation context and on to the block expression and the block declaration afterwards.
Reviewed By: ngorogiannis
Differential Revision: D19600377
fbshipit-source-id: dd49539bd
Summary:
The difference from default set is mostly in:
1. turning on warnings 32..39 `unused X`,
2. turning off warning 66 `unused open!` since `open! IStd` is present
in pretty much all the files.
3. Non-exhaustive pattern match is now treated as a compilation
error.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19646047
fbshipit-source-id: c84ba628a
Summary:
Refactor all occurences of `is_strict_mode` to use `NullsafeMode`
instead. This will allow introducing _local_ typechecking modes for
nullsafe in the follow up patches.
Reviewed By: ezgicicek
Differential Revision: D19639883
fbshipit-source-id: bdf535b66
Summary: Moving this big tuple to a record, because it's cleaner code, and I need to add another element in the next diff.
Reviewed By: ezgicicek
Differential Revision: D19640389
fbshipit-source-id: 86b1576a0
Fernando Gasperi Jabalera
Mitya Lyubarskiy
Sungkeun Cho
Dulma Churchill
Nikos Gorogiannis
Ezgi Çiçek
Jules Villard
Scott Owens
Artem Pianykh