Re: [patch] wordexp resource cleanup

[Peter Rosin <peda at lysator dot liu dot se>]

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?

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.