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: 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
Summary:
Update from dune 1 to dune 2. The change was mostly straightforward
except that change to (modes ...) default to exe only was somewhat
unexpected.
Anyhow, with the community moving to dune 2 it's good to keep up.
Reviewed By: ngorogiannis
Differential Revision: D19605895
fbshipit-source-id: 1f9830de8
Summary: This will be eventually used in RacerD. Further diffs up the stack will migrate away the starvation-specific aspects.
Reviewed By: ezgicicek
Differential Revision: D19623392
fbshipit-source-id: a72650718
Summary:
Prevent returning a negative cost bound when calling `substring(begin_index, end_index)` when either is possible
- `begin_index < 0`
- `begin_index > end_index`
Instead, return unit cost since such cases either throw `IndexOutOfBoundsException ` at runtime or correspond to having two symbolic bounds that cannot be semantically compared.
Reviewed By: dulmarod
Differential Revision: D19619410
fbshipit-source-id: cf5e8cb7b
Summary:
The "access path" memory model (equal access paths iff equal object addresses) is suited to when aliasing occurs only at the roots (i.e. variables). When there is intentional aliasing in the middle of an access path, this model will miss the aliasing. For instance if `[x.f] == [y.g]`, then also `[x.f.h] == [y.g.h]`, but the latter access paths are unequal.
In Java, non-static inner classes consistently alias `this.this$0` inside an inner class, which points to the "parent" outer-class object. So if two inner-class objects (belonging to different inner classes) access `this(type:InnerClassA).this$0.f` and `this(type:InnerClassB).this$0.f` the equality will be missed (many other combinations exist). This isn't strictly due to the memory model -- any alias analysis would have to do some class invariant inference to detect this.
For this purpose `AccessPath.inner_class_normalize` exists (it replaces `this.this$0` with `this` of the appropriate type), but this breaks the invariant that we know which formal parameter is at the root (there may not even exist a `this` parameter if the method is static). So this was buggy.
Here we simply recursively remove the synthetic field prefix of the accesses list, while computing forwards the object type. This is only applied when we check aliasing across threads. This will also allow actuals/parameters substitutions (stacked diff) which normalisation was breaking.
Reviewed By: jberdine
Differential Revision: D19601455
fbshipit-source-id: 7e42667b6
Summary:
Rename DeclaredNonnull -> UncheckedNonnull, Nonnull -> StrictNonnull.
This is a preparatory step to introduce one more nullability option,
specifically CheckedNonnull to support local nullsafe mode.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19619289
fbshipit-source-id: 12a3ff814
Summary:
This will help adoption of non-transitive strictification modes.
The main use-case we are aiming with this function is:
- During strictification, a method is made nullable, but it is not feasible to fix all
callers straight away. Suppressing nullsafe with an appropriate task or
comment will make it easier to unblock the strictification and move
forward.
Reviewed By: artempyanykh
Differential Revision: D19599098
fbshipit-source-id: 3769bbd3d
Summary:
1. I can not imagine a useful usecase for strict node, lets not
complicate the code.
2. Add docs.
Reviewed By: artempyanykh
Differential Revision: D19599091
fbshipit-source-id: 1d37a717f
Summary:
Reorder assertNotNull and assumeNotNull:
1. More useful methods go first.
2. More useful methods explicitly recommended over less useful.
Reviewed By: artempyanykh
Differential Revision: D19599084
fbshipit-source-id: 1b9ad9ef6
Summary: The translation of closures includes a load instruction for the captured variable, then we add that corresponding id to the closure. This doesn't correspond to an actual "use" of the captured variable in the source program though, and causes false positives. Here we remove the ids from the domain when that id is being added to a closure.
Reviewed By: ngorogiannis
Differential Revision: D19557352
fbshipit-source-id: 52b426011
Summary:
- Add `Nullsafe` annotation as a general mechanism to specify
type-checking behaviour for nullsafe.
- Document annotation params and provide usage examples with
explanations.
- Add tests to demonstrate the behaviour with different type-checking
modes.
No implementation is added. This diff serves as an RFC to hash out the
details before I dive into code.
Reviewed By: ngorogiannis
Differential Revision: D19578329
fbshipit-source-id: b1a9f6162
Mitya Lyubarskiy
Fernando Gasperi Jabalera
Dulma Churchill
Nikos Gorogiannis
Ezgi Çiçek
Sungkeun Cho
Jules Villard
Artem Pianykh