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

[Eugene Loh]

Shouldn't this have at least one test?

Update copyright years?

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"?

> 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".

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

cocmplexity

> 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)?

> (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.

> this doesn't seem at all likely in the near future at least.)
>
> 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));
> +
> +		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.