From mboxrd@z Thu Jan  1 00:00:00 1970
From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers)
Date: Tue, 11 Feb 2014 20:51:40 +0000 (UTC)
Subject: [lttng-dev] RFC: Fix crash in dlerror()
In-Reply-To: <52F9746F.5070302@mentor.com>
References: <52F55511.2080309@mentor.com>
	<1380151240.21051.1391875592403.JavaMail.zimbra@efficios.com>
	<52F66118.5010003@mentor.com>
	<905376946.21151.1391898148330.JavaMail.zimbra@efficios.com>
	<52F9746F.5070302@mentor.com>
Message-ID: <1494953964.23233.1392151900002.JavaMail.zimbra@efficios.com>

----- Original Message -----
> From: "Stefan Seefeld" <stefan_seefeld@mentor.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com>
> Cc: lttng-dev at lists.lttng.org
> Sent: Monday, February 10, 2014 7:53:03 PM
> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror()
> 
> Mathieu,
> 
> after our follow-up discussion I agree to the proposed patch. A few
> details below...

Great!

> 
> On 02/08/2014 05:22 PM, Mathieu Desnoyers wrote:
> 
> > Interesting approach. Then I wonder if we couldn't simply lookup every
> > symbol
> > we're interested in whenever any of the overridden function is called and
> > we notice a NULL pointer, and provide a simplistic "static" allocator for
> > every function overridden.
> > 
> > Please let me know if something like the attached patch works for you.
> > It seems more solid to cover all the cases, so that if the dlsym()
> > implementation
> > ever chooses to use other functions from the memory allocator, it will
> > still work.
> 
> OK, agreed.
> 
> 
> > +static
> > +void *static_realloc(void *ptr, size_t size)
> > +{
> > +	/* Don't free previous memory location */
> > +	return static_calloc_aligned(1, size, 1);
> > +}
> 
> The above of course also needs a memcpy operation, to preserve the
> original data.

OK, added.

> 
> > +		if (cur_alloc.malloc == NULL) {
> >  			fprintf(stderr, "mallocwrap: unable to find malloc\n");
> > -			return NULL;
> > +			abort();
> >  		}
> 
> I'm actually not quite sure what behaviour I would prefer as a user. My
> library-author feeling tell me to not abort the process, but return an
> error condition (i.e., NULL) and let the caller take care of how to
> handle it.
> On the other hand, not finding malloc certainly is a critical error.
> And, this isn't a library in the proper sense, but a preloaded wrapper,
> so users can always re-run his apps without it.
> Hmm...

I'm tempted to go the shortcut route (abort()) since this is really just a
wrapper. I'm attaching an updated version, which fixes a couple of incorrect
handling of the realloc case for the static allocator. I did not add yet the
mmap() stuff, since it adds a bit of complexity to the patch, and I would be
tempted to keep things simple in -rc. I really doubt that real-life use-cases
will have error messages going beyond 4kB. And if it ever happens, we'll hit a
abort() within the wrapper, and it will be easy to fix. I'm tempted to leave
this without the mmap stuff for 2.4, so we don't add regressions late in the
cycle, is that OK with you ?

Thanks,

Mathieu

> 
> Otherwise the patch looks good.
> 
> Thanks,
> 		Stefan
> 
> 
> --
> Stefan Seefeld
> CodeSourcery / Mentor Graphics
> http://www.mentor.com/embedded-software/
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-lttng-ust-malloc.patch
Type: text/x-patch
Size: 9308 bytes
Desc: not available
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20140211/cda0f891/attachment-0001.bin>