Require eval::Call handlers to always bind a return value if the function should return something

Clang Frontend Static Analyzer
1

Downstream, we had issues with some checkers, that were eval::Calling some APIs, but never bound the return value.
In such cases, no entry will be present in the Environment, hence any expressions loading from it will end up having UnknownSVal, poisoning computations using it. It also affects taint analysis, as taint cannot be bound to UnknownSVals.

To prevent such mistakes, I’d rather assert that eval::Call implementations must bind a value to the currently evaluated CallExpr and LocationContext by the end of the CheckerManager::runCheckersForEvalCall().

To register if such a bindExpr was invoked, we could check if the relevant expression and location context match the one set prior to invoking the eval::Call callbacks.

Note that, I would allow checker to explicitly bind UnknownSVal to the CallExpr if they want to. The point is to be explicit and reliable.

Here is a quick and dirty proof-of-concept implementation for now, to kick off some discussion.
enforceEvalCallsToBindReturnValue.patch (3.9 KB)

After applying this patch, I can see 21 tests failing:

  Clang :: Analysis/array-punned-region.c
  Clang :: Analysis/asm.cpp
  Clang :: Analysis/bsd-string.c
  Clang :: Analysis/bstring.c
  Clang :: Analysis/bstring_UninitRead.c
  Clang :: Analysis/bug_hash_test.cpp
  Clang :: Analysis/cast-value-notes.cpp
  Clang :: Analysis/chroot.c
  Clang :: Analysis/complex.c
  Clang :: Analysis/cstring-ranges.c
  Clang :: Analysis/lambdas-generalized-capture.cpp
  Clang :: Analysis/lambdas.mm
  Clang :: Analysis/null-deref-path-notes.c
  Clang :: Analysis/null-deref-ps-region.c
  Clang :: Analysis/smart-ptr-text-output.cpp
  Clang :: Analysis/smart-ptr.cpp
  Clang :: Analysis/std-c-library-functions-arg-cstring-dependency.c
  Clang :: Analysis/string.c
  Clang :: Analysis/temporaries.cpp
  Clang :: Analysis/weak-functions.c
  Clang :: Analysis/wstring.c

In most cases, some debug.ExprInspection checker was declared with non-void return types triggering this assertion, so let’s ignore issues raised for that case. That way we end up with this list:

  Clang :: Analysis/bsd-string.c
  Clang :: Analysis/bstring.c
  Clang :: Analysis/bstring_UninitRead.c
  Clang :: Analysis/cast-value-notes.cpp
  Clang :: Analysis/chroot.c
  Clang :: Analysis/cstring-ranges.c
  Clang :: Analysis/null-deref-path-notes.c
  Clang :: Analysis/null-deref-ps-region.c
  Clang :: Analysis/smart-ptr-text-output.cpp
  Clang :: Analysis/smart-ptr.cpp
  Clang :: Analysis/std-c-library-functions-arg-cstring-dependency.c
  Clang :: Analysis/string.c
  Clang :: Analysis/weak-functions.c
  Clang :: Analysis/wstring.c

I haven’t checked this list, but for instance the Analysis/chroot.c certainly uncovered that the ChrootChecker doesn’t bind the return value for chroot and chdir calls.

Do you think it would be nice to have this invariant?
@NoQ @Xazax-hun @Szelethus

2

+1, I support this idea. It is really easy to miss modelling some aspects of the functions in EvalCall, the more help we have to guide the devs the better.

3

Good idea, I support this kind of invariant!

As an alternative implementation, I think that instead of a runtime assert this invariant could be checked by the (compile-time) modification of the type signature of the evalCall callback from the current
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
to something like
std::optional<SVal> evalCall(const CallEvent &Call, CheckerContext &C) const;
where returning nullopt is equivalent to returning false in the old method (and signifes that the current checker didn’t evaluate the call), and a returned SVal is used as the return valuel of the simulated function.

However, I also support adding the assertion especially if changing the type signature would lead to more cumbersome code in some checkers.

4

std::optional<SVal> evalCall(const CallEvent &Call, CheckerContext &C) const;

I love this both for the purposes of this discussion, and for the purposes of another, even more powerful invariant:

  • Setting the return value at evalCall() is the only situation when a checker is allowed to modify the Environment.

(The new signature would eliminate the exceptional case, so we can easily forbid checkers from modifying the Environment entirely.)

Also worth noting that

  • It is never acceptable to overwrite an existing Environment binding, even in the engine.

but IIRC we’re quite far away from fulfilling this last invariant; there are a few mutually cancelling bugs that need to be taken care of first.