[DTrace-devel] [PATCH 2/6] dtprobed: stop malloc making syscalls in the DOF parser child

[Nick Alcock]

On 22 May 2023, Eugene Loh via DTrace-devel stated:

> Shouldn't this have at least one test?

It does -- tst.manyprobes.sh emits 200+KiB of DOF, which is more than
enough to trigger this bug without this fix.

> Update copyright years?

Oh dammit.

> On 5/22/23 16:20, Nick Alcock via DTrace-devel wrote:
>> If dtprobed receives DOF larger than the glibc mmap threshold (64KiB by
>> default), it will make an mmap() to allocate space for it in the parser
>> child.  Since it's in a seccomp jail, this is denied and the child is
>> killed.  Whoops.
>>
>> Rejigging the entire parser child to avoid allocations is possible, but
>
> Is "rejig" is a British idiom?  How about saying "changing"?

Err... is it a Britishism? Checking the OED :)

... nah, most citations are from the US! It's just more recent than I
thought -- first citation from 1965, so *probably* originated in the
20th century sometime.

"Rejig" is not quite the same as "change": the implication is a major
change for a minor reason.

>> needless: we can just ask glibc to never call mmap and keep enough space
>> free at the top of the heap for plausible allocations, make a big
>> allocation right before nailing ourselves into the jail, and keep
>> going. We have a (fairly small) maximum size for a given piece of DOF,
>> and there is no risk of heap fragmentation because we free everything
>> on every message received, so a fixed size (with respect to that
>
> Use a full stop rather than the comma before "so a fixed size".

I tried both that and a semicolon and they both read as much clunkier
than what I have now. It's "We have X, so Y", a logical imputation; you
can't really use a full stop, I don't think.

Perhaps 'thus' instead of 'so', but that reads as terribly formal.

>> maximum) should always be enough.
>>
>> (There is a slight extra cocmplexity here: we have to hide the malloc()
>
> cocmplexity

I meant to do that!! (Fixed.)

>> and free() for the big allocation in another translation unit (and turn
>> LTO off for that translation unit), or GCC will recognise that the
>> malloc/free are do-nothing, and optimize them away.)
>
> Isn't there a simpler solution?  I don't know.  Maybe assert(p = malloc()); free(p)?

I tried that :( GCC doesn't stop optimizing just cos you're in an
assertion, alas.

I don't quite understand how it knows that malloc() and free() are
paired like that, I mean if anything has side effects (on the heap) it's
malloc(). I think it must be the __attribute__((malloc)) it's declared
with: it knows that an __attribute__((malloc))'ed pointer is guaranteed
to be new, and that free() consumes it, and that therefore the two put
together with nothing between them are NOP... which is usually true, but
here we are interested in its usually-invisible side effects on the heap.
It even realises that writing into it, then reading out of it are NOP,
probably because it eliminates the read (since nothing uses its result),
then the write, then oh look we can drop the whole pointer.

Modern compilers are pretty smart.

>> (If glibc ever stops respecting M_MMAP_MAX or M_TRIM_THRESHOLD we'll be
>> in bigger trouble and might need our own simpleminded allocator, but
>
> Hyphenate simple-minded.

Done. (Google Trends suggests they're more or less equally popular. I
have no real preference.)

>> diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
>> @@ -250,12 +253,21 @@ dof_parser_start(int sync_fd)
>> -		if (!_dtrace_debug)
>> +		mallopt(M_MMAP_MAX, 0);
>> +		mallopt(M_TRIM_THRESHOLD, -1);
>> +		seccomp_fill_memory_really_free(seccomp_fill_memory_really_malloc(DOF_MAXSZ * 2));

(The function names I used are ridiculous, I know. Happy to change them
for something else, _seccomp_malloc or something? Nothing unambiguously
better occurred to me, and it's not as if these things should ever be
called from anywhere else.)

>> +
>> +		if (!_dtrace_debug) {
>>   			if (syscall(SYS_seccomp, SECCOMP_SET_MODE_STRICT, 0, NULL) < 0)
>>   				_exit(1);
>> +		}
>
> Why is the "if (syscall())" statement being wrapped in {}?  I thought we avoided such braces where we can.

Artifact of code motion: I used to do the mallopt()s and malloc() in
there too (because they're useless if you're not going to seccomp()):
but it's actually useful to track down such problems if we do the
mallopt() and malloc() all the time, because then you can spot the cause
of this class of crash by running dtprobed with -d (turning off
seccomp), running it under strace -f, and feeding it the problematic
message. The strace() will then show the mmap() or sbrk() or whatever
plain as day.

(Fixed.)

-- 
NULL && (void)