This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix invalid free of memory allocated during rtld init


On Thu, Jan 17, 2013 at 9:53 AM, Andreas Schwab <schwab@suse.de> wrote:
> Carlos O'Donell <carlos@systemhalted.org> writes:
>
>> An assumption in this case might be:
>> ~~~
>> Early rtld startup can't use malloc, instead it uses
>> a dummy malloc that doesn't support freeing memory
>> (except for the last block allocated).
>> ~~~
>
> That's not the point.  The point is that you can't use free on memory
> allocated during rtld init.  Exactly what I wrote in the subject.

Good point, in that case a more relevant assumption would be:
~~~
The normal allocator does not support freeing memory allocated
by the minimal allocator using during rtld startup before libc.so has
been fully loaded.
~~~

>> I need your help, and so do the others in this
>> community, we'd like to learn from your experience.
>
> You learn a lot more if you try to find it out yourself and ask specific
> questions if you are stuck somewhere.

I agree. Yet, I have no formal training in educational theory
to back up this claim. My claim is solely based on my personal
experience with this kind of learning.

However, it is my opinion that your statement, while true,
doesn't grant you the right as a patch submitter to place the
entire burden of review on the reviewer. I therefore I do not
accept "the reviewer will learn more" as a valid reason for
not including more detail in the patch submission.

A high quality submission includes sufficient rationale to explain
why the change is needed for example:
~~~
In a dynamically linked executable the dynamic loader calls
_dl_init_paths() at startup from dl_main. The call to _dl_init_paths()
is done before libc's malloc() can be used and therefore all allocations
by malloc() are handled by the  dummy allocator (dl-minimal.c).
Calls made to decompose_rpath() in _dl_init_paths() use malloc()
to allocate writable strings. The real allocator doesn't
support freeing these early allocations and we must mark them
as "not malloced" (since they aren't malloced by the real malloc)
to prevent code in open_path() from freeing the entries when the
directories are found not to exist.

In a non-dynamic executable the code in question is never
used because of the surrounding #ifdef SHARED
(even though _dl_init_paths is called by the static startup
via _init in csu/init-first.c).

This patch fixes a case in _dl_init_paths() where we call
decompose_rpath() and fail to set malloced to zero to
indicate that the memory can't be freed.
~~~

This is all information that you must have known to write
to the patch. Putting it into the patch email as rationale
assists the reviewer in doing their job.

Getting back on topic:

(1) Testing?

How did you test this?

(2) Regression test?

Could you please provide a test case to prevent this from
regressing? Such a test case will likely have implementation
knowledge of the internals of the link map, but I think that's
OK. Some tests need such information and are useful.

Unless others object to such tests?

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]