misc/162396: [patch] remove loop in globpexp1()@lib/libc/gen/glob.c

[Cheng-Lung Sung]

>Number:         162396
>Category:       misc
>Synopsis:       [patch] remove loop in globpexp1()@lib/libc/gen/glob.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          change-request
>Submitter-Id:   current-users
>Arrival-Date:   Wed Nov 09 05:10:08 UTC 2011
>Closed-Date:
>Last-Modified:
>Originator:     Cheng-Lung Sung
>Release:        FreeBSD 8.2-RELEASE amd64
>Organization:
FreeBSD @ Taiwan
>Environment:
System: FreeBSD going04.iis.sinica.edu.tw 8.2-RELEASE FreeBSD 8.2-RELEASE #0: Tue Mar 8 12:09:34 CST 2011 root at going04.iis.sinica.edu.tw:/usr/obj/amd64/usr/src/sys/GOING04_SMP amd64
>Description:

One can see that globexp1() is called with GLOB_BRACE.
    
In globexp1(), examine the pattern: 
    only "{}" => call glob0()
    no LBRACE => call glob0()
    with LBRACE => call globexp2 to deal with expression inside LBRACE

Take a look in glob0():
    glob0, return 0 if success, otherwise error (except GLOB_NOMATCH)
        => return err
        => return globextend()/GLOB_NOMATCH
        => return 0 

In globexp2()
    => non matched braces
        => call glob0(), return 0
    => matched brace-pair
        => parse brace
            => reach rbrace, call globexp1(), set return value to *rv

    set *rv = 0, return 0

*which implies always return 0 in globexp2*
As a result, the following code block in globexp1 can be reduced from:
-       while ((ptr = g_strchr(ptr, LBRACE)) != NULL)
-               if (!globexp2(ptr, pattern, pglob, &rv, limit))
-                       return rv;
to:
+       if ((ptr = g_strchr(ptr, LBRACE)) != NULL)
+               return globexp2(ptr, pattern, pglob, limit)


Besides, since *rv should indicate the acctual return value, the setting *rv = 0 and return 0 is ambiguous.
So the flow in globexp2() should be:

In globexp2()
    => non matched braces
        => call glob0(), return 0
    => matched brace-pair
        => parse brace
            => reach rbrace, call globexp1(), set return value to *rv
                => if success in glob0
                    return 0
                => if failed in glob0 (but not GLOB_NOMATCH)
                    return err


>How-To-Repeat:
	
>Fix:


diff --git a/lib/libc/gen/glob.c b/lib/libc/gen/glob.c
index c3d9f08..40c5808 100644
--- a/lib/libc/gen/glob.c
+++ b/lib/libc/gen/glob.c
@@ -156,7 +156,7 @@ static int	 globextend(const Char *, glob_t *, size_t *);
 static const Char *	
 		 globtilde(const Char *, Char *, size_t, glob_t *);
 static int	 globexp1(const Char *, glob_t *, size_t *);
-static int	 globexp2(const Char *, const Char *, glob_t *, int *, size_t *);
+static int	 globexp2(const Char *, const Char *, glob_t *, size_t *);
 static int	 match(Char *, Char *, Char *);
 #ifdef DEBUG
 static void	 qprintf(const char *, Char *);
@@ -240,15 +240,13 @@ static int
 globexp1(const Char *pattern, glob_t *pglob, size_t *limit)
 {
 	const Char* ptr = pattern;
-	int rv;
 
 	/* Protect a single {}, for find(1), like csh */
 	if (pattern[0] == LBRACE && pattern[1] == RBRACE && pattern[2] == EOS)
 		return glob0(pattern, pglob, limit);
 
-	while ((ptr = g_strchr(ptr, LBRACE)) != NULL)
-		if (!globexp2(ptr, pattern, pglob, &rv, limit))
-			return rv;
+	if ((ptr = g_strchr(ptr, LBRACE)) != NULL)
+		return globexp2(ptr, pattern, pglob, limit)
 
 	return glob0(pattern, pglob, limit);
 }
@@ -260,9 +258,10 @@ globexp1(const Char *pattern, glob_t *pglob, size_t *limit)
  * If it fails then it tries to glob the rest of the pattern and returns.
  */
 static int
-globexp2(const Char *ptr, const Char *pattern, glob_t *pglob, int *rv, size_t *limit)
+globexp2(const Char *ptr, const Char *pattern, glob_t *pglob, size_t *limit)
 {
 	int     i;
+	int     rv;
 	Char   *lm, *ls;
 	const Char *pe, *pm, *pm1, *pl;
 	Char    patbuf[MAXPATHLEN];
@@ -297,8 +296,7 @@ globexp2(const Char *ptr, const Char *pattern, glob_t *pglob, int *rv, size_t *l
 
 	/* Non matching braces; just glob the pattern */
 	if (i != 0 || *pe == EOS) {
-		*rv = glob0(patbuf, pglob, limit);
-		return 0;
+		return glob0(patbuf, pglob, limit);
 	}
 
 	for (i = 0, pl = pm = ptr; pm <= pe; pm++)
@@ -344,7 +342,9 @@ globexp2(const Char *ptr, const Char *pattern, glob_t *pglob, int *rv, size_t *l
 #ifdef DEBUG
 				qprintf("globexp2:", patbuf);
 #endif
-				*rv = globexp1(patbuf, pglob, limit);
+				rv = globexp1(patbuf, pglob, limit);
+				if (rv && rv != GLOB_NOMATCH) /* if errors occured */
+					return rv;
 
 				/* move after the comma, to the next string */
 				pl = pm + 1;
@@ -354,7 +354,6 @@ globexp2(const Char *ptr, const Char *pattern, glob_t *pglob, int *rv, size_t *l
 		default:
 			break;
 		}
-	*rv = 0;
 	return 0;
 }
 

>Release-Note:
>Audit-Trail:
>Unformatted: