[DTrace-devel] [PATCH 5/6] dtprobed: retry DOF parses at least once

[Nick Alcock]

On 23 May 2023, Eugene Loh via DTrace-devel uttered the following:

> Another patch I don't understand.  (My fault, not yours.)  But I'm guessing it's good.

Basically it's a "just in case" patch: the fact that almost any syscall
can crash the seccomp-jailed child does mean it's a bit fragile. With
the malloc fixes, every crash cause I know of is fixed, but what if,
say, malloc() decides that after a particular pattern of
allocs-and-frees it wants to do an mmap()? That's not a permitted
syscal, so the parser child dies -- but it's very likely that that
pattern of allocs-and-frees was longer than one message's worth, so if
we restarted it and tried again, the DOF would go through fine. (And if
it doesn't, that's fine, we can reject it.)

This also reduces any other bugs that might otherwise cause
cross-message crashes and DOF loss into bugs that only lose DOF which is
actually corrupted, which seems like a good idea.

Normally saying "oh we segfaulted, let's try again" in a parser for
arbitrary user-provided messages is dangerous -- but in this case, we're
protected by the seccomp jail. It can't do hardly *anything*, so
throwing messages at it once more seems likely to be a good idea.

> Any chance of a test?

I don't have a reliable way to crash the DOF parser child, no :P
(and if I had one I'd fix it!).

> And, some extremely minor comments...
>
> On 5/22/23 16:20, Nick Alcock via DTrace-devel wrote:
>> The whole point of jailing the DOF parser is so that failures are
>> harmless (to do harm to anything other than itself an attacking process
>> would need to compromise the state of the parser process such that it
>> not only doesn't crash but returns malicious results affecting
>> *subsequent calls*, which is quite difficult).  This implies that DOF
>
> Grammatically, that parenthetical remark is a complete sentence.  So
> how about a full stop after "harmless," giving the reader a chance to
> catch their breath.

OK, done.

>> -	provider = dof_read(req, parser_out_pipe[0]);
>> -	if (!provider || provider->type != DIT_PROVIDER)
>> -		goto err;
>> +		errmsg = "parsed DOF read failed";
>> +		provider = dof_read(req, parser_out_pipe[0]);
>> +		if (!provider) {
>> +			if (tries++ > 1)
>> +				goto err;
>> +			dof_parser_tidy(1);
>> +			continue;
>> +		}
>> +		if (provider->type != DIT_PROVIDER)
>> +			goto err;
>> +		break;
>> +	} while (!provider);
>
> FWIW, I assume that test will always be true and could be written while(1)?  But perhaps that would be no clearer?

It's not always true because of the if (!provider) above: it does a
continue and we want it to go back to the top of the loop to try again.

(I rephrased this from Yet Another Goto because, uh, I thought it was
clearer: but perhaps not...)

-- 
NULL && (void)