glob must not call globfree

glob must not call globfree

[Andreas Schwab]

glob must never call globfree by itself, it is the sole resposibility of
the caller to do that.

Andreas.

2002-02-07  Andreas Schwab  <schwab@suse.de>

	* sysdeps/generic/glob.c: Don't call globfree on the passed glob
	structure.

--- sysdeps/generic/glob.c.~1.40.~	Wed Jan  2 11:11:57 2002
+++ sysdeps/generic/glob.c	Thu Feb  7 13:39:11 2002
@@ -1,4 +1,4 @@
-/* Copyright (C) 1991-1999, 2000, 2001 Free Software Foundation, Inc.
+/* Copyright (C) 1991-1999, 2000, 2001, 2002 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -417,11 +417,7 @@
 #else
 	  char *onealt = (char *) malloc (strlen (pattern) - 1);
 	  if (onealt == NULL)
-	    {
-	      if (!(flags & GLOB_APPEND))
-		globfree (pglob);
-	      return GLOB_NOSPACE;
-	    }
+	    return GLOB_NOSPACE;
 #endif
 
 	  /* We know the prefix for all sub-patterns.  */
@@ -500,8 +496,6 @@
 #ifndef __GNUC__
 		  free (onealt);
 #endif
-		  if (!(flags & GLOB_APPEND))
-		    globfree (pglob);
 		  return result;
 		}
 
@@ -932,7 +926,6 @@
 	  if (status != 0)
 	    {
 	      globfree (&dirs);
-	      globfree (pglob);
 	      return status;
 	    }
 
@@ -942,7 +935,6 @@
 			    pglob->gl_pathc - old_pathc))
 	    {
 	      globfree (&dirs);
-	      globfree (pglob);
 	      return GLOB_NOSPACE;
 	    }
 	}
@@ -974,7 +966,6 @@
 	      if (pglob->gl_pathv[newcount] == NULL)
 		{
 		  globfree (&dirs);
-		  globfree (pglob);
 		  return GLOB_NOSPACE;
 		}
 
@@ -1007,10 +998,7 @@
 	  if (prefix_array (dirname,
 			    &pglob->gl_pathv[old_pathc + pglob->gl_offs],
 			    pglob->gl_pathc - old_pathc))
-	    {
-	      globfree (pglob);
-	      return GLOB_NOSPACE;
-	    }
+	    return GLOB_NOSPACE;
 	}
     }
 
@@ -1033,10 +1021,7 @@
  	    size_t len = strlen (pglob->gl_pathv[i]) + 2;
 	    char *new = realloc (pglob->gl_pathv[i], len);
  	    if (new == NULL)
-	      {
-		globfree (pglob);
-		return GLOB_NOSPACE;
-	      }
+	      return GLOB_NOSPACE;
 	    strcpy (&new[len - 2], "/");
 	    pglob->gl_pathv[i] = new;
 	  }

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: glob must not call globfree

[Andreas Schwab]

This was pointed out by Tim Waugh in private mail: wordexp does not always
call globfree after glob.

Andreas.

2002-02-07  Andreas Schwab  <schwab@suse.de>

	* sysdeps/generic/wordexp.c (do_parse_glob): Always call globfree
	after glob.

--- sysdeps/generic/wordexp.c.~1.6.~	Fri Aug 17 09:15:21 2001
+++ sysdeps/generic/wordexp.c	Thu Feb  7 14:12:33 2002
@@ -1,5 +1,5 @@
 /* POSIX.2 wordexp implementation.
-   Copyright (C) 1997, 1998, 1999, 2000, 2001 Free Software Foundation, Inc.
+   Copyright (C) 1997, 1998, 1999, 2000, 2001, 2002 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Tim Waugh <tim@cyberelk.demon.co.uk>.
 
@@ -395,6 +395,7 @@
     {
       /* We can only run into memory problems.  */
       assert (error == GLOB_NOSPACE);
+      globfree (&globbuf);
       return WRDE_NOSPACE;
     }
 

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: glob must not call globfree

[Roland McGrath]

I don't have the spec handy but it seems unlikely to me that a caller
getting GLOB_NOSPACE is expected to call globfree.  Are you sure?

Re: glob must not call globfree

[Ulrich Drepper]

Andreas Schwab <schwab@suse.de> writes:

> 2002-02-07  Andreas Schwab  <schwab@suse.de>
> 
> 	* sysdeps/generic/glob.c: Don't call globfree on the passed glob
> 	structure.

I very much doubt this is correct.  I've never seen any requirement
like this.  If somebody intercepts the globfree calls for whatever
reasons and finds them handling memory which wasn't returned by a call
to glob this is the problem of that code.  Fortunately for whoever
makes this assumption one of the optimizations on the table for 2.3
will eliminate this.  If you send me a complete patch to use
INTDEF/INTUSE you get the effect you want.

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

Re: glob must not call globfree

[Andreas Schwab]

Roland McGrath <roland@frob.com> writes:

|> I don't have the spec handy but it seems unlikely to me that a caller
|> getting GLOB_NOSPACE is expected to call globfree.  Are you sure?
|> 

Here is the quote from the standard:

RETURN VALUE

       Upon successful completion, glob() shall return 0. The argument
       pglob->gl_pathc shall return the number of matched pathnames and
       the argument pglob->gl_pathv shall contain a pointer to a
       null-terminated list of matched and sorted pathnames. However, if
       pglob->gl_pathc is 0, the content of pglob->gl_pathv is undefined.

       The globfree() function shall not return a value.

       If glob() terminates due to an error, it shall return one of the
       non-zero constants defined in <glob.h>. The arguments
       pglob->gl_pathc and pglob->gl_pathv are still set as defined above.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: glob must not call globfree

[Andreas Schwab]

Ulrich Drepper <drepper@redhat.com> writes:

|> Andreas Schwab <schwab@suse.de> writes:
|> 
|> > 2002-02-07  Andreas Schwab  <schwab@suse.de>
|> > 
|> > 	* sysdeps/generic/glob.c: Don't call globfree on the passed glob
|> > 	structure.
|> 
|> I very much doubt this is correct.  I've never seen any requirement
|> like this.  If somebody intercepts the globfree calls for whatever
|> reasons and finds them handling memory which wasn't returned by a call
|> to glob this is the problem of that code.  Fortunately for whoever
|> makes this assumption one of the optimizations on the table for 2.3
|> will eliminate this.  If you send me a complete patch to use
|> INTDEF/INTUSE you get the effect you want.

I don't understand what you are trying to say.  Could you please clearify
what this issue has to do with INTDEF/INTUSE?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: glob must not call globfree

[Ulrich Drepper]

Andreas Schwab <schwab@suse.de> writes:

> I don't understand what you are trying to say.  Could you please clearify
> what this issue has to do with INTDEF/INTUSE?

If glob uses the globfree function via the INTUSE macro the call isn't
visible from the outside.

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

Re: glob must not call globfree

[Andreas Schwab]

Ulrich Drepper <drepper@redhat.com> writes:

|> Andreas Schwab <schwab@suse.de> writes:
|> 
|> > I don't understand what you are trying to say.  Could you please clearify
|> > what this issue has to do with INTDEF/INTUSE?
|> 
|> If glob uses the globfree function via the INTUSE macro the call isn't
|> visible from the outside.

You do not understand.  glob must *not* call globfree.  See POSIX.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: glob must not call globfree

[Thorsten Kukuk]

On Sat, Feb 09, Andreas Schwab wrote:

> Ulrich Drepper <drepper@redhat.com> writes:
> 
> |> Andreas Schwab <schwab@suse.de> writes:
> |> 
> |> > I don't understand what you are trying to say.  Could you please clearify
> |> > what this issue has to do with INTDEF/INTUSE?
> |> 
> |> If glob uses the globfree function via the INTUSE macro the call isn't
> |> visible from the outside.
> 
> You do not understand.  glob must *not* call globfree.  See POSIX.

I think at first we should clearify the problem:

In error case, we return a struct, where gl_pathc is not zero and
gl_pathv is undefined (because of the globfree before). According to
POSIX, this is wrong, even in error case the struct must have defined
values.
In the moment our problem is, if a program calls correct globfree(),
the program will seg.fault, because gl_pathc is not zero, but gl_pathv
is undefined what is not allowed according to POSIX.
If we apply Andreas Schwab patch, current, working programs will have
a memory leak.

I think the problem is our globfree: If called, it only checks if
gl_pathv is not NULL. I think it should also check, that gl_pathc
is not zero and set it to zero after freeing gl_pathv. This fixes
all of our current problems with this.

I suggest the following patch:

2002-02-09  Thorsten Kukuk  <kukuk@suse.de>

	* sysdeps/generic/glob.c (globfree): Only free memory if
	gl_pathc is not zero, set gl_pathc to zero afterwards.

--- sysdeps/generic/glob.c
+++ sysdeps/generic/glob.c	2002/02/09 15:00:04
@@ -1056,7 +1056,7 @@
 globfree (pglob)
      register glob_t *pglob;
 {
-  if (pglob->gl_pathv != NULL)
+  if (pglob->gl_pathc && pglob->gl_pathv != NULL)
     {
       size_t i;
       for (i = 0; i < pglob->gl_pathc; ++i)
@@ -1064,6 +1064,7 @@
 	  free ((__ptr_t) pglob->gl_pathv[pglob->gl_offs + i]);
       free ((__ptr_t) pglob->gl_pathv);
     }
+  pglob->gl_pathc = 0;
 }

-- 
Thorsten Kukuk       http://www.suse.de/~kukuk/        kukuk@suse.de
SuSE GmbH            Deutschherrenstr. 15-19       D-90429 Nuernberg
--------------------------------------------------------------------    
Key fingerprint = A368 676B 5E1B 3E46 CFCE  2D97 F8FD 4E23 56C6 FB4B

Re: glob must not call globfree

[Ulrich Drepper]

Thorsten Kukuk <kukuk@suse.de> writes:

> I think the problem is our globfree: If called, it only checks if
> gl_pathv is not NULL. I think it should also check, that gl_pathc
> is not zero and set it to zero after freeing gl_pathv. This fixes
> all of our current problems with this.

gl_pathc != 0 for a globfree call is a requirement on the user.  The
implementation shouldn't check it.

If anything has to be changed (I'm still not 100% sure but am willing
to concede it) the glob() function must make sure that gl_pathc is set
to zero if it called globfree().  I've checked in a patch for that.
Give it a try with your tests and please consider adding the tests to
the test suite.

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

Re: glob must not call globfree

[Thorsten Kukuk]

On Sun, Feb 10, Ulrich Drepper wrote:

> Thorsten Kukuk <kukuk@suse.de> writes:
> 
> > I think the problem is our globfree: If called, it only checks if
> > gl_pathv is not NULL. I think it should also check, that gl_pathc
> > is not zero and set it to zero after freeing gl_pathv. This fixes
> > all of our current problems with this.
> 
> gl_pathc != 0 for a globfree call is a requirement on the user.  The
> implementation shouldn't check it.
> 
> If anything has to be changed (I'm still not 100% sure but am willing
> to concede it) the glob() function must make sure that gl_pathc is set
> to zero if it called globfree().  I've checked in a patch for that.
> Give it a try with your tests 

If globfree is not modified it cannot work: We can call globfree with
a correct struct but it will seg.fault, since it inores gl_pathc.

> and please consider adding the tests to the test suite.

I will try to extract a small test case from it.

  Thorsten

-- 
Thorsten Kukuk       http://www.suse.de/~kukuk/        kukuk@suse.de
SuSE GmbH            Deutschherrenstr. 15-19       D-90429 Nuernberg
--------------------------------------------------------------------    
Key fingerprint = A368 676B 5E1B 3E46 CFCE  2D97 F8FD 4E23 56C6 FB4B