Summary: Use_after_free was used both for biabduction and pulse, and the biabduction version is blacklisted by default. As a result, the Pulse version was also disabled unintentionally. This changes the name of the old use_after_free so that now we can get use_after_free bugs whenever pulse is enabled.
Reviewed By: skcho
Differential Revision: D17182687
fbshipit-source-id: 539ca69de
Summary:
In integrations where the capturing process isn't forked off the main Infer process, but launched, eg, via a script pretending to be a compiler, the reference indicating whether the server is running will always be false, and thus such integrations will never try to connect to the write daemon.
Fix this by
- making `sqlite-write-daemon` authoritative wrt connecting to the daemon.
- launching the daemon earlier in the setup process.
Reviewed By: jberdine
Differential Revision: D17204002
fbshipit-source-id: 23d452fac
Summary:
See motivation below.
This diff is dealing with FieldNotNullable:
- move not relevant subclasses into dedicated classes and files
- modify the tests so they comply with the standards below
--Motivation--
Gradual mode we are going to introduce is an invasive change in how Infer
treats nullability semantics.
In order to make the change in a controllable way, we need the tests to comply with the
following standards and conventions.
1. For each code peace where we expect a bug to happen, the there should be
corresponding (minimally different from above) peace of code where we expect a bug to NOT happen. (This is to ensure bug is happening for exact reason we think it is happening).
2. Conversely: for each peace of code where we expect a bug to be NOT
present, there shuold be a peace of code where the bug IS happening.
(Otherwise there can be too many reasons for a bug NOT to happen).
3. Convention: end corresponding methods IsOK and IsBUG correspondingly.
4. Keep code examples as small as possible.
Reviewed By: ngorogiannis
Differential Revision: D17183222
fbshipit-source-id: 83d03e67f
Summary:
It adds `typ` field in Sil.Load. The field will be used by the analyzer in the following diffs.
Motivation: Interbo generates a symbolic value when evaluating expressions including parameter symbols. At that time, it is done with depending on their types, e.g., an integer, a pointer to struct or a pointer to array. Without the type, it is hard to generate a correct symbolic value that will be instantiated later in call sites. Thus, evaluating RHS of the load statement, the type of RHS is better to be given.
Reviewed By: jvillard
Differential Revision: D17163350
fbshipit-source-id: f7f0f1429
Summary:
It uses inline record for Sil.Load and Sil.Store for preparing the
following extention.
Reviewed By: dulmarod
Differential Revision: D17161288
fbshipit-source-id: 637ea7bfa
Summary: It prints non-verbose program variables in the report.
Reviewed By: ngorogiannis
Differential Revision: D17163943
fbshipit-source-id: c3f3c2887
Summary:
An exception thrown during capture/analysis may leave the daemon
running. Kill it even when one is thrown.
Reviewed By: martintrojer
Differential Revision: D17181090
fbshipit-source-id: a7b002f23
Summary: With this predicate we are able to check for static global variables in AL.
Reviewed By: ddino
Differential Revision: D17164848
fbshipit-source-id: a3d10598c
Summary:
We currently use storage_class only for checking is_static, adding the flag instead in the plugin to improve perf by avoiding string comparisons.
update-submodule: facebook-clang-plugins
Reviewed By: ngorogiannis
Differential Revision: D17156173
fbshipit-source-id: 2b84a0b84
Summary:
In next diff, we are going to introduce a new mode of nullsafe
(gradual). For testing, we are going to employ the strategy used by jvillard
for Pulse.
In this diff we split tests into two subfolders, one for the default and one for the gradual
mode.
We are planning to make the gradual mode default eventually. For that, most
new features will make sense for gradual mode, and we will mostly evolve
tests for that mode.
As for 'default' mode, we need to preserve tests mostly to ensure we don't introduce
regressions.
Occasionally, we might make changes that make sense for both modes, in
this (expected relatively rare) cases we will make changes to both set
of tests.
An alternative strategy would be to have two sets of issues.exp files,
one for gradual and one for default mode. This has an advantage of each
java file to be always tested twice, but disadvantage is that it will be
harder to write meaningful test code so that it makes sense for both
modes simultaneously.
Reviewed By: ngorogiannis
Differential Revision: D17156724
fbshipit-source-id: a92a9208f
Summary:
This abstraction was not always used consistently.
Its usage made more sense when it supported both present annotations and
optional annotation (which got removed in previous diff).
The rought semantic of that was "what is the inferred type for such and
such value (variable or expression) in typestate". So it is not really
_annotation_ in first place, it is more like "what we inferred about
nullability given annotations, known special cases, and current sybmolic
execition state".
Let's explicitly rename `map` to `is_nullable`. If/when we need to
enhance this further (and we likely will), we will do it accordingly.
Reviewed By: jvillard
Differential Revision: D17153434
fbshipit-source-id: 3c85b56df
Summary:
`Present` annotation was an experiment made many years ago that never
got into real usage. The idea was to annotate Optional<> types with
Present, which means that it is safe to call get().
We don't plan to support `Present` annotation for optional types in the
near future.
Support of `Present` annotation requires extra levels of abstraction
that make the changing the behavior and introducing new features harder.
A lot of checks for nullability are written in generic way so they also
check for presense.
Getting rid of that will allow us to simplify our
work for introducing new semantics for nullsafe.
Reviewed By: ngorogiannis
Differential Revision: D17153432
fbshipit-source-id: c5ea9bdf1
Summary:
Implementation of write-serializer for Sqlite. Points of note:
- A Unix socket is used for communication. This avoids buffer-size limitations, as the objects we send for writing may exceed said limits.
- No daemon is used if running under buck or in genrule mode, as this usually means a single-threaded job capturing into the DB.
- When the daemon is running, read-only access is *not* enforced for other processes. This makes starting and stopping the daemon during Infer execution easier and more robust. In WAL mode this should not have any effect on performance.
- This version is not economical with connections, it uses one per query, todo.
Reviewed By: jvillard
Differential Revision: D17077183
fbshipit-source-id: fa9877d6c
Summary: Developing the Sqlite-writer process further, a type `command` is introduced, which will used for sending instructions down a communications channel to the daemon. For now, the commands are interpreted locally.
Reviewed By: skcho
Differential Revision: D16985056
fbshipit-source-id: 2aa20908d
Summary:
Write contention is becoming a problem in parallel capture (eg when make runs with high parallelism) or when analysis writes CFGs to the DB in parallel (eg when analysing blocks in ObC). This is believed to lead to BUSY errors in Sqlite.
This is step 1 of a process where all writes are cordoned-off in one module, and fixing the interface for that module.
Reviewed By: skcho
Differential Revision: D16985034
fbshipit-source-id: 3d7ce381b
Summary:
When running with high parallelism and a large number of insertions in the DB (eg, ObjC analysis with block specialisation), we see MISUSE exceptions thrown by Sqlite **when trying to bind parameters to queries**. It does not always occur, and maybe that's because the check in Sqlite that throws this error is documented as "probabilistic". For the same reason, it is plausible that high parallelism increases the chance of detection.
According to documentation this unequivocally means a bug in our usage of the API (https://www.sqlite.org/rescode.html#misuse), in particular that a parameter is re-bound while the query is running (https://www2.sqlite.org/cvstrac/wiki?p=LibraryRoutineCalledOutOfSequence). I believe this may have to do with `result_fold_rows` (as it's the only one that uses a query that can be continued, and thus misused), but I have not managed to track the bug.
Always resetting the query before using it is a defensive measure that seems to make these errors go away (and turn some of them to BUSY timeouts, which should be addressed by a write serialiser, but in any case it's a more logical state of affairs = higher parallelism means more contention thus possibly timeouts due to lock usage).
Reviewed By: jvillard
Differential Revision: D17147447
fbshipit-source-id: 7ef3cc73f
Summary:
Since it does not make sense to get ranges of non-integer values and
use them as approximate iteration numbers, this diff ignores control
values that only contain non-integer symbols.
Reviewed By: ezgicicek
Differential Revision: D17130967
fbshipit-source-id: f5ba58d52
Summary: This tests the previous commit D17093980, which moves incremental analysis to run before capture
Reviewed By: ngorogiannis
Differential Revision: D17113475
fbshipit-source-id: 702d967b3
Summary:
Currently, the specs directory is cleaned after running capture. This means that the `changed-files` are interpreted in the context of the second set of source files. Therefore if a procedure is deleted from the second set of source files, its specs file will not be deleted.
This moves the cleaning of the specs directory to before capture, to avoid this problem.
Reviewed By: ngorogiannis
Differential Revision: D17093980
fbshipit-source-id: e1a8d8a54
Summary: This diff extends size alias domain for keeping one more alias of a Java temporary variable.
Reviewed By: ezgicicek
Differential Revision: D16984082
fbshipit-source-id: 244bbd0ee
Summary: This diff ignores boundends when getting the value range.
Reviewed By: ezgicicek
Differential Revision: D17114363
fbshipit-source-id: cca8745e3
Summary: Like we removed empty edges from the `pre_heap` in D16419183, let's do the same to `post_heap`.
Reviewed By: skcho
Differential Revision: D17111336
fbshipit-source-id: c35fcbabb
Summary:
Before this diff we would record when some values came from the "address
of" logical variables. This makes no sense and also was incorrectly
marking these addresses as "written to" when they appeared in the post
of a procedure, because their attributes weren't empty (they had the
"address of stack variable" attribute).
Reviewed By: ngorogiannis
Differential Revision: D17131210
fbshipit-source-id: 6cc3c465a
Summary: When a positive bound is expected, min(1,x) can be simplified to 1.
Reviewed By: ezgicicek
Differential Revision: D17091884
fbshipit-source-id: 3a89a44fa
Summary:
This did not work. One can not create a param that depends on another param (dynamic!) value
```
infer --dynamic_dispatch
/Users/mityal/infer/infer/bin/infer: unknown option '--dynamic_dispatch'.
```
No info in the manual:
```
find . -name "*.txt" | xargs grep "dynamic"
```
Reviewed By: jvillard
Differential Revision: D17113568
fbshipit-source-id: 87d0a18ba
Summary:
I found it very confusing that running infer with --debug makes the
report to be different.
Intuitively, I expect (and I think majority of users would expect) that
`--debug` makes things more verbose (and potentially more slow / consuming
more memory and disk space), but does not change anything apart from it.
One pro of preserving existing behavior, pointed by jvillard:
- Suppose some check is experimental or disabled in the config. The
users expect the issue to be found, but it does not show up. They run
`infer --debug` to understand the behavior, and suddenly the issue shows
up.
I, hovewer, find this pro not important enough and potentially confusing
the users even more.
(If they want to investigate seriously, they can always use
--no-filtering, and there are a lot of cases when the issue does not
show up for others, much hard to undertand reasons, than the fact that
it is disabled).
Reviewed By: jvillard
Differential Revision: D17113750
fbshipit-source-id: 46cc93503
Summary:
The purpose of DefinitelyNotNullable currently is bit unclear; let's
rename it so that the intention is obvious.
Reviewed By: artempyanykh
Differential Revision: D16984529
fbshipit-source-id: 696d58315
Summary:
`nullsafe` currently allows the following:
```
public void Nonnull Object willBeOK() { return null; }
```
But disallows the following:
```
public void Object willBeAnIssue() { return null; }
```
This was a deliberate choice made back in 2014.
The motivation was to provide a way to tell the checker "I know it can not be null, trust me".
A huge problem with that approach is that it is extremely non-intuitive and surprising, and contradicts with pretty much everything when Nonnull or similar annotations are used in external world.
This is not the way how checkers should be supressed.
We do provide 2 options to express this intention, namely `assertNotNull` and `assumeNotNull` would do the thing.
This is a much better approach for additional reason: assertNotNull is
granular and applies only to the exact expression that is under
question. In contrast, suppressing the check on the whole function level
make any modifications of a function dangerous.
Reviewed By: artempyanykh
Differential Revision: D16984213
fbshipit-source-id: 0ba0f623b
Summary:
This diff revises some models of Java String.
They had been implemented by C's string models such as models of
`strlen` or `strcat`, however, Java's String is different to C's,
rather is similar to C++'s String object.
Reviewed By: ezgicicek
Differential Revision: D17093136
fbshipit-source-id: b4f2cb4d0
Summary: Numeric attribute ranks are getting confused with addresses. Add an option (false by default) to MakePPUniqRankSet which prevents printing of the ranks.
Reviewed By: jvillard
Differential Revision: D17094269
fbshipit-source-id: 353c52fca
Summary:
`from_string` is too benign in constrast with what this method is really
doing (and oh my what it is really doing).
There are a lot of potential follow ups to clean this up even more, but
this is beyond the scope of this diff
Reviewed By: jvillard
Differential Revision: D17070826
fbshipit-source-id: 3d190039e
Summary:
`__inferbo_empty`, `__inferbo_min`, and `__inferbo_set_size` were in the
"include-based" cpp model.
Reviewed By: jvillard
Differential Revision: D17072034
fbshipit-source-id: dd840331f
Summary:
This diff uses the models of vector for modelling string in Cpp.
Depends on D16963153
Reviewed By: ezgicicek
Differential Revision: D16963166
fbshipit-source-id: 5effe2d72
Summary:
This is more powerful than `"symbols"` for more advanced use-cases. Keep
`"symbols"` unchanged to make migrating easier.
Differential Revision: D16985756
fbshipit-source-id: dfbb09393
Summary: Adding new predicate for checking whether a variable is defined as extern. May be useful in AL rules.
Reviewed By: jvillard
Differential Revision: D16961690
fbshipit-source-id: 0677077dc
Summary:
The clang frontend has bugs. When a bug we know about happens some
exception is raised and, most of the time, logged away so as not to
crash the whole process. This catching of exceptions wasn't done from
testDeterminator so it could crash where capture didn't. This diff wraps
the crashy function in test determinator to avoid that.
Reviewed By: ngorogiannis
Differential Revision: D16963178
fbshipit-source-id: 87a4ff70b
Summary:
This isn't really a "config" part of the frontend and this change is
needed later to catch these errors more robustly.
Reviewed By: ngorogiannis
Differential Revision: D16963177
fbshipit-source-id: 293b23acf
Summary: This diff prunes array sizes in Java by adding the size alias on the `get_array_length` function calls.
Reviewed By: ezgicicek
Differential Revision: D16983501
fbshipit-source-id: a924af09d
Summary:
Rename some AL source files so they mention AL explicitly instead of
"cFrontend" which could be confused with the clang frontend itself.
Reviewed By: ezgicicek
Differential Revision: D16962539
fbshipit-source-id: 29237cd1c
Summary: AL makes for close to a third of the source files in clang/. Put the code in its own folder for clarity.
Reviewed By: ezgicicek
Differential Revision: D16962438
fbshipit-source-id: 3373e69b9
Summary:
This diff avoids that an integer value is pruned to the bottom by
comparing to a pointer.
For example, before this diff,
assume((int*)x == p);
assume((int*)x != p);
where x is an integer, x is pruned to the bottom in both of the assume
cases. So, there were some, unintentional and false, unreachable
code.
Depends on D16960199
Reviewed By: ezgicicek
Differential Revision: D16964735
fbshipit-source-id: 90a3c8c80
Summary:
It changes the order of StdBasicString and StdVector for easier
reviewing of the following diff.
Reviewed By: ezgicicek
Differential Revision: D16963153
fbshipit-source-id: 50325e4e1
Summary:
The access path format forced some weird patterns on this code, simplify
using the access expression structure.
Reviewed By: ezgicicek
Differential Revision: D16960660
fbshipit-source-id: e8faf619e
Dulma Churchill
Nikos Gorogiannis
Mitya Lyubarskiy
Sungkeun Cho
Phoebe Nichols
Ezgi Çiçek
Jules Villard