Summary: A stepping stone to have descriptive issue types for each kind of flow rather that lumping everything into `QUANDARY_TAINT_ERROR`.
Reviewed By: mbouaziz
Differential Revision: D6126690
fbshipit-source-id: a7230c0
Summary:
Before this change, analyses using HIL needed to pass `IdAcessPathMapDomain.empty` to abstract interpreter, and would get back the map as part of the post.
This is a confusing API and was a pain point for Dino in trying to use HIL.
This diff adds a HIL wrapper around the abstract interpreter that hides these details.
It replaces `LowerHIL.makeDefault` as the new "simplest possible way" to use HIL.
Reviewed By: jberdine
Differential Revision: D6125597
fbshipit-source-id: 560856b
Summary:
Looked at some problematic summaries and am noticing some common patterns.
Adding some dynamic checks to be run in debug mode in order to make sure my fixes for these patterns are real.
Reviewed By: jeremydubreil
Differential Revision: D5779593
fbshipit-source-id: 9de6497
Summary:
Install ocamlformat from github as part of `make devsetup`, and use it
for formatting OCaml (and jbuild) code.
Reviewed By: jvillard
Differential Revision: D6092464
fbshipit-source-id: 4ba0845
Summary: Sinks weren't being printed when passthroughs are empty (which, for now, is always). Oops!
Reviewed By: jvillard
Differential Revision: D6110164
fbshipit-source-id: 4488ab0
Summary: This will make it easier to generalize the checker to handling uninitialized struct fields.
Reviewed By: ddino
Differential Revision: D6099484
fbshipit-source-id: b9c534b
Summary:
Refactor `RegisterCheckers` to give a record type to checkers instead of a tuple type.
Print active checkers with their per-language information.
Improve the manual entries slightly.
Reviewed By: sblackshear
Differential Revision: D6051167
fbshipit-source-id: 90bcb61
Summary:
Whenever we see a use of a lock, infer that the current method can run in a multithreaded context. But only report when there's a write under a lock that can be read or written without synchronization elsewhere.
For now, we only infer this based on the direct usage of a lock; we don't assume a caller runs in a multithreaded context just because its (transitive) callee can.
We can work on that trickier case later, and we can work on smarter inference that takes reads under sync into account. But for now, warning on unprotected writes of reads that occur under sync appears to be too noisy.
Reviewed By: jberdine
Differential Revision: D5918801
fbshipit-source-id: 2450cf2
Summary:
This will allow most of the checkers, except the bi-abduction, to skip the analysis on the specialized clone of the methods used to handle dynamic dispatch. Doing this, we can run the bi-abduction analysis using:
infer -a checkers --biabduction
without risk of conflicts on the resolution of dynamic dispatch.
Reviewed By: sblackshear
Differential Revision: D6052347
fbshipit-source-id: 0c75bf3
Summary:
It looks like the old code for expanding access paths assumed that `FormalMap.get_formals_indexes` assumed the returned list tuples would be sorted by index, but it's actually sorted by var name.
As a consequence, formals might be expanded into the wrong actuals.
This diff fixes the problem by not relying on `get_formals_indexes`.
Reviewed By: jberdine
Differential Revision: D6056365
fbshipit-source-id: 09f3208
Summary:
This was a crutch from the days before ownership analysis.
We shouldn't need it anymore, and it was actually causing FP's because we were skipping analysis of `ImmutableList.builder()` and not understanding that the return value is owned.
Reviewed By: jeremydubreil
Differential Revision: D6035631
fbshipit-source-id: afa0ade
Summary:
9c7fc65 introduced a large performance regression, this diff eliminates it and a bit more.
Instead of constructing the quotiented access list map in a two-step process of first constructing a map of all accesses and then quotienting it, the quotiented map is constructed directly by using a coarser comparison function on keys. Partitioning the access map O(number of access paths) times, using an apparently expensive partition predicate, seems to be causing trouble based on rough profile data.
Reviewed By: da319
Differential Revision: D6005262
fbshipit-source-id: 077846c
Summary: Stack-allocated variables cannot be raced on in cpp as every thread has its own stack. At the beginning of the analysis we add ownership to the local variables.
Reviewed By: jberdine
Differential Revision: D6020506
fbshipit-source-id: 0a90a97
Summary: Now that we report write-write races involving more than one write, we need to improve the traces accordingly.
Reviewed By: jberdine
Differential Revision: D6026845
fbshipit-source-id: b1366dd
Summary: The may alias analysis relation that the thread safety analysis uses is very specific to Java and causes many false alarms for C++ code. This diff disables it when analyzing C++ code. Improving it to avoid false negatives is left for later.
Reviewed By: sblackshear
Differential Revision: D5974182
fbshipit-source-id: 9c7fc65
Summary:
The analyzer currently does not understand the control flow of
Singletons, which leads to false alarms. This diff is an unsound hack
that simply ignores any read or write accesses made when computing the
value of a singleton.
Reviewed By: sblackshear
Differential Revision: D5979639
fbshipit-source-id: 34caecb
Summary:
Model folly::SharedMutex lock and unlock operations, some
apache::thrift::concurrency::ReadWriteMutex operations, some
folly::RWSpinLock operations, and folly::MicroSpinLock operations.
Reviewed By: sblackshear
Differential Revision: D5974225
fbshipit-source-id: 19e2816
Summary:
Previously, annotating something ThreadSafe meant "check that it is safe to run all of this procedure's methods in parallel with each other" (including self-parallelization).
This makes sense, but it means that if the user writes no annotations, we do no checking.
I'm moving toward a model of inferring when an access might happen on a thread that can run concurrently with other threads, then automatically checking that it is thread-safe w.r.t to all other accesses to the same memory (on or off the current thread thread).
This will let us report even when there are no `ThreadSafe` annotations.
Any method that is known to run on a new thread (e.g., `Runnable.run`) will be modeled as running on a thread that can run in parallel with other threads, and so will any method that is `synchronized` or acquires a lock.
In this setup, adding `ThreadSafe` to a method just means: "assume that the current method can run in parallel with any thread, including another thread that includes a different invocation of the same method (a self race) unless you see evidence to the contrary" (e.g., calling `assertMainThread` or annotating with `UiThread`).
The key step in this diff is changing the threads domain to abstract *what threads the current thread may run in parallel with* rather than *what the current thread* is. This makes things much simpler.
Reviewed By: jberdine
Differential Revision: D5895242
fbshipit-source-id: 2e23d1e
Summary:
Indicate if read or write is protected, and do not print only the
field but also the object involved in the race.
Reviewed By: sblackshear
Differential Revision: D5974250
fbshipit-source-id: 351a576
Summary:
Expanding traces currently works in the following way:
Given a `TraceElem.Kind` `k` we want to report in `foo`, we look for a callee `C` of `foo` that has a `TraceElem.Kind` equal to `k` in its summary, grab the summary for `C`, then repeat until we bottom out.
This isn't very flexible: it insists on equality between `TraceElem.Kind`'s as the criteria for expanding a trace.
This diff introduces a new `matches` function for deciding when to expand a trace from a caller into a callee.
Clients that don't want strict equality can implement a fuzzier kind of equality inside this function.
I've gone ahead and done this for the trace elemes of thread-safety.
In the near future, equivalent access paths won't always compare equal from caller to callee, so we want to match their suffixes instead.
Reviewed By: jvillard
Differential Revision: D5914118
fbshipit-source-id: 233c603
Summary: Not using this for now, and it seems good to simplify the complex domain as much as we can.
Reviewed By: jberdine
Differential Revision: D5970233
fbshipit-source-id: a451503
Summary:
Inject a marker using a global variable in <iostream>, and whitelist it so that
the frontend translates it.
Use the marker in the SIOF checker to tell whether a file includes <iostream>.
If so, start the analysis of its methods assuming that the standard streams are
initialised.
Reviewed By: sblackshear
Differential Revision: D5941343
fbshipit-source-id: 3388d55
Summary:
The previous domain for SIOF was duplicating some work with the generic Trace
domain, and basically was a bit confused and confusing. A sink was a set of
global accesses, and a state contains a set of sinks. Then the checker has to
needlessly jump through hoops to normalize this set of sets of accesses into a
set of accesses.
The new domain has one sink = one access, as suggested by sblackshear. This simplifies
a few things, and makes the dedup logic much easier: just grab the first report
of the list of reports for a function.
We only report on the fake procedures generated to initialise a global, and the
filtering means that we keep only one report per global.
Reviewed By: sblackshear
Differential Revision: D5932138
fbshipit-source-id: acb7285
Summary:
The only language types we have are Java/Clang/Python. The unit of analysis is a source file, and you can't write a source file that mixes two or more of these languages (to the best of my knowledge).
This diff simplifies using the assumption that all procedures in a file are written in the same language.
Reviewed By: jeremydubreil
Differential Revision: D5886942
fbshipit-source-id: 88c3759
Summary:
The only language types we have are Java/Clang/Python. The unit of analysis is a source file, and you can't write a source file that mixes two or more of these languages (to the best of my knowledge).
This diff simplifies using the assumption that all procedures in a file are written in the same language.
Reviewed By: jeremydubreil
Differential Revision: D5886942
fbshipit-source-id: 8555a16
Summary: Only Eradicate uses this, no need to create it for every checker.
Reviewed By: jeremydubreil
Differential Revision: D5886775
fbshipit-source-id: 7242437
Summary:
A Java cluster checker currently defines a "cluster" as all of the procedures in the same class.
But the cluster checker actually knows about all the procedures defined in the same source file.
In some checkers (such as thread-safety), we want to aggregate results across classes in the same file, not just methods in the same class.
This refactoring leaves the behavior the same for now, but will make it easier to do this in the near future.
Reviewed By: jeremydubreil
Differential Revision: D5885896
fbshipit-source-id: 0815fca
Summary: Handling the utility functions for asserting that we're on background thread.
Reviewed By: jberdine
Differential Revision: D5863435
fbshipit-source-id: 3ad95b5
Summary:
Previously, we just tracked a boolean representing whether we were possibly on the main thread (true) or definitely not on the main thread (false).
In order to start supporting `Thread.start`, `Runnable.run`, etc., we'll need something more expressive.
This diff introduces a lattice:
```
Any
/ \
Main Background
\ /
Unknown
```
as the new threads domain. The initial value is `Unknown`, and we introduce `Main` in situations where we would have introduced `true` before.
This (mostly) preserves behavior: the main difference is that before code like
```
if (*) {
assertMainThread()
} else {
x.f = ...
}
```
would have recorded that the access to `x.f` was on the main thread, whereas now we'll say that it's on an unknown thread.
Reviewed By: peterogithub
Differential Revision: D5860256
fbshipit-source-id: efee330
Summary:
It's useful to be able to disable de-duplication on the command line with `--no-filtering`.
Gate de-duplication with `Config.filtering` and move the de-duplication tests to a new directory under the build systems tests.
Reviewed By: jeremydubreil
Differential Revision: D5865329
fbshipit-source-id: 5094f5b
Summary:
Suggesting to add `_Nullable` on the fields checked for, or assigned to, `nullptr` will allow the biabduction analysis to report null dereferences that are related to the lifetime of objects.
Depends on D5832147
Reviewed By: sblackshear
Differential Revision: D5836538
fbshipit-source-id: c1b8e48
Summary: That match branch is Java-only but could be reached with a C++ method, causing a crash.
Reviewed By: jberdine, mbouaziz
Differential Revision: D5814041
fbshipit-source-id: 6b1b501
Summary: It's useful to be able to configure both max depth and max width
Reviewed By: jeremydubreil
Differential Revision: D5801567
fbshipit-source-id: 0138cd7
Summary:
Races on the internal implementation of data structures defined in
system headers are currently not detected, since the memory accesses
are in procedures that are not analyzed.
This diff adds models for a few std::map operations that indicate if
they read or write the underlying representation.
Reviewed By: da319
Differential Revision: D5804293
fbshipit-source-id: 55ff28c
Summary:
Read the documentation and it doesn't seem like these functions are guaranteed to choose the same value in different runs.
I hypothesize that these may be the source of flakiness in the thread-safety tests/smoke tests.
Reviewed By: jvillard
Differential Revision: D5794384
fbshipit-source-id: 02b7a96
Summary:
We supported globals as sources before, but we did so by allowing ClangTrace etc. to match against any access path in the footprint of the trace.
This is very powerful/flexible, but it's ultimately not a good idea because it leads to traces that are hard to read.
This is because a footprint source doesn't have any information about its provenance: we might know that the value came from a global, but we don't know where the read occurred.
The mechanism for handling procedure calls as sources already knows how to solve this problem.
This diff implements globals as sources as a special case of procedure call sources instead.
This will give us much nicer traces with full provenance of the read from the global.
Reviewed By: mbouaziz
Differential Revision: D5772299
fbshipit-source-id: 491ae81
Summary:
When a lambda has an `auto` parameter, the inferred type of the parameter because part of the name.
Our heuristic for identifying lambda was checking if the lambda's name was exactly `operator()`, which won't catch this case.
Reviewed By: jvillard
Differential Revision: D5753323
fbshipit-source-id: 85ff75a
Summary:
- failwith police: no more `failwith`. Instead, use `Logging.die`.
- Introduce the `SimpleLogging` module for dying from modules where `Logging`
cannot be used (usually because that would create a cyclic dependency).
- always log backtraces, and show backtraces on the console except for usage errors
- Also point out in the log file where the toplevel executions of infer happen
Reviewed By: jeremydubreil
Differential Revision: D5726362
fbshipit-source-id: d7a01fc
Summary:
I'm working on parameterizing access trees with a config that will limit max depth + perhaps width if needed.
This is a stepping stone to enforcing max depth.
Reviewed By: jvillard
Differential Revision: D5693453
fbshipit-source-id: c15b0ee
Summary:
Rather than printing the footprint using its actual representation (an access trie with bool nodes), print it as a set of access paths.
This makes it easier to read sources in Quandary specs/debug Quandary.
Reviewed By: jeremydubreil
Differential Revision: D5682630
fbshipit-source-id: ac55b7f
Summary:
Add `is_empty` to `AbstractDomain.WithBottom` sig and use the empty checks for nicer printing of access trees: don't print empty nodes/traces.
This should make it easier to debug Quandary; it's pretty hard to stare at an access tree and see what's going on right now.
Reviewed By: jberdine
Differential Revision: D5682248
fbshipit-source-id: 56d2a9d
Summary:
This simplifies the jbuild files: no need to list these files explicitly
anymore, nor to exclude them explicitly from the main `InferModules` library
(due to their different compilation flags).
Isolate common parts into jbuild.common do `cat`-based code inclusion into
jbuild files to factorize code.
Reviewed By: jberdine
Differential Revision: D5678328
fbshipit-source-id: 6d7d925
Summary:
We now represent the footprint with an access trie, so this code is no longer required.
This lets us simplify things a bit
Reviewed By: jberdine
Differential Revision: D5664484
fbshipit-source-id: c35edf2
Summary:
In looking at summaries that Quandary took a long time to compute, one thing I notice frequently is redundancy in the footprint sources (e.g., I might see `Footprint(x), Footprint(x.f), Footprint(x*)`).
`sudo perf top` indicates that joining big sets of sources is a major performance bottleneck, and a large number of footprint sources is surely a big part of this (since we expect the number of non-footprint sources to be small).
This diff addresses the redundancy issue by using a more complex representation for a set of sources. The "known" sources are still in a set, but the footprint sources are now represented as a set of access paths (via an access trie).
The access path trie is a minimal representation of a set of access paths, so it would represent the example above as a simple `x*`.
This should make join/widen/<= faster and improve performance
Reviewed By: jberdine
Differential Revision: D5663980
fbshipit-source-id: 9fb66f8
Summary:
The previous widening operator added stars to the *end* of paths that existed in `next` but not `prev`. This is not enough to ensure termination in the case where the trie is growing both deeper and wider at the same time.
The newly added test demonstrates this issue. In the code, there's an ever-growing path of the form `tmp.prev.next.prev.next...` that wasn't summarized by the previous widening operator. The new widening is much more aggressive: it replaces *any* node present in `next` but not `prev` with a `*` (rather than trying to tack a star onto the end). This fixes the issue.
This issue was causing divergence on tricky doubly-linked list code in prod.
Reviewed By: jeremydubreil
Differential Revision: D5665719
fbshipit-source-id: 1310a92
Summary:
Instead of a whitelist and blacklist and default issue types and default
blacklist and filtering, consider a simpler semantics where
1. checkers can be individually turned on or off on the command line
2. most checkers are on by default
3. `--no-filtering` turns all issue types on, but they can then be turned off again by further arguments
This provides a more flexible CLI and is similar to other options in the infer
CLI, where "global" behaviour is generally avoided.
Dynamically created checkers (eg, AL linters) cause some complications in the
implementation but I think the semantics is still clear.
Also change the name of the option to mention "issue types" instead of
"checks", since the latter can be confused with "checkers".
Reviewed By: jberdine
Differential Revision: D5583238
fbshipit-source-id: 21de476
Summary:
We previously lumped ownership predicates in with all other predicates. That limited us to a flat ownership domain.
This diff separates out the ownership predicates so we can have a richer lattice of predicates with each access path.
This lets us be more precise; for example, we can now show that
```
needToOwnBothParams(Obj o1, Obj o2) {
Obj alias;
if (*) { alias = o1; } else { alias = o2; }
alias.f = ... // both o1 and o2 need to be owned for this to be safe
}
void ownBothParamsOk() {
needToOwnBothParams(new Obj(), new Obj()); // ok, would have complained before
}
```
is safe.
Reviewed By: jberdine
Differential Revision: D5589898
fbshipit-source-id: 9606a46
Summary: Useful for identifying user-controlled array accesses that could lead to buffer overflows
Reviewed By: mbouaziz
Differential Revision: D5520985
fbshipit-source-id: 92984f6
Summary:
Record the list of access paths (if any) used in the index expression for each array access.
This will make it possible to use array accesses as sinks in Quandary
Reviewed By: jeremydubreil
Differential Revision: D5531356
fbshipit-source-id: 8204909
Summary:
It's nice to have "raw" as the default kind of access path, since it's used much more often than the abstraction.
This is also a prereq for supporting index expressions in access paths, since we'll need mutual recursion between accesses and access paths.
Reviewed By: jeremydubreil
Differential Revision: D5529807
fbshipit-source-id: cb3f521
Summary:
Works the same way as read/write races on fields, except that are more relaxed (er, unsound) in deciding whether two containers may alias.
This is needed to avoid reporting a ton of FP's; full explanation in comments.
Reviewed By: da319
Differential Revision: D5493404
fbshipit-source-id: 0a5d8b1
Summary:
The way we represented container writes before was pretty hacky: just use a dummy field for the name of the method that performs the container write.
This diff introduces a new access kind for container writes that is much more structured.
This will make it easier to soundly handle aliasing between containers and support container reads in the near future.
Reviewed By: da319
Differential Revision: D5465747
fbshipit-source-id: e021ec2
Summary: This gives us more expressive power when defining sources--we can use heuristics like "`foo(o)` only returns a source when `o` is not a constant".
Reviewed By: jvillard
Differential Revision: D5467935
fbshipit-source-id: f3d581d
Summary:
Pretty basic: warn when we see an assignment instruction `x = ...` and `x` is not live in the post of the instruction.
Only enabled for Clang at the moment because linters already warn on this for Java. But we can enable it later if we want to (should be fully generic).
Reviewed By: jeremydubreil
Differential Revision: D5450439
fbshipit-source-id: 693514c
Summary: Making it simple to add a new access type for "un-annotated interface call" in an upcoming diff.
Reviewed By: da319
Differential Revision: D5445914
fbshipit-source-id: f29e342
Summary:
This just makes the warnings silent for now. We may improve the analysis to check if the null check on the captured fields are consistent with the annotation on the corresponding parameters.
Eradicate also has the same issue. I added a test to outline this. The biabduction analysis will also probably fail on the same of annotation lookup. We may want implement the proper fix at the level of `Annotation.field_has_annot`.
Reviewed By: sblackshear
Differential Revision: D5419243
fbshipit-source-id: 6460de8
Summary: This will allow us to gradually get rid of the exceptions thrown during the analysis while detecting the regressions earlier
Reviewed By: jberdine, jvillard
Differential Revision: D5385154
fbshipit-source-id: 605e3f5
Summary:
Conversion and reformat of infer source using ocamlformat
auto-formatting tool.
Current status:
- Because Reason does not handle docstrings, the output of the
conversion is not 'Warning 50'-clean, meaning that there are
docstrings with ambiguous placement. I'll need to manually fix
them just before landing.
Reviewed By: jvillard
Differential Revision: D5225546
fbshipit-source-id: 3bd2786
Summary:
They are expected to occur in C++ code, so don't fail on them. For now
just skip them, although a better treatment of dynamic dispatch may be
needed later.
Reviewed By: da319, mbouaziz
Differential Revision: D5292462
fbshipit-source-id: 4285514
Summary: Looks much less confusing when C++ templates with `<stuff>` are involved.
Reviewed By: mbouaziz
Differential Revision: D5255551
fbshipit-source-id: f4a93e6
Summary:
After D5245416 I was taking a closer look and decided it's best to get rid of the `Interprocedural` module altogether.
Since jeremydubreil's refactoring to pass the summaries around everywhere, this module doesn't do much (it used to make sure the summary actually got stored to disk).
Client code is shorter and simpler without this module.
Reviewed By: mbouaziz
Differential Revision: D5255400
fbshipit-source-id: acd1c00
Summary: The docs for this said that it stores the summary to disk, which is no longer true. `compute_summary` is more descriptive of what it actually does now.
Reviewed By: jberdine
Differential Revision: D5245416
fbshipit-source-id: f5138cd
Summary: We had a model for `Pools.SimplePool`, but were missing models for `Pools.Pool`. Since `SimplePool` and `SynchronizedPool` both extend `Pool`, modeling it should cover all of the cases.
Reviewed By: ngorogiannis
Differential Revision: D5236280
fbshipit-source-id: 9bbdb25
Summary:
The ThreadSafety analysis currently reports on methods only if some
class in the file defining the method is annotated ThreadSafe, or
if it is called by some other such method call. Conflating files and
classes is a bit of a Javaism that seems to be unnecessary.
Reviewed By: sblackshear
Differential Revision: D5182319
fbshipit-source-id: aa77754
Summary:
:
No longer use deprecated reporting function for the suggest nullable checker
Depends on D5205009
Reviewed By: grievejia
Differential Revision: D5205843
fbshipit-source-id: f6dd059
Summary:
Now that we can run several inter-procedural analyses at the same time, we should no longer use the function `Reporting.log_error_deprecated` as it logs the errors in the specs table. This specs table is normally used for caching and will be deprecated in favor of having a cache summaries for the callees in the `Ondemand` module (to avoid deserialising a callee more than once within the same process).
This revision just renames the reporting functions.
Reviewed By: sblackshear
Differential Revision: D5205009
fbshipit-source-id: b066549
Summary:
Read/write race errors should always show one trace for a read and one trace for a write.
We forget to pass the conflicting writes to the reporting function in one case, which prevented us from showing a well-formed trace.
Fixed it by making the `conflicts` parameter non-optional
Reviewed By: jberdine
Differential Revision: D5209332
fbshipit-source-id: 05da01a
Summary: These can be useful in other checkers that have a notion of footprint.
Reviewed By: jvillard
Differential Revision: D5189193
fbshipit-source-id: c5bd91b
Summary:
First step toward addressing bad traces that happen in examples like
```
void sourceMethod() {
Obj source = (Obj) InferTaint.inferSecretSource();
callSameSink(null, source); // index: 1
}
void callSameSink(Obj o1, Obj o2) {
callMySink1(o1); // flows via o1 ~= index 0, don't expand
callMySink2(o2); // flows via o2 ~= index 1, can expand
}
void callMySink1(Obj o) {
... // maybe interesting something happens here that doesn't happen in callMySink2
InferTaint.inferSensitiveSink(o); // flows via o ~= index 0, can expand
}
void callMySink2(Obj o) {
InferTaint.inferSensitiveSink(o); // flows via o ~= index 0, can expand
}
```
The issue is that when we recreate a trace to the sink starting from `sourceMethod`, we don't know which of the calls to `callMySink` to expand/include in the trace.
If we expand the call to `callMySink(o1)`, we'll get a bogus trace.
In this example that's not such a big deal, but imagine the case where the first call to `callMySink` is a different function that transitively calls the sink through some long and confusing path.
Remembering the index at which taint flows into each sink will let us choose which sinks are safe to expand.
This diff just adds indexes to the API; it's not actually propagating the index info or using it during expansion yet.
Reviewed By: jeremydubreil
Differential Revision: D5170563
fbshipit-source-id: ba4b096
Summary:
Change the API of `Logging` wrt to writing to files and to the console (see
changes in logging.mli).
Write only to one log file: infer-out/log. Prefix each line with the kind of
warning and the PID of the process emitting it. Writing with `O_APPEND` is
atomic so the file should not get garbled by concurrent writes. To get the
output of a single process, find out which one interests you by looking at
infer-out/log, then `grep ^[<PID>] infer-out/log`.
Introduce 3 log levels for debug output and command-line options to set them
for various categories individually.
Change tons of `"\n"` to `"@\n"` so the `Format` module is aware of newlines
without us having to look through every character of every logged string for
`\n` characters.
Reviewed By: mbouaziz
Differential Revision: D5165317
fbshipit-source-id: 93c922f
Summary:
This makes it clearer that something went wrong. Most `failwith` did not set
this prefix already, so I opted to append it automatically and remove it from
the few instances that added it manually.
Also add quotes around bad user arguments to lessen possible confusion.
Reviewed By: jberdine
Differential Revision: D5182272
fbshipit-source-id: 20e4769
Summary:
This is a refactoring diff to put the info into the abstract domain
to track when we have done steps which would invalidate "I think I have a proof".
Subsequent diffs will start manipulating ThumbsUpDomain
Reviewed By: sblackshear
Differential Revision: D5172181
fbshipit-source-id: 51ceba6
Summary: We were almost always using `~report_reachable:true`, and in the cases where we weren't it is fine to do so. In general, a sink could read any state from its parameters, so it makes sense to complain if anything reachable from them is tainted.
Reviewed By: mbouaziz
Differential Revision: D5169067
fbshipit-source-id: ea7d659
Summary:
This was a subtle one. The ranking function of `aux` is the cardinality of `m`..
But if `may_alias` is not reflexive, then `k_part` will be empty, `non_k_part` will be the same size, and we'll diverge.
Sneakily, `may_alias` is actually *not* reflexive because `is_subtype t1 t2` doesn't check for the equality of `t1` and `t2`.
That is confusing and should be fixed separately.
For now, just make sure `may_alias` is always reflexive and add an assertion that `k_part` is never empty.
Reviewed By: jeremydubreil
Differential Revision: D5177427
fbshipit-source-id: 0549d6a
Summary:
This is a minimal change to (poorly) recognize and model std::mutex
lock and unlock methods, and to surface all thread safety issues for
C++ based on the computed summaries with no filtering.
This ignores much of the Java analysis, including everything about the
Threads domain. The S/N is comically low at this point.
Reviewed By: sblackshear
Differential Revision: D5120485
fbshipit-source-id: 0f08caa
Summary:
ThreadSafety.may_alias crashed on C++ code because it assumed Java
field names.
Reviewed By: sblackshear
Differential Revision: D5147284
fbshipit-source-id: d10841f
Summary: Using Conjunction for thread join has known false negatives. Finer grained recording of threading information fixes this.
Reviewed By: sblackshear
Differential Revision: D5111161
fbshipit-source-id: aab483c
Summary: The debug HTML for Quandary/thread-safety was still printing the SIL instructions, which is not very helpful. Print the HIL instructions instead.
Reviewed By: jeremydubreil
Differential Revision: D5112696
fbshipit-source-id: a0aa925
Summary: Gflags is a popular library used to create command line arguments. Flags shouldn't flow directly to `exec` etc.
Reviewed By: jvillard, mbouaziz
Differential Revision: D5058393
fbshipit-source-id: ab062f8
Summary: There was no option to trigger this checker so it was not possible to enable it when not running the default list of checkers
Reviewed By: jberdine
Differential Revision: D5057088
fbshipit-source-id: 7af36f5
Summary:
An array has a static or dynamic length (number of elements), but it also has a
stride, determined by the type of the element: `sizeof(element_type)`. We don't
have a good `sizeof()` function available on SIL types, so record that stride
in the array type.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4969697
fbshipit-source-id: 98e0670
Summary:
With this change, running the biabduction analysis with
infer -a infer -- ...
or with:
infer -a checkers --biabduction -- ...
take the same time and give the same list of results.
Reviewed By: sblackshear
Differential Revision: D5026676
fbshipit-source-id: ef23911
Summary: A lot of C++ library functions look like this, so it's important to have.
Reviewed By: mbouaziz
Differential Revision: D5026082
fbshipit-source-id: 6f421b6
Summary:
Now,
infer -a infer -- ...
and
infer -a checkers --biabduction -- ...
will return the same list of errors
Reviewed By: sblackshear
Differential Revision: D5023223
fbshipit-source-id: f52ce5d
Summary:
Ran the build with -w,-32 , delete code, repeat, until a fixpoint of no more warnings is reach.
Unfortunately we cannot fatal on w32 because ppx_compare can generate dead code (eg `compare_t` and only `compare` is used).
Reviewed By: mbouaziz
Differential Revision: D4945800
fbshipit-source-id: c95afb6
Summary:
The Siof checkers can now be run with:
infer -a checkers --siof -- ...
and also runs by default using:
infer -a checkers -- ...
Reviewed By: jberdine
Differential Revision: D5009731
fbshipit-source-id: e0e2168
Summary:
First step to be able to enable and disable the checkers to run in the following form:
> infer -a checkers --checker1 --checker2 --checker3 -- ...
and have a predefined list of checkers that are run by default with:
> infer -a checkers -- ...
Reviewed By: sblackshear
Differential Revision: D5007377
fbshipit-source-id: d7339ef
Summary:
While working on making the AI framework simpler to use, it become hard to change the shared API while keeping these unused checkers compiling, and even harder to keep their functionality since there is no tests for them.
Also, these checkers are not useful as proof of concept since using the AI framework is the preferred way to write new kinds of analysis now.
Reviewed By: sblackshear
Differential Revision: D4999387
fbshipit-source-id: 497284b
Summary:
This gives the option to run the biabduction analysis together with the other Clang-based checkers with the command:
infer -a checkers --biabduction -- ...
The filtering does not work yet because the filtering for the biabduction analysis matches the analyzer `Infer`, and does not filter much when the analyzer is `Checkers`, which is the case here.
Reviewed By: sblackshear
Differential Revision: D4773834
fbshipit-source-id: 16300cc
Summary:
Last step for converting thread-safety and quandary to HIL.
Push the logic for managing the id map and converting the instructions into a functor.
This way, client analyses can simply write HIL transfer functions and call the functor.
Reviewed By: jberdine
Differential Revision: D4989987
fbshipit-source-id: 485169e
Summary: The name of the source file was passed around everywhere but can also be accessed from the location associated to every node.
Reviewed By: sblackshear
Differential Revision: D4981848
fbshipit-source-id: 2ee592e
Summary:
Before we understood ownership, we needed this to avoid a mountain of Builder-related FP's.
Now that we have fairly sophisticated understanding of ownership, we can kill this hack.
Reviewed By: jaegs
Differential Revision: D4940238
fbshipit-source-id: 8d86e57
Summary: Before, running any of these would crash with `Unsupported infer analyzer with Buck flavors:`
Reviewed By: jberdine
Differential Revision: D4970769
fbshipit-source-id: 76be6d5
Summary:
Modify the type of `Exp.Sizeof ...` to include the value that the expression
evaluates to according to the compiler, or None if it cannot be known
statically.
Use this information in inferbo.
Mostly unused in the BiAbduction checker for now, although it could be useful
there too.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4953634
fbshipit-source-id: be0999d
Summary: The purpose of the annotation reachability analysis is to report when a method annotated with `X` never calls, directly or indirectly, another method annotated with `Y`. However, there can be different call stacks following different execution paths from `X` to `Y`. Reporting more than one call stack ending with the same annotated procedure does not bring more signal to the end user. So the purpose of this diff is to avoid those duplicated reports and report at most one annotation reachability issue per end of call stack.
Reviewed By: sblackshear
Differential Revision: D4942765
fbshipit-source-id: 46325a7
Summary: The analysis logic was split between the treatment of the instructions and the definition of the domain, making the code more complicated that it should. This diff moves more of the logic into the domain definition and change to variable names to more descriptive ones
Reviewed By: sblackshear
Differential Revision: D4936414
fbshipit-source-id: ff59de7
Summary:
As an interprocedural checker, SIOF should not run unless explicitly required.
Make it a new type of analyzer like other similar checkers.
Reviewed By: mbouaziz
Differential Revision: D4937820
fbshipit-source-id: a9e2d38
Summary: Sawja assigns them on multiple control-flow paths, so they're not SSA.
Reviewed By: peterogithub
Differential Revision: D4896745
fbshipit-source-id: c805216
Summary:
There are false positives in the current analysis due to the
use of conjunction in the treatment of threaded. Changing conjunction to disjunction
removes these false positives. Some new false negatives arise, but all the old tests pass.
This is a stopgap towards a better solution being planned.
Reviewed By: sblackshear
Differential Revision: D4883280
fbshipit-source-id: c2a7e6e
Summary:
Limit the use of `SourceFile.invalid` (renamed from `SourceFile.empty`) as much
as possible. In particular, do not generate bogus procnames for external global
variables: their translation unit was set to the invalid source file, now we
distinguish between extern/non-extern global variables more explicitly.
`SourceFile.invalid` is still used in too many places to actually remove it, often as a dummy initial value that never gets used, but sometimes as an actual value... Worse, we cannot fail on all operations on `SourceFile.Invalid` yet: the `SourceFile.to_string` method is used in too many places where it could get `SourceFile.Invalid` as argument. It's easy to see where it's used by making it raise in the code, then running the tests. This results in spaghetti backtraces that are hard to trace back to a root cause.
Reviewed By: akotulski, jeremydubreil
Differential Revision: D4860019
fbshipit-source-id: 45be040
Summary:
It's distracting to see the debug HTML for the preanalysis when you're trying to debug something else.
Also, it breaks the nice bi-abduction debug feature of marking the visited nodes as green.
Reviewed By: akotulski
Differential Revision: D4858578
fbshipit-source-id: 8e77976
Summary: This should make the reports much easier to understand. We can generalize to reporting a stack trace for all of the writes in the future if we wish.
Reviewed By: peterogithub
Differential Revision: D4845641
fbshipit-source-id: 589fdbc
Summary: Prereq for reporting a call stack for both the read and write in a read/write race.
Reviewed By: peterogithub
Differential Revision: D4845603
fbshipit-source-id: ebfeb9b
Summary: If two public methods touch the same state and only one is marked `ThreadSafe`, it's reasonable to report unsafe accesses on both of them.
Reviewed By: peterogithub
Differential Revision: D4785038
fbshipit-source-id: 5a80da4
Summary: This will avoid the redefine this Map and Set module as pretty printable when used to create abstract domains.
Reviewed By: sblackshear
Differential Revision: D4811849
fbshipit-source-id: e2f6763
Summary:
*Unless* the unprotected write runs on the main thread and the read doesn't.
Otherwise, we'll already report on the unprotected write, and we don't want to duplicate.
Reviewed By: peterogithub
Differential Revision: D4798357
fbshipit-source-id: 5de06a0
Summary:
Otherwise, we can get an exception when calling `Fieldname.java_get_field`.
Thanks to ngorogiannis for reporting.
Reviewed By: jeremydubreil
Differential Revision: D4805197
fbshipit-source-id: 3141bb1
Summary:
This is step further simplify the code to avoid cases where the summary of the procedure being analyzed can exist in two different versions:
# one version is the summary passed as parameter to every checker
# the other is a copy of the summary in the in-memory specs table
This diff implements:
# the analysis always run through the `Ondemand` module (was already the case before)
# the summary of the procedure being analyzed is created at the beginning of the on-demand analysis call
# all the checkers run in sequence, update their respective part of the payload and log errors to the error table
# the summary is store at the end of the on-demand analysis call
Reviewed By: sblackshear
Differential Revision: D4787414
fbshipit-source-id: 2d115c9
Summary:
This checker was always running by default but was apparently never reporting.
This checkers can always be run using:
infer -a checkers --checkers-repeated-calls -- ...
Reviewed By: sblackshear
Differential Revision: D4782472
fbshipit-source-id: 5ec77f4
Summary:
Add support for Makefiles to the copyright linter. Makefiles are a bit
different than shell because they should start with the copyright notice
straight away (whereas shell starts with the #! stuff).
Reviewed By: mbouaziz
Differential Revision: D4786620
fbshipit-source-id: 504dc23
Summary: Bringing the logic back to where it was before the big refactoring of the reporting logic.
Reviewed By: peterogithub
Differential Revision: D4774541
fbshipit-source-id: afeaaf8
Summary: We only need one "global" view of all the summaries in a file.
Reviewed By: peterogithub
Differential Revision: D4773646
fbshipit-source-id: 29e5316
Summary:
Move all of the reporting on top of the aggregation functionality.
This lets us delete lots of code
Reviewed By: peterogithub
Differential Revision: D4772223
fbshipit-source-id: 47cc51a
Summary:
This was the one type of races we were not yet reporting (besides ones that use the wrong synchronization :)).
Wrote new utility function to aggregate all accesses by the memory they access.
This makes it easy to say which accesses we should report and what their conflicts are.
Eventually, we can simplify the reporting of other kinds of unsafe accesses using this structure.
Reviewed By: peterogithub
Differential Revision: D4770542
fbshipit-source-id: 96d948e
Summary:
We can simplify the code now that the procedure callback are always executed through Ondemand. The procedure callback is still registered for Ondemand analysis by the time we run the cluster callbacks. This allows to run allows to run `Summary.read_summary`, which may run the analysis on-demand, while collecting the summaries for reporting errors.
This allows further simplifications of the Ondemand API.
Reviewed By: sblackshear
Differential Revision: D4764251
fbshipit-source-id: d0bdda4
Summary: This was annoying as "jump to next error" was otherwise always jumping to this warning about shadowing `|>`
Reviewed By: sblackshear
Differential Revision: D4767571
fbshipit-source-id: 932145c
Summary:
For collections whose type does not express that the collection is thread-safe (e.g., `Collections.syncrhonizedMap` and friends).
If you annotate a field holding one of these collections, we won't warn when you mutate the collection.
Reviewed By: jeremydubreil
Differential Revision: D4763565
fbshipit-source-id: 58b487a
Summary: It seems that we were not really using the `Bottom` part of the domain as a pair of (empty call map, empty tracking var map) was already acting as bottom.
Reviewed By: sblackshear
Differential Revision: D4759757
fbshipit-source-id: 53dedfe
Summary: This call is redundant and is already done in `AbstractInterpreter`
Reviewed By: sblackshear
Differential Revision: D4754251
fbshipit-source-id: af2d11e
Summary:
If I read off the main thread and write on the main we
could have a race. (Writes off main are already reported.)
Reviewed By: sblackshear
Differential Revision: D4746138
fbshipit-source-id: 8b6e9c5
Summary:
Split Fieldname.t into `Java` and `Clang`. Each of them have different naming conventions and this way it's easier to differentiate between them.
Make `Java` variant store string instead of mangled since mangled part was always empty
Changes to `Clang` variant are coming in the next diff
Reviewed By: jeremydubreil
Differential Revision: D4746708
fbshipit-source-id: c5858a8
Summary:
Reorganize by using a top-level iteration over the access map and using a helper function for updating the caller accesses.
The new code is shorter and much more readable.
Reviewed By: peterogithub
Differential Revision: D4740657
fbshipit-source-id: 8e18cd5
Summary: Add `QualifiedCppName.t` and some functions to manipulate it. More places will start using this type (such as `Procnames` or `Typ.Name`) in later diff
Reviewed By: jberdine
Differential Revision: D4738991
fbshipit-source-id: 8f20dd6
Summary: Now that all the checkers are now run in a way that will prevent conflicts between them, we can make this change that was breaking the analysis.
Reviewed By: jvillard
Differential Revision: D4621953
fbshipit-source-id: f17c729
Summary:
One limitation of Eradicate is that certain nullability patterns are not expressible using simply the `Nullable` annotation.
One such pattern is using the knowledge that a function returns null when passed null, but returns an object otherwise.
The annotation `PropagatesNullable` is a variant of `Nullable` applied to parameters when their value propagates to the return value.
A method annotated
```
B m(PropagatesNullable A x) { return x == null ? x : B(x); }
```
indicates that `m` returns null if `x` is null, or an object of class `B` if the argument is not null.
Examples with multiple parameters are in the test cases.
This diff builds some infrastructure for annotation transformers: the example above represents the identity function on nullability annotations.
Reviewed By: jvillard
Differential Revision: D4705938
fbshipit-source-id: 9f6194e
Summary:
Before, `trace_of_pname` only grabbed unprotected writes from the summary, so the traces ending in an unprotected read were truncated.
We now look at reads too when appropriate.
Reviewed By: peterogithub
Differential Revision: D4719740
fbshipit-source-id: 28f6e63
Sam Blackshear
Dino Distefano
Jeremy Dubreil
Josh Berdine
Mehdi Bouaziz
Jules Villard
Daiva Naudziuniene
Kyriakos Nikolaos Gkorogiannis
Jia Chen
Andrzej Kotulski
Peter O'Hearn
Dulma Churchill
Martino Luca
Cristiano Calcagno