Summary: Just changing ClangTrace to actually look at the different sanitizer kinds.
Reviewed By: jeremydubreil
Differential Revision: D6325086
fbshipit-source-id: 5da236d
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:
In a summary, you never want to see a trace where non-footprint sources flow to a sink.
Such a trace is useless because nothing the caller does can make more data flow into that sink.
Reviewed By: jeremydubreil
Differential Revision: D5779983
fbshipit-source-id: d06778a
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: Previously, this would incorrectly classify types like `map<std::string, int>` as a buffer
Reviewed By: mbouaziz
Differential Revision: D6125530
fbshipit-source-id: c8564de
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:
`pp_instr_list` was not tailrec causing a stack overflow on big code.
Also simplified a few things
Reviewed By: jvillard
Differential Revision: D5995451
fbshipit-source-id: 40a4911
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:
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: Add procedure where error is triggered (if available) + bad data to messages.
Reviewed By: jvillard
Differential Revision: D5756398
fbshipit-source-id: a16f7cf
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:
A function can both be a sink and propagate source info, but we currently ignore the summary for any function that is also a sink.
This will cause us to under-report for (e.g.) `src1 = source(); src2 = strcpy(dest, src1); exec(src2)`.
This is both a potential buffer overflow and a potential shell injection, but we won't report the second issue.
Reviewed By: jberdine
Differential Revision: D5676167
fbshipit-source-id: 232ab2f
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:
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: Useful for identifying user-controlled array accesses that could lead to buffer overflows
Reviewed By: mbouaziz
Differential Revision: D5520985
fbshipit-source-id: 92984f6
Summary:
When a sink name is specified in `.inferconfig` or in OCaml, it might conflict with a function of the same name that has a different number of args.
We shouldn't try to create a sink in this case, and we definitely shouldn't crash.
Reviewed By: jeremydubreil
Differential Revision: D5561216
fbshipit-source-id: fa1859b
Summary: A temporary workaround until we can understand why this happens and fix it.
Reviewed By: jeremydubreil
Differential Revision: D5559838
fbshipit-source-id: dc86eb9
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: 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:
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:
In C++ it may happen that a procedure return 'variable' is an access
path that cannot be translated to Hil. For now just skip these instead
of crashing.
Reviewed By: mbouaziz
Differential Revision: D5356134
fbshipit-source-id: 977dfba
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: This makes it possible to see which tainted parameter can flow to a sink, which is quite useful.
Reviewed By: jeremydubreil
Differential Revision: D5213297
fbshipit-source-id: 1371b5a
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: 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: 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: Have found this useful in Quandary for fbcode, where we want to do this for folly due to its use of assembly (details in comments).
Reviewed By: mbouaziz
Differential Revision: D5167564
fbshipit-source-id: bf6d7e0
Summary:
For now, we just support clearing the taint on a return value.
Ideally, we would associate a kind with the sanitizer and only clear taint that matches that kind.
However, it's fairly complicated to make that work properly with footprint sources.
I have some ideas about how to do it with passthroughs instead, but let's just do the simple thing for now.
Reviewed By: jeremydubreil
Differential Revision: D5141906
fbshipit-source-id: a5b8b5e
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: String are very important for taint analysis, have to make sure that we have the right models/the right behaviors for unknown code.
Reviewed By: jvillard
Differential Revision: D5054832
fbshipit-source-id: 7e7ee07
Summary: In particular, the heuristics for propagating taint via unknown code needs to be aware of the frontend's trick of introducing dummy return variables.
Reviewed By: mbouaziz
Differential Revision: D5046345
fbshipit-source-id: da87665
Summary:
There are two pointer-related operations you can do in C++ but not Java that we need to support in taint analysis:
(1) `*formal_ptr = ...` when `formal_ptr` is a formal that's a pointer type. Java doesn't have raw pointers, so we didn't need to handle this case.
(2) Passing by reference, which Java also doesn't have (everything is pass-by-value).
Reviewed By: mbouaziz
Differential Revision: D5041246
fbshipit-source-id: 4e8f962
Summary: Same as D5026082, but allowing specification in JSON rather than harcoded in Infer.
Reviewed By: jeremydubreil
Differential Revision: D5030042
fbshipit-source-id: 8a6cfee
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: Stops Quandary errors from getting dropped on the floor when it runs alongside the other checkers.
Reviewed By: jeremydubreil
Differential Revision: D5010801
fbshipit-source-id: 2847f61
Summary:
Needed because this is how the Clang frontend translates returns of non-POD, non pointer values (I think)?
Will handle the more general case of pass by reference soon.
Reviewed By: jvillard
Differential Revision: D5017653
fbshipit-source-id: 1fbcea5
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: Sawja assigns them on multiple control-flow paths, so they're not SSA.
Reviewed By: peterogithub
Differential Revision: D4896745
fbshipit-source-id: c805216
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:
If we couldn't project the callee access path into the caller during summary application, we still added the corresponding trace to the caller state.
This was wasteful; it just bloats the caller with state it will never look at.
Fixed it by making `get_caller_ap_node` return `None` when the state won't be visible in the caller.
Reviewed By: jeremydubreil
Differential Revision: D4727937
fbshipit-source-id: 87665e9
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:
Changes every checker to take a summary as parameter and return the updated summary to the next checker. Since several operations, like `Reporting.log_*` are modifying the summary in memory by loading them from the in-memory cache of summaries, we currently need to rely on `Specs.get_summary_unsafe` to return the updated version of the summary.
This diff allows to change the API of `Reporting` to take a summary as input and progressively remove all the calls `Specs.get_summary_unsafe` independently from adding the possibility to run several checkers at the same time. The final objective to have every checker just passing around the summary of the procedure being analyzed, and having the in-memory cache only use to store the summaries of the callees.
Reviewed By: sblackshear
Differential Revision: D4649252
fbshipit-source-id: 98f7ca7
Summary:
There was a bug where we allowed ourselves to project local variables from the callee summary into an access path in the caller.
We should only be able to project callee variables that are in the footprint.
Reviewed By: jeremydubreil
Differential Revision: D4684868
fbshipit-source-id: 53a2b9d
Summary: In some cases where a function is called directly on a formal (e.g, `def foo(o) { callSomething(o) }`, we were failing to propagate the footprint trace to the caller.
Reviewed By: jeremydubreil
Differential Revision: D4502404
fbshipit-source-id: d4d632f
Summary:
At one point I thought we'd want to have lots of different schedulers for things like exploring loops in different orders, but that hasn't materialized.
Let's make the common use-case simpler by hiding the `Scheduler` parameter inside the `AbstractInterpreter` module.
We can always expose `MakeWithScheduler` later if we want to.
Reviewed By: jberdine
Differential Revision: D4508095
fbshipit-source-id: 726e051
Summary:
When the receiver type and return type of an unknown call are the same, propagate taint to both the receiver and the return type.
This does the right thing for common "builder-style" methods that both update and return the receiver.
We already had custom models for a few such methods (e.g., `StringBuilder.append`), but we can remove them now.
Reviewed By: jeremydubreil
Differential Revision: D4490071
fbshipit-source-id: 325ea88
Summary:
If we have code like
```
o.setF(source())
sink(o)
```
and `setF` is an unknown method, we probably want to report.
Reviewed By: jeremydubreil, mburman
Differential Revision: D4438896
fbshipit-source-id: 5edd204
Summary: This will be useful in upcoming changes to the thread-safety analysis as well.
Reviewed By: dkgi
Differential Revision: D4402146
fbshipit-source-id: c750127
Summary: Generalized the CppTrace into a Clang trace because we don't currently have separate checkers for Obj-C and Cpp. Happy to separate them later if there is a good reason
Reviewed By: akotulski
Differential Revision: D4394952
fbshipit-source-id: e288761
Summary:
A domain should not definite its initial state, since distinct users of the domain may want to choose different initial values.
For example, one user might want to bind all of the formals to some special values, and one user might want the initial domain to be an empty map
This diff makes this distinction clear in the types by (a) requiring the initial state to be passed to the abstract interpreter and (b) lifting the requirement that abstract domains define `initial`.
Reviewed By: jberdine
Differential Revision: D4359629
fbshipit-source-id: cbcee28
Summary: This more easily allow to switch between the different modes for handeling dynamic dispatch
Reviewed By: sblackshear
Differential Revision: D4367556
fbshipit-source-id: 795d2c4
Summary:
There's a lot of boilerplate work to be done when adding a new kind of source.
This diff tries to reduce the boilerplate by making a functor do all the work.
The functor:
(1) adds a notion of "footprint kind" to the source
(2) packages the source with a call site
Reviewed By: jvillard
Differential Revision: D4349224
fbshipit-source-id: 5e1701a
Summary:
We currently can only model the return values of functions as sources.
In order to model inputs of endpoints as sources, we need the capability to treat the formals of certain functions as sources too.
This diff adds that capability by adding a function for getting the tainted sources to the source module, then using that info in the analysis.
Reviewed By: jeremydubreil
Differential Revision: D4314738
fbshipit-source-id: dd7d423
Summary: Different analyses need different preanalyses to run. It doesn't make sense for all of the pre-analyses to be bundled together into one package.
Reviewed By: jvillard
Differential Revision: D4348243
fbshipit-source-id: 46a8ebd
Summary:
Previously, summaries worked by flattening the access tree representing the post of the procedure into (in essence) a list of functions from caller input traces to callee output traces.
This is inefficient in many ways, and is also much more complex than just using the original access tree as the summary.
One big inefficiency of the old way is this: calling `Trace.append` is slow, and we want to do it as few times as possible.
Under the old summary system, we would do it at most once for each "function" in the summary list.
Now, we'll do it at most once for each node in the access tree summary.
This will be a smaller number of calls, since each node can summarize many input/output relationships.
Reviewed By: jeremydubreil
Differential Revision: D4271579
fbshipit-source-id: 34e407a
Summary:
Before, the Interprocedural functor was a bit inflexible. You couldn't do custom postprocessing like normalizing the post state or coverting the post from an astate type to a summary type.
Now, you can do whatever you want by passing a custom `~compute_post` function.
Since `AbstractInterpreter.compute_post` can be used by clients who don't care to do anything custom, this doesn't create too much boilerplate.
Reviewed By: jvillard
Differential Revision: D4309877
fbshipit-source-id: 8d1d85d
Sam Blackshear
Josh Berdine
Mehdi Bouaziz
Jules Villard
Jeremy Dubreil
Andrzej Kotulski
Martino Luca
Cristiano Calcagno