[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [Lsb-test] /tset/LSB.os/genuts/glob/T.glob 30



On Thu, 2004-07-22 at 06:21, Andrew Josey wrote:
> (from one of my engineers...)
> 
> I believe this is a bug in the glibc globfree() function.
> 
> In version 1.49 of sysdeps/generic/glob.c I see this code in the glob()
> function:
> 
>           status = glob_in_dir (filename, dirs.gl_pathv[i],
>                                 ((flags | GLOB_APPEND)
>                                  & ~(GLOB_NOCHECK | GLOB_NOMAGIC)),
>                                 errfunc, pglob);
>           if (status == GLOB_NOMATCH)
>             /* No matches in this directory.  Try the next.  */
>             continue;
> 
>           if (status != 0)
>             {
>               globfree (&dirs);
>               globfree (pglob);
>               pglob->gl_pathc = 0;
>               return status;
>             }
> 
> When an unreadable directory is encountered, glob_in_dir() returns
> GLOB_ABORTED.  The above code then calls globfree() before returning.
> This is in itself non-compliant, as glob() is supposed to return
> partial results on error.  

In other places in the code, GLOB_APPEND is checked before globfree() is
called.  So perhaps we need to add this check here as well.  (Partial
results seem to be implemented via recursive glob() calls with
GLOB_APPEND set, so this should take care of the problem.)

But, strictly speaking, this is unrelated to the problem at hand.

> However, the double-free problem is because
> the call to globfree() in the above code frees pglob->gl_pathv, then
> when the application calls globfree() that call also tries to free
> pglob->gl_pathv.  It could be fixed by setting pglob->gl_pathv to
> NULL in globfree() after freeing it:
> 
> 	void
> 	globfree (pglob)
> 	     register glob_t *pglob;
> 	{
> 	  if (pglob->gl_pathv != NULL)
> 	    {
> 	      size_t i;
> 	      for (i = 0; i < pglob->gl_pathc; ++i)
> 		if (pglob->gl_pathv[pglob->gl_offs + i] != NULL)
> 		  free ((__ptr_t) pglob->gl_pathv[pglob->gl_offs + i]);
> 	      free ((__ptr_t) pglob->gl_pathv);
> ADD ->        pglob->gl_pathv = NULL;
> 	    }
> 	}

I suppose it's implicit to the spec that pglob's entire state is
undefined after calling globfree() on it.  (At least, I don't see any
explicit statement to that effect in SUSv3.)

If this is the case, one cannot assume the integrity of any part of
pglob, including gl_pathc.  I therefore tend to agree with Thorsten that
gl_pathc should be checked in globfree and set to zero there as well. 
See:

http://sources.redhat.com/ml/libc-hacker/2002-02/msg00095.html

This would advance the implicit statement in the spec that setting
gl_pathc to 0 is a flag that pglob is invalid.  The only disadvantage
would be the additional conditional test (for gl_pathc) over the above
patch.  But is globfree() really so performance-critical that we can't
afford it?



Reply to: