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
Summary: Keep the type name of the class as the key in the map constructed from class names to their methods in a file. This will be used later, and also why string?
Reviewed By: dulmarod
Differential Revision: D19557707
fbshipit-source-id: aa8569581
Summary: D19496263 had refactored cost to its own subdirectory. In this process, it introduced a perf degradation by bundling of collection and computation of constraints which relies on imperative union find. This diff fixes that by separating them.
Reviewed By: ngorogiannis
Differential Revision: D19598610
fbshipit-source-id: 0e466522d
Summary: Add the interface that will be used to lock Procedures.
Reviewed By: ngorogiannis
Differential Revision: D19580578
fbshipit-source-id: b5e334b18
Summary: `String.split(regexp)` returns an array that is split by the given regexp. If the regexp doesn't match, the original string is returned. Hence, the resulting array's length must be in `[1, max(1, n_u -1)]` where`n_u` is the upper bound of the string's length.
Reviewed By: dulmarod
Differential Revision: D19578318
fbshipit-source-id: 675af7376
Summary:
Adding a new check as part of the SelfInBlock checker, for cases when a strong pointer to self, that is not self, is captured in the block.
This will cover the following wrong scenarios:
1. strongSelf is a local variable in a block as it should be, and then it's used in a sub-block, in which case it's a captured variable.
2. weakSelf is defined without the weak attribute by mistake.
Reviewed By: ngorogiannis
Differential Revision: D19538036
fbshipit-source-id: 151871745
Summary:
In practice, condition redundant is extremely noisy and low-signal
warning (hence it is turned off by default).
This diff does minor tweaks, without the intention to change anything
substantially:
1/ Change severity to advice
2/ Change "is" to "might be"
3/ Describe the reason in case the origin comes from a method.
The short term motivation is to use 3/ for specific use-case: running nullsafe on codebase and
identify most suspicious functions (that are not annotated by often
compared with null).
Reviewed By: artempyanykh
Differential Revision: D19553571
fbshipit-source-id: 2b43ea0af
Summary:
This is/can be useful for future changes that make the reporting more
precise based on the exact origin.
See the follow up diff as an example.
Reviewed By: artempyanykh
Differential Revision: D19553567
fbshipit-source-id: c2b2c28a1
Summary: Currently, empty summaries are passed to the cluster callbacks. This is pointless and could potentially lead to recomputing already analysed summaries. This change passes only procnames to the callback, and `Ondemand` is used to load the summary or analyse as needed.
Reviewed By: jvillard
Differential Revision: D19544043
fbshipit-source-id: 28ab642c3
Summary: There is no need to provide type environments to cluster analysers, since the execution environment can be used to retrieve those on demand.
Reviewed By: jvillard
Differential Revision: D19543561
fbshipit-source-id: f9b064011
Summary: We were reporting strongSelf Not Checked when the variable was checked in a conditional, this diff fixes that.
Reviewed By: jvillard
Differential Revision: D19535943
fbshipit-source-id: f8e64e1b7
Summary: I noticed when looking into a false positive of strongSelf Not Checked, that there were some inconsistencies in the translation of if statements with an and, with an extra redundant join only if using a method in the condition that returned an object. So I could repro the problem and investigate and found the place of the inconsistency in the translation. This diff fixes it without changing things too much.
Reviewed By: jvillard
Differential Revision: D19518368
fbshipit-source-id: 47a6a778c
Summary:
The order by which the scheduler visits odd and even methods here
will determine if there is any report at all. This is a bad test
so remove.
Reviewed By: fgasperij
Differential Revision: D19535537
fbshipit-source-id: 6b64b0de9
Summary: Adding reporting to strongSelf Not Checked when strongSelf is passed to a method in a not explicitly nullable position.
Reviewed By: ezgicicek
Differential Revision: D19330872
fbshipit-source-id: 95871a70a
Summary: After receiving feedback about this, I'm changing the reporting of strongSelf Not Checked to only in cases where it can cause a crash. Here I'm adding reporting for field access, and removing general reporting. In a next diff, I'll also add reporting for passing strongSelf to methods in not explicitly nullable positions.
Reviewed By: skcho
Differential Revision: D19329842
fbshipit-source-id: 35beb2aa3
Summary:
The RestartScheduler needs to know if the worker finished it's task
because:
1. there was no more work to do or
2. found that a needed Procname was already taken (this part is not yet implemented)
This need was addressed by (i) making the functions that the workers execute
return a value of task_result.t intead of unit and (ii) adding a
constructor to the worker_message.t (FinishedTask).
Reviewed By: ngorogiannis
Differential Revision: D19467783
fbshipit-source-id: a76b02b6c
Summary:
1. One should use either a writer or a stream to send a response, but not both.
2. A response should be forwarded only if it was commited.
Both properties are extracted from API comments on classes in the servlet API.
Reviewed By: jvillard
Differential Revision: D19514568
fbshipit-source-id: 79f0257ed
Summary:
If data comes from an outer OutputStream, then this outer OutputStream
needs to be flushed before getting the byte array.
Reviewed By: jvillard
Differential Revision: D19514569
fbshipit-source-id: e3e025394
Summary: We were lacking this kind of test where one interface refines the nullability of the other.
Reviewed By: ngorogiannis
Differential Revision: D19514245
fbshipit-source-id: fa3e781f3
Summary: `cost.ml` is huge. Let's split it to its logical parts (basically creating new files for the modules that were already in `cost.ml`) and move all cost related files into `\cost` directory. While we are at it, let's add `mli` files too.
Reviewed By: jvillard
Differential Revision: D19496263
fbshipit-source-id: 45096db4c
Summary: Deadlocks often result in two reports if not deduplicated (two traces), so there is some logic for doing that. Locks recently became an opaque type, with the `get_access_path` loophole supporting that deduplication ordering. Fix that here and remove `Lock.get_access_path`.
Reviewed By: jvillard
Differential Revision: D19465223
fbshipit-source-id: b597e3c65
Summary:
This is a common enough case to make error message specific.
Also let's ensure it's modelled.
Reviewed By: artempyanykh
Differential Revision: D19431899
fbshipit-source-id: f34459cb3
Summary:
The previous diff changes the message for params case, this one handles
return.
Reviewed By: artempyanykh
Differential Revision: D19430706
fbshipit-source-id: f897f0e56
Summary:
This diff gets global constant array values from their initializers. The `find_global_array` function is
added to memory domain, which finds values of global array locations during the ondemand value
generation.
Reviewed By: ngorogiannis
Differential Revision: D19300143
fbshipit-source-id: 7b0b84c42
Summary: Use more informative method names, and add comments explaining the logic behind each test. Correct two cases which are FPs instead of legitimate reports.
Reviewed By: artempyanykh
Differential Revision: D19465227
fbshipit-source-id: 29332e2b9
Summary: Change `MayBlock` and `StrictModeCall` constructors from taking a string to a `Procname.t`, which was the sole source of that string anyway.
Reviewed By: ezgicicek
Differential Revision: D19465226
fbshipit-source-id: e3ed6ef88
Summary:
If a race exists in two or more overloads of the same method and we use only the class and method name in the report text, then the current bug hashing algorithm will identify the two reports as duplicates.
To avoid this, the report had the class, method and list of type parameters. This is unreadable, however, and redundant (the report is already located within the method in question). So at the risk of duplicates, use only class+method names.
Also, fix a bug in `Procname.pp_simplified ~withclass` where `withclass` was ignored for C++/ObjC methods.
Now:
> Read/Write race. Non-private method `FrescoVitoImageSpec.onCreateInitialState(...)` indirectly reads with synchronization from `factory.AnimatedFactoryProvider.sImpl`. Potentially races with unsynchronized write in method `FrescoVitoImageSpec.onEnteredWorkingRange(...)`.@ [Litho components are required to be thread safe because of multi-threaded layout](https://fburl.com/background-layout). Reporting because current class is annotated `MountSpec`, so we assume that this method can run in parallel with other non-private methods in the class (including itself).
Before
> Read/Write race. Non-private method `void FrescoVitoImageSpec.onCreateInitialState(ComponentContext,StateValue,StateValue,Uri,MultiUri,ImageOptions,FrescoContext,Object,ImageListener)` indirectly reads with synchronization from `factory.AnimatedFactoryProvider.sImpl`. Potentially races with unsynchronized write in method `FrescoVitoImageSpec.onEnteredWorkingRange(...)`.@ [Litho components are required to be thread safe because of multi-threaded layout](https://fburl.com/background-layout). Reporting because current class is annotated `MountSpec`, so we assume that this method can run in parallel with other non-private methods in the class (including itself).
Reviewed By: artempyanykh
Differential Revision: D19462277
fbshipit-source-id: aebc20d89
Summary:
Currently, impurity analysis is oblivious to skipped functions which might e.g. return a non-deterministic value, write to memory or have some other side-effect. This diff fixes that by relying on Pulse's skipped functions to determine impurity. Any unknown function which is not modeled to be pure is assumed to be impure.
This is a heuristic. We could have assumed them to be pure by default as well.
Reviewed By: jvillard
Differential Revision: D19428514
fbshipit-source-id: 82efe04f9
Summary:
Let's collect the list of all skipped functions with a `proc_name` but no summary in Pulse's memory. This will be useful for the impurity analysis later (next diff).
Concretely, we extend Pulse's domain with a map from skipped calls to their respective traces. For efficiency, we only keep a single trace per skipped call.
For impurity analysis, tracking skipped calls in Pulse allows us to rely on Pulse's strong memory model to get rid of infeasible paths as opposed to creating an independent checker which wouldn't be able to do that.
Reviewed By: jvillard
Differential Revision: D19428426
fbshipit-source-id: 3c5e482c5
Summary:
As suggested by Ilya, the current message can be improved in a way that
it can contain more clear action. I also added artempyanykh's explanation at the
end of message to provide an additional justification from common sense
perspective.
But most importantly, the previous message was missing a space which is
eye bleeding, how come haven't I noticed this before, I can't stand it
OMG.
Reviewed By: artempyanykh
Differential Revision: D19430271
fbshipit-source-id: dd31f7adb
Summary:
Inferbo analyzed some program points unreachable incorrectly, because of unsound semantics of band
operator, which did not handle the case when given parameters are pointer values.
Reviewed By: ngorogiannis
Differential Revision: D19392705
fbshipit-source-id: dd590508c
Summary:
This diff revises the generation of unknown value. If the type of the unknown value generating is
int, it does not add the "Unknown" pointer/array value.
Reviewed By: ngorogiannis
Differential Revision: D19392696
fbshipit-source-id: e1b3c9a3a
Summary: In impurity analysis, pick up the pulse summary rather than re-analyzing. Re-order the checkers so that we first analyze pulse.
Reviewed By: jvillard
Differential Revision: D19448296
fbshipit-source-id: 2987fa848
Summary:
This diffs does: (1) move `get_formals` to `BufferOverrunUtils` (2) use separate `get_formals` in
`BufferOverrunChecker`, in order to simplify the following diff.
Reviewed By: jvillard
Differential Revision: D19432280
fbshipit-source-id: bfb4df118
Summary:
The restart scheduler will now have two phases:
1. Analyze all the procs obtained from the sources.
2. Run the FileScheduler on the sources.
The second step aims to analyze only the File level analyzers
requirements.
Reviewed By: ngorogiannis
Differential Revision: D19430244
fbshipit-source-id: b4f9ee69b
Summary: `dune build check` will compile all the ml files much quicker than `dune build`
Reviewed By: jvillard
Differential Revision: D19427864
fbshipit-source-id: 5221d32bc
Summary:
Introduce a new notion of equality for comparing abstract addresses in distinct threads:
```
(** Abstract address for a lock. There are two notions of equality:
- Equality for comparing two addresses within the same thread/process/trace. Under this,
identical globals and identical class objects compare equal. Locks represented by access paths
rooted at method parameters must have equal access paths to compare equal. Paths rooted at
locals are ignored.
- Equality for comparing two addresses in two distinct threads/traces. Globals and class objects
are compared in the same way, but locks represented by access paths rooted at parameters need
only have equal access lists (ie [x.f.g == y.f.g]). This allows demonically aliasing
parameters in *distinct* threads. This relation is used in [may_deadlock]. *)
```
Reviewed By: skcho
Differential Revision: D19347307
fbshipit-source-id: 9f338731b
Summary:
This diff avoids that `array_sizeof` returns bottom value when given Java enum values, which
introduced unreachable code inadvertently.
Reviewed By: ngorogiannis
Differential Revision: D19409077
fbshipit-source-id: 2816fd995
Summary: Access expressions can appear in casts, or sometimes other constructors, inside a `HilExp.t`. Extraction of the access expression can ignore those wrappers. Introduce a single function for doing that throughout the analyser.
Reviewed By: ezgicicek
Differential Revision: D19410673
fbshipit-source-id: a724cb466
Summary:
The property SkipAfterRemove already had a test, but not for
intra-procedural violations. This adds a test for that case.
Reviewed By: ngorogiannis
Differential Revision: D19330471
fbshipit-source-id: 1dd1c3ad7
Summary:
Now we can either disable it, or enable it only.
For integrations that disable it, this will allow to enable it for
NullsafeStrict classes without enabled it fully
Reviewed By: artempyanykh
Differential Revision: D19409131
fbshipit-source-id: a2b1fe650
Summary: This diff implements this for Field Not Initialized check
Reviewed By: artempyanykh
Differential Revision: D19393989
fbshipit-source-id: cf60e8d53
Summary: add subdirectories so that we can run each java file against its own topl properties
Reviewed By: rgrig
Differential Revision: D19347302
fbshipit-source-id: 562830774
Summary:
This diff does it for nullable dereference and assignment violations
rules which happen under NullsafeStrict case.
Follow up are to make the same for inheritance and field initializer
violations.
Possible follow up includes making error message more specific and
articulare this this is a nullsafe strict mode.
Reviewed By: artempyanykh
Differential Revision: D19392916
fbshipit-source-id: 2554ac7a7
Summary: In whole-program mode, analysing a method requires analysing first all constructors of the same class. This is not needed in normal mode, so gate that computation under `starvation_whole_program` for efficiency.
Reviewed By: artempyanykh
Differential Revision: D19393412
fbshipit-source-id: 2277e6b5e
Summary:
The RestartScheduler now uses it's own of_list function to build a
ProcessPool.TaskGenerator to be able to use a queue as the underlying
data structure.
Reviewed By: ngorogiannis
Differential Revision: D19390055
fbshipit-source-id: d57b493b7
Summary:
Previously, _override resolution_ considered only the number of
arguments. This led to many FPs in nullsafe's _Inconsistent Subclass
Annotation_ check.
Current version also checks that argument types match. However, we
still don't handle type parameters and erasure, so in this sense the
rules are incomplete.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19393201
fbshipit-source-id: a0c75b8dd
Summary:
That was used only by the now-defunct Java Buck integration without
genrules.
Reviewed By: ngorogiannis
Differential Revision: D19176545
fbshipit-source-id: 8253b614a
Summary:
You should use `--buck-java` instead, which uses the new "genrule
master" integration.
This diff makes it impossible to select the previous integration.
Upcoming diffs will clean up the resulting dead code.
This also make infer fail hard when no buck mode is specified in a buck
capture command, eg `infer -- buck build //foo:foo`. The reason is that
we need to choose between 3 incompatible integrations and making any of
them the default will confuse at least one person in the future.
Reviewed By: ngorogiannis
Differential Revision: D19176391
fbshipit-source-id: 707d18b50
Summary:
These changes introduce the RestartScheduler which for now is a copy of the FileScheduler:
- added it as a possible argument to the recently added `--scheduler` option.
- made the necessary changes in `InferAnalyze` to call it if it was chosen.
Reviewed By: ngorogiannis
Differential Revision: D19373505
fbshipit-source-id: 98f065057
Summary:
`diff` by default doesn't colorize its output, while `git diff`
does. Since we already depend on git it seems like a safe change and
brings some quality of life improvement while working with tests.
Reviewed By: ngorogiannis
Differential Revision: D19374691
fbshipit-source-id: 8872b527c
Summary:
`infer/tests/infer.make` unconditionally runs
```
$(call check_no_duplicates,$(INFER_OUT)/duplicates.txt)
```
as a part of `test` target. This requires the flag to be set to
produce `duplicates.txt`.
`clang.make` does this, but `java.make` was missing the flag which led
to an error on running `make tests` within
e.g. `tests/codetoanalyze/java/nullsafe-default`:
```
grep: infer-out/duplicates.txt: No such file or directory
```
Reviewed By: mityal
Differential Revision: D19373857
fbshipit-source-id: c2fcbe9dd
Summary:
This diff avoids that null-retuned path's abstract value ruins that of non-null-returned path.
What this diff does is: when joining two abstract states, one is null-return-path and the other is
non-null-return-path (`return obj;`), it keeps the method calls of `obj` from the
non-null-return-path.
While this design is unsound, I think it should work in practice.
Reviewed By: ezgicicek
Differential Revision: D19348313
fbshipit-source-id: cf5d0f3ff
Summary: This diff captures global initializers ondemand, like we do for functions defined in headers.
Reviewed By: ezgicicek
Differential Revision: D19346947
fbshipit-source-id: 05174e6a4
Summary: The type-name definition for Java can be potentially improved (eg increase sharing, or comparison speed, much like `QualifiedCppName`) by switching away from `Mangled.t` which is essentially a string. First step is to abstract the type.
Reviewed By: jberdine
Differential Revision: D19087508
fbshipit-source-id: 91a81f63b
Summary: Now that we have the kind of lock stored (global/class obj/path rooted at parameter), use it for comparison/equality, while ignoring the root variable of the access path, which is only used for printing.
Reviewed By: skcho
Differential Revision: D19346801
fbshipit-source-id: c65661dc6
Summary: Add command line option that directs racerd to treat all return values from unknown code (including abstract methods) as owned objects. This is essentially treating return values with full angelicism
Reviewed By: artempyanykh
Differential Revision: D19368375
fbshipit-source-id: 6a10153fa
Summary:
Java treats switch on nullables in a non-obvious way (throws an NPE
surprise) so lets have a decidated test exactly for this.
Reviewed By: ngorogiannis
Differential Revision: D19371280
fbshipit-source-id: d9867b6d6
Summary:
As part of enabling substitution of arguments into lock names in summaries, and potentially changing the equality relation over locks (so that we can eventually make equal `x.f.g` and `this.f.g` for example), the lock type needs to be elaborated so that the root of an access path is classified into:
- global variables are clearly separated from all other classes (invariant to substitution)
- class objects are also separate (also invariant, but identified by type)
- all other parameters remember their positional index (so that substitution becomes easier)
For now the comparison/equality is kept identical, so as to make easy CI comparisons.
Reviewed By: skcho
Differential Revision: D19232577
fbshipit-source-id: 0cd6a43db
Summary: Before going to a new lock representation that will allow, eg, substitutions with arguments for parameters on method calls, make the interface of the lock type abstract to ease the transition.
Reviewed By: mityal
Differential Revision: D19309359
fbshipit-source-id: 5277357ee
Summary:
Demonstrate that the per-file type environments don't prevent
the deadlock report here. The fear was that when the analyser
tries to locate the methods of the endpoint class, it might fail to
do so because the types might be stored in different type
environments (per file).
Reviewed By: mityal
Differential Revision: D19225908
fbshipit-source-id: 097e4aeea
Summary: This diff use actuall call path in the cost results instead of `class name + method name`.
Reviewed By: ngorogiannis
Differential Revision: D19194969
fbshipit-source-id: b72018586
Ezgi Çiçek
Nikos Gorogiannis
Artem Pianykh
Fernando Gasperi Jabalera
Mitya Lyubarskiy
Dulma Churchill
Jules Villard
Radu Grigore
Martin Trojer
Sungkeun Cho
Kfir Schindelhaim