[patch] wordexp resource cleanup

[Peter Rosin]

On 2012-09-06 23:22, Peter Rosin wrote:
> On 2012-08-26 23:52, Peter Rosin wrote:
>> On 2012-08-20 14:01, Peter Rosin wrote:
>>> On 2012-08-20 12:29, Peter Rosin wrote:
>>>> On 2012-08-20 12:17, Peter Rosin wrote:
>>>>> On 2012-08-20 00:00, Peter Rosin wrote:
>>>>>> While looking up how wordexp worked I noticed that the resource handling
>>>>>> in the implementation was "somewhat lacking". So here's a follow-up to
>>>>>> take care of a few corner cases. (Not even compile-tested as I don't
>>>>>> know how to set that up without reading up on the subject, but you
>>>>>> wouldn't trust a newcomer anyway, right?)
>>>>>
>>>>> Whoops! There were more bugs in that function, so here's an update. This
>>>>> one has actually been tested and works AFAICT.
>>>>
>>>> Whoops, that's a misleading ChangeLog entry. The first part of it
>>>> referred to fixing a bug that I had introduced myself when sanitizing
>>>> the realloc calls. So, a better ChangeLog would be:
>>>
>>> Whoops again, more bugs. The following program leaks like hell (with a
>>> self-built Cygwin bash supporting --wordexp, pending an update from Eric):
>>>
>>> ---------------8<------------------------------
>>> #include <wordexp.h>
>>>
>>> int
>>> main (void)
>>> {
>>>   wordexp_t we;
>>>
>>>   we.we_offs = 3000;
>>>   while (1) {
>>>     wordexp ("/usr/bin/*", &we, WRDE_DOOFFS);
>>>     wordfree (&we);
>>>   }
>>>   return 0;
>>> }
>>> ---------------8<------------------------------
>>>
>>> So, here's an update fixing that too. Reading opengroup docs on wordexp
>>> (http://pubs.opengroup.org/onlinepubs/009695399/functions/wordexp.html)
>>> suggests that it should be ok to rely on the offset count in the given
>>> wordexp_t, and I find nothing on that page suggesting that it is not
>>> ok to store the non-use of an offset in the we_offs member. So, that's
>>> what I did.
>>
>> Whoops again. It was wishful thinking that made me conclude that we_offs
>> could be reused like that. When rereading I can see that we_offs is
>> only sacred if WRDE_DOOFFS is given. We therefore need to store the
>> non-use of an offset someplace else. But we can't expand wordexp_t, as
>> that would break existing apps already compiled to allocate the existing
>> size of memory needed for wordexp_t.
>>
>> The best I could come up with was to allocate one extra pointer and no
>> longer have we_wordv point directly at the allocated memory, but instead
>> point one pointer into the allocation. That way, we get a private area
>> at the start of the allocation to use as we see fit.
>>
>> I added a comment in the code explaining this abnormality.
>>
>> If you consider this too convoluted and prefer that application using
>> the we_offs member for their own needs when WEDE_DOOFFS is not used
>> simply deserve to lose, I'm perfectly happy with the wordexp-3.patch in
>> the previous mail.
> 
> Whoops again. Anyway, this time when looking up the thread to PING the
> patch, I noticed that I used exit instead of _exit in the child. So,
> continuing the monologue, I have now replaced exit(1) with
> _exit(EXIT_FAILURE) for the cases where things go south in the child.
> I'm still pretty happy with wordexp-3.patch, but it too needs this
> s/exit/_exit/ change of course.
> 
> Is the patch too big? Should I split it? Are we waiting for a new Cygwin
> bash to appear to simplify testing? Anything?

Forgot the patch...

Cheers,
Peter

	* libc/posix/wordexp.c (wordexp): Handle expanded words longer
	than 500 bytes.	Don't leak file streams. Help wordfree step past
	we_offs entries when freeing. Return WRDE_NOSPACE on resource
	allocation failure. Cleanup leftover resources when failing.
	* libc/posix/wordfree.c (wordfree): Step past we_offs words
	before starting to free.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: wordexp-5.patch
Type: text/x-patch
Size: 7719 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/newlib/attachments/20120913/83e79928/attachment.bin>