[DTrace-devel] [PATCH 05/14] alloca: new faults

[Kris Van Hees]

On Wed, Mar 09, 2022 at 01:24:12PM +0000, Nick Alcock wrote:
> On 7 Mar 2022, Kris Van Hees said:
> 
> > On Wed, Mar 02, 2022 at 01:44:58PM +0000, Nick Alcock via DTrace-devel wrote:
> >> We'll use DTRACEFLT_BADSIZE to distinguish in error messages between
> >> out-of-bounds errors in bcopy and the like, and size-too-large errors.
> >> (Without this, it's hard to tell if a, say, 0x12 in the fault value
> >> is a nearly-NULL pointer or a wrong size.)
> >
> > Makes sense.
> >
> >> We'll use DTRACEFLT_INTERR to indicate can-never-happen errors where we
> >> have tested the actual condition already (and emitted a proper error)
> >> followed by a check to placate the verifier (which will always succeed
> >> if the actual condition check failed).  The latter failure happening at
> >> runtime always indicates a bug in DTrace, not in the user's code, so
> >> indicate as much.
> >
> > Here I am confused.  If this is a fault that can never happen, why do we
> > even generate code to report such a non-existant fault?  Based on your
> 
> Because without it, the verifier thinks control does not terminate in a
> condition when it in fact always will (because of a conditional the
> verifier does not take into consideration) and we get a spurious
> verifier failure.
> 
> The idea is that we always test a bound that the verifier cannot follow
> (e.g. offset + read size does not run off the end of the region, where
> the read size is in a reg, so the verifier ignores it) before testing
> one that satisfies the verifier (that offset + statically determined
> maximum possible read size -- SIZEMAX -- does not run off the end of the
> region). Of course then I went and did the tests in the opposite order,
> making INTERR possible to trigger. I'll fix that!
> 
> > description this would be used in cases where the condition has already
> > been tested and a proper error has emitted.  Since a probe error results
> > in an immediate return from the clause function (after reporting the
> > fault by means of the ERROR probe clauses), the following code will never
> > execute.
> >
> > So, I really do not see the need for this new fault that can never happen.
> 
> It won't -- but the verifier will run through it because it thinks it
> might execute, and we need at the very least to do a goto exit. The
> INTERR is to make sure that if it ever *does* execute, we know, because
> it's a bug.

So just put in an 'exit'.  No need to even use a jump to an exit label.  Just
a plain 'exit', emitted using BPF_RETURN().