[DTrace-devel] [PATCH 05/10] dtrace provider: don't use functions from uprobes.h any more

[Kris Van Hees]

On Tue, Aug 29, 2023 at 04:47:25PM +0100, Nick Alcock wrote:
> On 22 Aug 2023, Kris Van Hees spake thusly:
> > On Wed, Aug 02, 2023 at 02:26:55PM +0100, Nick Alcock via DTrace-devel wrote:
> >> +		if (mapp->pr_file->first_segment != mapp)
> >> +			mapp = mapp->pr_file->first_segment;
> >> +
> >> +		asprintf(&spec, "%s:0x%lx", mapp->pr_file->prf_mapname,
> >> +			 sym.st_value - mapp->pr_vaddr);
> >
> > One problem is that asprintf() allocates memory for spec and that is done
> > using the libc allocation routines and not dt_alloc(), and so it is not
> > really correct that it gets freed in attach() using dt_free().  Right now,
> > in practical sense, that ends up being the same thing but it would be wrong
> > if dt_alloc() / dt_free() ever gets implemented to use alternative routines.
> >
> > To avoid possible future issues, it should be freed using free().
> 
> Ack (and done), but frankly my opinion here is that dt_alloc, dt_free,
> dt_zalloc et al are needless complexity, and we should just drop the lot
> with a coccinelle script or something, just like I did for libctf. The

I agree that we should move away from them because the concept that they seem
to encapsulate never came to be (as far as I can see).  There may have been a
plan for this back when the original code was written, but I honestly do not
even know what they were planning.  But yes, for now, let's be consistemt.  And
we can certainly work towards just using the libc equivalents (that are the
functions used under the covers anyway).

Maybe at the next release cycle.

> way to replace the allocator is to interpose malloc/calloc/realloc like
> everyone else does, not to define your own allocator routines with a
> different and less convenient interface (which were never used
> consistently even in the Solaris days, and certainly never paired
> consistently). Every other way is just a recipe for, not fewer bugs, but
> many more. Even having them tied to malloc() as they are now is
> annoying, because a bunch of bug detectors simply point at dt_alloc()
> etc as the source of memory leaks, overrun allocations, etc, rather than
> their *actual* source one call frame up. (Thankfully valgrind shows the
> whole call stack.)
> 
> Using dt_alloc et al consistently would be awful: you'd have to eschew
> everything else that allocated memory, like asprintf() above, in favour
> of calling dt_alloc ourselves (and just forget things that can't be
> passed preallocated buffers: we certainly wouldn't be able to use e.g.
> libelf any more). And using it, but not consistently, is pointless. It's
> no use for leak detection, bounds detection, or alternative allocators.
> What's it for? Making DTrace harder to maintain? :)
> 
> -- 
> NULL && (void)