Summary: Preparing to extend HIL with Dereference and AddressOf expressions. Next steps: (1) change SIL -> HIL translation to preserve address of and dereference; (2) adapt analyses based on HIL to make use access expressions.
Reviewed By: jeremydubreil
Differential Revision: D6961928
fbshipit-source-id: 51da919
Summary:
The boolean lock domain is simple and surprisingly effective.
But it's starting to cause false positives in the case where locks are nested.
Releasing the inner lock also releases the outer lock.
This diff introduces a new locks domain: a map of locks (access paths) to a bounded count representing an underapproximation of the number of times the lock has been acquired.
For now, we just use a single dummy access path to represent all locks (and thus a count actually would have been sufficiently expressive; we don't need the map yet).
But I'm planning to remove this limitation in a follow-up by refactoring the lock models to give us an access path.
Knowing the names of locks could be useful for error messages and suggesting fixes.
Reviewed By: jberdine
Differential Revision: D6182006
fbshipit-source-id: 6624971
Summary:
Previously, we could understand than an access was safe either because it was possibly owned or protected by a thread/lock, but not both. If an access was both protected by a lock and rooted in a paramer (i.e., possibly owned), we would forget the ownership part of the precondition and remember only the lock bit. This leads to false positives in cases where an access protected by a lock is owned, but another unowned access to the same memory is not protected by a lock (see the new `unownedLockedWriteOk` E2E test for an example).
This diff makes access safety conditions disjunctive so we can simultaneously track whether an access is owned and whether an access is protected by a thread/lock. This will fix false positives like the one explained in T24015160.
Reviewed By: jberdine
Differential Revision: D6671489
fbshipit-source-id: d17715f
Summary:
Was trying to decide where to add a new Java utility function and realized that things are a bit disorganized.
Some operations on `Typ.Name.t`'s live in `Typ.Procname`, and some live inside an inner `Java` module whereas some are outside of the module with a `java_` prefix.
Let's move toward putting all Java/C/Objc/C++-specific functions in dedicated modules.
This diff does some of the work for Java.
There are Java-specific functions that operate on `Typ.Procname.t`'s that will have to be converted to work on `Typ.Procname.Java.t`'s, but changing those clients will be more involved.
Will also move C/Objc/C++ functions in a follow-up.
Reviewed By: jeremydubreil
Differential Revision: D6737724
fbshipit-source-id: cdd6e68
Summary:
Found the dead code with the script in the next commit, iteratively until no
warnings remained.
Methodology:
1. I kept pretty-printers for values, which can be useful to use from infer's REPL (or
when printf-debugging infer in general)
2. I kept functions that formed some consistent API (but not often, so YMMV), for instance if it looked like `Set.S`, or if it provides utility functions for stuff in development (mostly the procname dispatcher functions)
3. I tried not to lose comments associated with values no longer exported: if the value is commented in the .mli and not the .ml, I moved the comment
4. Some comments needed updating (not claiming I caught all of those)
5. Sometimes I rewrote the comments a bit when I noticed mis-attached comments
Reviewed By: mbouaziz
Differential Revision: D6723482
fbshipit-source-id: eabaafd
Summary:
Upgrade ocamlformat to 0.3, and (necessarily) base to v0.10.0.
- Fix accumulated mis-formatting
- Update opam.lock to unbreak clean build
- Update to base v0.10.0
- Update opam.lock for base
- Update offline opam repo
- Everyone should already have removed their ocamlformat pin
- ocamlformat 0.3 supports output to stdout natively
- bump version of ocamlformat
Reviewed By: jeremydubreil
Differential Revision: D6636741
fbshipit-source-id: 41a56a8
Summary: There was a back and forth conversion between `string` and `IssueType.t` which was not necessary.
Reviewed By: sblackshear
Differential Revision: D6562747
fbshipit-source-id: 70b57a2
Summary:
This diff adds a layer of report deduplication logic in addition to
the existing scheme.
Suppose issue 1 with trace1a and trace1b, and issue 2 with trace2a and
trace2b. If trace1a ends at the same location as trace2a (resp.,
trace2b) and trace1b ends at the same location as trace2b (resp.,
trace2a), then consider issues 1 and 2 to be duplicates.
This chooses to report the issue with the smaller sum of trace
lengths, breaking ties using the issue hashes, and eventually the
entire issue. Therefore there is a potential for flakiness with
respect the the choice of which report to make within a
hash-equivalence class.
Reviewed By: sblackshear
Differential Revision: D6519607
fbshipit-source-id: 63210ab
Summary: It is difficult to understand a lock consistency violation if error message includes an access path with a logical variable or a temporary variable as a base. As a temporary fix, we want to suppress all such warnings.
Reviewed By: sblackshear
Differential Revision: D6460559
fbshipit-source-id: 6f3fc18
Summary: To avoid false positives, we treat `operator[]` in cpp as container read. Moreover, if a container `c` is owned, we make all accesses `c[i]` to be also owned.
Reviewed By: sblackshear
Differential Revision: D6396574
fbshipit-source-id: 94aabff
Summary:
Deduping issues when generating a single report and then diffing the
reports can lead to introduced issues being considered duplicates of
existing issues.
Reviewed By: sblackshear
Differential Revision: D6414673
fbshipit-source-id: bba81fd
Summary:
In C++ some modeled functions have definitions, which leads to traces
that contain an access from the modeling, but continue on into the
implementation of the modeled function. Such traces appear the same as
those that are truncated due to limitations of the buck integration in
the Java analysis. Since all Java models are for functions without
definitions in the code base, this diff limits the truncated trace
suppression to the Java analysis.
Reviewed By: sblackshear
Differential Revision: D6373793
fbshipit-source-id: 1f01509
Summary:
Naming a variable `_foo` makes the compiler not warn about them if they are
unused, but there are lots of instances of such variables in the code where
they are in fact used, defeating the warning and introducing confusion for
those used to this naming convention.
Basically `sed -i -e "s/ _\([a-zA-Z][a-zA-Z0-9_']*\)/ \1_/g" **/*.ml` followed
by manual fixing of compilation errors (lots of `compare__foo` ->
`compare_foo_`).
Reviewed By: mbouaziz
Differential Revision: D6358837
fbshipit-source-id: 7ffb4ac
Summary: In a thread safety report we used the access path from the final sink. This diffs change the report to include the expanded access path from the initial sink.
Reviewed By: sblackshear
Differential Revision: D6297848
fbshipit-source-id: 2386063
Summary:
Change ocamlformat installation procedure to use opam instead of
pinning.
Reformat all code with v0.2, which has a few improvements.
Reviewed By: jvillard
Differential Revision: D6292057
fbshipit-source-id: 759967f
Summary:
When fuzzy-matching cpp names, allow to match only a prefix of
blacklist entries.
Reviewed By: da319
Differential Revision: D6233055
fbshipit-source-id: a3a4913
Summary: We were conflating reads/writes with container reads/writes that created false positives.
Reviewed By: sblackshear
Differential Revision: D6232768
fbshipit-source-id: 39159cb
Summary:
When C++ functions are translated to SIL procedures, their type is C rather then C++. In RacerD, we want to treat C++ functions the same as C++ methods.
Added a function to check if the procedure is Objc/Objc++/C/C++.
Reviewed By: sblackshear
Differential Revision: D6209523
fbshipit-source-id: 293f938
Summary:
Seems it should have been done there all along.
The analyzer does not currently understand the implementation of
atomicity in folly::AtomicStruct.
The analyzer does not currently understand when std::atomic operations
are are used correctly versus incorrectly.
The analyzer does not currently understand that the representation of
folly::ThreadLocal is, ah, thread-local, leading to false alarms.
The analyzer does not currently understand the control flow /
scheduling constraints imposed by the implementation of Future.
It seems that the implementation of folly::Optional is more C++
template magic than the analyzer can currently understand.
The model of std::vector contains bogus memory accesses, leading to
false alarms.
Reviewed By: sblackshear
Differential Revision: D6226199
fbshipit-source-id: 8cb083b
Summary:
Destructors usually do not race with other methods.
We do not want to analyze or report on destructors.
Reviewed By: sblackshear
Differential Revision: D6222145
fbshipit-source-id: 5266622
Summary: More general version of the fix in D6138749. This diff moves RacerD's lock modeling into a separate module and uses the module in the HIL translation to check when a function has lock/unlock semantics.
Reviewed By: jberdine, da319
Differential Revision: D6191886
fbshipit-source-id: 6e1fdc3
Summary: Functions that do not belong to a class or a struct are translated to c-style functions even in the context of cpp. We need to add ownership to locals for c-style functions too.
Reviewed By: sblackshear
Differential Revision: D6196882
fbshipit-source-id: 715f129
Summary: `std::unique_lock` constructor allows to create a unique lock without locking the mutex. `std::unique_lock::try_lock` returns true if mutex has been acquired successfully, and false otherwise. It could be that an exception is being thrown while trying to acquire mutex, which is not modeled.
Reviewed By: jberdine
Differential Revision: D6185568
fbshipit-source-id: 192bf10
Summary:
The concurrency analyzer often does not understand object lifetimes
well enough to realize that destructors are usually not called in
parallel with any other methods. This leads to false alarms. This diff
suppresses these by simply skipping destructors in the concurrency
analysis.
Reviewed By: sblackshear
Differential Revision: D6182646
fbshipit-source-id: e9d1cac
Summary:
The clang frontend translates static locals incorrectly, in the sense
that the initializer is executed many times instead of once. This
leads to false alarms in the concurrency analysis. This diff
suppresses these by ignoring accesses to static locals.
Reviewed By: sblackshear
Differential Revision: D6182644
fbshipit-source-id: d8ca4c0
Summary:
Code often uses std::unique_lock::owns_lock to test if a deferred lock
using the 2-arg std::unique_lock constructor actually acquired the
lock.
Reviewed By: sblackshear
Differential Revision: D6181631
fbshipit-source-id: 11e9df2
Summary:
Use a distinct issue type for the Java and C++ concurrency analyses,
as the properties they are checking are significantly different.
Reviewed By: sblackshear
Differential Revision: D6151682
fbshipit-source-id: 00e00eb
Summary:
Due to limitations in our Buck integration, the thread-safety analysis cannot create a trace that bottoms out in a Buck target that is not a direct dependency of the current target.
These truncated traces are confusing and tough to act on.
Until we can address these limitations, let's avoid reporting on truncated traces.
Reviewed By: jeremydubreil
Differential Revision: D5969840
fbshipit-source-id: 877b9de
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:
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
Daiva Naudziuniene
Sam Blackshear
Jules Villard
Josh Berdine
Jeremy Dubreil
Peter O'Hearn
Dulma Churchill
Philip Cohn-Cort