Memleak in glob()

Memleak in glob()

[Tim Rühsen]

[-- Attachment #1.1: Type: text/plain, Size: 330 bytes --]

Hi,

fuzzing glob.c immediately discovered a leak.

At ~L600 in glob.c, 'dirname' is heap allocated.
It is free'd at label 'out', but some code paths directly return without 
jumping there.

Attached is a patch fixing the issue for me, but just take it as a proof of 
concept. You might prefer a different approach.

Regards, Tim

[-- Attachment #1.2: 0001-Fix-memleak-in-glob.patch --]
[-- Type: text/x-patch, Size: 5056 bytes --]

From=200b7efcb91485a808edd4a39c58d6dfe267b4097b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= <tim.ruehsen@gmx.de>
Date: Sat, 1 Jul 2017 20:35:55 +0200
Subject: [PATCH] Fix memleak in glob()

---
 lib/glob.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/glob.c b/lib/glob.c
index d4fdc1737..ec3cce66f 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -306,6 +306,7 @@ next_brace_sub (const char *cp, int flags)
 
 #endif /* !defined GLOB_ONLY_P */
 
+#define RETURNVAL(r) retval=(r); goto out
 /* Do glob searching for PATTERN, placing results in PGLOB.
    The bits defined above may be set in FLAGS.
    If a directory cannot be opened or read and ERRFUNC is not nil,
@@ -1035,7 +1036,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                 if (flags & GLOB_TILDE_CHECK)
                   /* We have to regard it as an error if we cannot find the
                      home directory.  */
-                  return GLOB_NOMATCH;
+                  RETURNVAL(GLOB_NOMATCH);
               }
           }
         }
@@ -1067,7 +1068,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               free (pglob->gl_pathv);
               pglob->gl_pathv = NULL;
               pglob->gl_pathc = 0;
-              return GLOB_NOSPACE;
+              RETURNVAL (GLOB_NOSPACE);
             }
 
           new_gl_pathv = realloc (pglob->gl_pathv,
@@ -1101,11 +1102,11 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
           ++pglob->gl_pathc;
           pglob->gl_flags = flags;
 
-          return 0;
+          RETURNVAL (0);
         }
 
       /* Not found.  */
-      return GLOB_NOMATCH;
+      RETURNVAL (GLOB_NOMATCH);
     }
 
   meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE));
@@ -1151,7 +1152,9 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
       if (status != 0)
         {
           if ((flags & GLOB_NOCHECK) == 0 || status != GLOB_NOMATCH)
-            return status;
+		    {
+              RETURNVAL (status);
+            }
           goto no_matches;
         }
 
@@ -1170,7 +1173,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
             if (interrupt_state)
               {
                 globfree (&dirs);
-                return GLOB_ABORTED;
+                RETURNVAL (GLOB_ABORTED);
               }
           }
 #endif /* SHELL.  */
@@ -1189,7 +1192,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               globfree (&dirs);
               globfree (pglob);
               pglob->gl_pathc = 0;
-              return status;
+              RETURNVAL (status);
             }
 
           /* Stick the directory on the front of each name.  */
@@ -1200,7 +1203,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               globfree (&dirs);
               globfree (pglob);
               pglob->gl_pathc = 0;
-              return GLOB_NOSPACE;
+              RETURNVAL (GLOB_NOSPACE);
             }
         }
 
@@ -1222,7 +1225,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                 {
                 nospace2:
                   globfree (&dirs);
-                  return GLOB_NOSPACE;
+                  RETURNVAL (GLOB_NOSPACE);
                 }
 
               new_gl_pathv = realloc (pglob->gl_pathv,
@@ -1237,7 +1240,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                   globfree (&dirs);
                   globfree (pglob);
                   pglob->gl_pathc = 0;
-                  return GLOB_NOSPACE;
+                  RETURNVAL (GLOB_NOSPACE);
                 }
 
               ++pglob->gl_pathc;
@@ -1249,7 +1252,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
           else
             {
               globfree (&dirs);
-              return GLOB_NOMATCH;
+              RETURNVAL (GLOB_NOMATCH);
             }
         }
 
@@ -1295,7 +1298,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               flags = orig_flags;
               goto no_matches;
             }
-          return status;
+          RETURNVAL (status);
         }
 
       if (dirlen > 0)
@@ -1307,7 +1310,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
             {
               globfree (pglob);
               pglob->gl_pathc = 0;
-              return GLOB_NOSPACE;
+              RETURNVAL (GLOB_NOSPACE);
             }
         }
     }
@@ -1332,7 +1335,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               {
                 globfree (pglob);
                 pglob->gl_pathc = 0;
-                return GLOB_NOSPACE;
+                RETURNVAL (GLOB_NOSPACE);
               }
             strcpy (&new[len - 2], "/");
             pglob->gl_pathv[i] = new;
-- 
2.11.0


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Memleak in glob()

[Bruno Haible]

Hi Tim,

> Attached is a patch fixing the issue for me

The 'return 0;' in line 1104 is correct, because 'dirname' is stuffed into
the result array in the lines before. Therefore dirname must NOT be freed
here.

For the other ones, you may be right, but I wonder why scan.coverity.com did
not display them? Probably we need to upload a new build to scan.coverity.com.
Which brings me to a status question: Did you find volunteers for the triage
of the "defects" on scan.coverity.com? I stopped looking into it when you
said you would find some people.

Ultimately, for functions as complex as glob(), I now trust coverity.com
more than any person's review (including mine myself). Therefore I would like
to pursue the approach of using scan.coverity.com.

Bruno

Re: Memleak in glob()

[Tim Rühsen]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

Hi Bruno,

On Sonntag, 2. Juli 2017 02:34:17 CEST Bruno Haible wrote:
> Hi Tim,
> 
> > Attached is a patch fixing the issue for me
> 
> The 'return 0;' in line 1104 is correct, because 'dirname' is stuffed into
> the result array in the lines before. Therefore dirname must NOT be freed
> here.

This is right for one path, but not all the paths before 'return 0' stuff 
'dirname' into that array.
IMO, the correct approach would be to reset 'malloc_dirname' whenever 
'dirname' is stuffed/moved somewhere. In code that I am responsible for, 
especially complex code as glob(), I also set pointers to NULL after free() or 
when assigning/moving to another variable.

> For the other ones, you may be right, but I wonder why scan.coverity.com did
> not display them? Probably we need to upload a new build to
> scan.coverity.com. Which brings me to a status question: Did you find
> volunteers for the triage of the "defects" on scan.coverity.com? I stopped
> looking into it when you said you would find some people.

Sadly, I was too optimistic. I didn't forget about, and working on those is 
still on my list. But currently I am overwhelmed with stuff that has higher 
priority.

> Ultimately, for functions as complex as glob(), I now trust coverity.com
> more than any person's review (including mine myself). Therefore I would
> like to pursue the approach of using scan.coverity.com.

That's always a good idea. You could set up a cron job to automatically upload 
to Coverity, e.g. once a week or once a day. You can find instructions one the 
'upload for file analysis' page.

I am currently into fuzzing which is not only good for finding *real* issues 
(leaks, slowness, illegal memory access, integer overflow, ...). Fuzzing also 
provides you with test data that (nearly) fully covers all code paths. Those 
test inputs can (and should) be integrated into a project's test suite.

Are you going to GHM ? Wish, I could meet you there.

Regards, Tim

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Memleak in glob()

[Bruno Haible]

Hi Tim,

> > Did you find
> > volunteers for the triage of the "defects" on scan.coverity.com? I stopped
> > looking into it when you said you would find some people.
> 
> Sadly, I was too optimistic. I didn't forget about, and working on those is 
> still on my list. But currently I am overwhelmed with stuff that has higher 
> priority.

OK, this means the "find some people" is not going to happen soon. That I'll
continue the review on scan.coverity.com (as I don't find it _that_ low
priority, especially when I can concentrate on specific categories of issues).

> You could set up a cron job to automatically upload 
> to Coverity, e.g. once a week or once a day. You can find instructions one the 
> 'upload for file analysis' page.

Good idea. I haven't found a 'upload for file analysis' page, but the
instructions (including the mandatory, project specific token) are on the
project's "Upload a Project Build" page. In progress...

> > The 'return 0;' in line 1104 is correct, because 'dirname' is stuffed into
> > the result array in the lines before. Therefore dirname must NOT be freed
> > here.
> 
> This is right for one path, but not all the paths before 'return 0' stuff 
> 'dirname' into that array.

Ah, indeed. We should really rerun coverity after making changes to glob.c.

> In code that I am responsible for, 
> especially complex code as glob(), I also set pointers to NULL after free() or 
> when assigning/moving to another variable.

Such a coding style does not help. What you need is to reduce the complexity,
either by introducing inline functions, or by at least reducing the scope
of the variables in the big function.

The only situations where "set pointers to NULL after free()" is useful are:
  - when you're dealing with global variables or heap-allocated struct members
    (or class members in C++), or
  - when there is a (conservative) garbage collector around.

Bruno

Re: Memleak in glob()

[Tim Rühsen]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

Hi Bruno,

On Sonntag, 2. Juli 2017 14:00:27 CEST Bruno Haible wrote:
> > In code that I am responsible for,
> > especially complex code as glob(), I also set pointers to NULL after
> > free() or when assigning/moving to another variable.
> 
> Such a coding style does not help. What you need is to reduce the
> complexity, either by introducing inline functions, or by at least reducing
> the scope of the variables in the big function.
> 
> The only situations where "set pointers to NULL after free()" is useful are:
> - when you're dealing with global variables or heap-allocated struct
> members (or class members in C++), or
>   - when there is a (conservative) garbage collector around.

What I meant (in general) is the avoidance of 'use after free' [1] issues.
Not leaving around dangling pointers avoids this, so erroneous code is more 
unlikely to become a security issue (it's just a segmentation fault then).

In glob() the code is not resetting the dirname memory policy flag  when 
'dirname' is stuffed into another variable. No doing so can easily lead to 
either a double free and/or a use after free.

Regards, Tim

[1] https://www.owasp.org/index.php/Using_freed_memory

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Memleak in glob()

[Paul Eggert]

On 07/01/2017 01:44 PM, Tim Rühsen wrote:
> Hi,
>
> fuzzing glob.c immediately discovered a leak.
>
> At ~L600 in glob.c, 'dirname' is heap allocated.
> It is free'd at label 'out', but some code paths directly return without
> jumping there.
>
> Attached is a patch fixing the issue for me, but just take it as a proof of
> concept. You might prefer a different approach.
>
> Regards, Tim

glob.c is taken from glibc, right? Have you investigated whether these 
problems have been reported and/or fixed in glibc?

Re: Memleak in glob()

[Tim Rühsen]

[-- Attachment #1.1: Type: text/plain, Size: 829 bytes --]

On 07/03/2017 01:22 AM, Paul Eggert wrote:
> On 07/01/2017 01:44 PM, Tim Rühsen wrote:
>> Hi,
>>
>> fuzzing glob.c immediately discovered a leak.
>>
>> At ~L600 in glob.c, 'dirname' is heap allocated.
>> It is free'd at label 'out', but some code paths directly return without
>> jumping there.
>>
>> Attached is a patch fixing the issue for me, but just take it as a
>> proof of
>> concept. You might prefer a different approach.
>>
>> Regards, Tim
> 
> glob.c is taken from glibc, right? Have you investigated whether these
> problems have been reported and/or fixed in glibc?

I don't know if glibc takes the code from gnulib or the other way round.
But a quick look at [1] around L1012 looks like the same issue in glibc.

[1] https://code.woboq.org/userspace/glibc/posix/glob.c.html

Regards, Tim


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Memleak in glob()

[Tim Rühsen]

[-- Attachment #1.1: Type: text/plain, Size: 889 bytes --]

Password reset at
https://sourceware.org/bugzilla/enter_bug.cgi?product=glibc currently
doesn't work (yes, also checked in spam dir). Don't remember my pw, too
long ago.


So maybe someone else can open a bug with a link to this thread !?


With Best Regards, Tim


On 07/03/2017 01:22 AM, Paul Eggert wrote:
> On 07/01/2017 01:44 PM, Tim Rühsen wrote:
>> Hi,
>>
>> fuzzing glob.c immediately discovered a leak.
>>
>> At ~L600 in glob.c, 'dirname' is heap allocated.
>> It is free'd at label 'out', but some code paths directly return without
>> jumping there.
>>
>> Attached is a patch fixing the issue for me, but just take it as a
>> proof of
>> concept. You might prefer a different approach.
>>
>> Regards, Tim
> 
> glob.c is taken from glibc, right? Have you investigated whether these
> problems have been reported and/or fixed in glibc?
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Memleak in glob()

[Bruno Haible]

Tim Rühsen wrote:
> > > Attached is a patch fixing the issue for me
> > 
> > The 'return 0;' in line 1104 is correct, because 'dirname' is stuffed into
> > the result array in the lines before. Therefore dirname must NOT be freed
> > here.
> 
> This is right for one path, but not all the paths before 'return 0' stuff 
> 'dirname' into that array.

Fixed like this. Let's see what remaining issues Coverity reports in glob.c
(next Monday).


2017-07-06  Bruno Haible  <bruno@clisp.org>

	glob: Fix more memory leaks.
	* lib/glob.c (glob): Free dirname before returning.
	Reported by Coverity and Tim Rühsen.

diff --git a/lib/glob.c b/lib/glob.c
index dc0aff6..a38cf22 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -1091,6 +1091,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               p = mempcpy (pglob->gl_pathv[newcount], dirname, dirlen);
               p[0] = '/';
               p[1] = '\0';
+              if (__glibc_unlikely (malloc_dirname))
+                free (dirname);
             }
           else
             {

Re: Memleak in glob()

[Bruno Haible]

Hi Tim,

I wrote:
> Fixed like this. Let's see what remaining issues Coverity reports in glob.c
> (next Monday).

Now, coverity does not report any resource leak for lib/glob.c any more. But
IMO your analysis about the memory leak is still mostly correct. Except around
line 1070, where your patch would add a double-free, I think you're right.

So, I've applied this change. I'm not introducing a new macro RETURNVAL
because that does not seem to fit with the glibc coding style.


2017-07-10  Tim Rühsen  <tim.ruehsen@gmx.de>
            Bruno Haible  <bruno@clisp.org>

	glob: Fix more memory leaks.
	* lib/glob.c (glob): Use 'goto out' in order to free dirname before
	returning.
	Reported by Tim Rühsen.

diff --git a/lib/glob.c b/lib/glob.c
index a38cf22..3b3194a 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -1039,9 +1039,12 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                 free (malloc_pwtmpbuf);
 
                 if (flags & GLOB_TILDE_CHECK)
-                  /* We have to regard it as an error if we cannot find the
-                     home directory.  */
-                  return GLOB_NOMATCH;
+                  {
+                    /* We have to regard it as an error if we cannot find the
+                       home directory.  */
+                    retval = GLOB_NOMATCH;
+                    goto out;
+                  }
               }
           }
         }
@@ -1068,12 +1071,11 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
           if (newcount > SIZE_MAX / sizeof (char *) - 2)
             {
             nospace:
-              if (__glibc_unlikely (malloc_dirname))
-                free (dirname);
               free (pglob->gl_pathv);
               pglob->gl_pathv = NULL;
               pglob->gl_pathc = 0;
-              return GLOB_NOSPACE;
+              retval = GLOB_NOSPACE;
+              goto out;
             }
 
           new_gl_pathv = realloc (pglob->gl_pathv,
@@ -1113,7 +1115,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
         }
 
       /* Not found.  */
-      return GLOB_NOMATCH;
+      retval = GLOB_NOMATCH;
+      goto out;
     }
 
   meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE));
@@ -1159,7 +1162,10 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
       if (status != 0)
         {
           if ((flags & GLOB_NOCHECK) == 0 || status != GLOB_NOMATCH)
-            return status;
+            {
+              retval = status;
+              goto out;
+            }
           goto no_matches;
         }
 
@@ -1178,7 +1184,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
             if (interrupt_state)
               {
                 globfree (&dirs);
-                return GLOB_ABORTED;
+                retval = GLOB_ABORTED;
+                goto out;
               }
           }
 #endif /* SHELL.  */
@@ -1197,7 +1204,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               globfree (&dirs);
               globfree (pglob);
               pglob->gl_pathc = 0;
-              return status;
+              retval = status;
+              goto out;
             }
 
           /* Stick the directory on the front of each name.  */
@@ -1208,7 +1216,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               globfree (&dirs);
               globfree (pglob);
               pglob->gl_pathc = 0;
-              return GLOB_NOSPACE;
+              retval = GLOB_NOSPACE;
+              goto out;
             }
         }
 
@@ -1230,7 +1239,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                 {
                 nospace2:
                   globfree (&dirs);
-                  return GLOB_NOSPACE;
+                  retval = GLOB_NOSPACE;
+                  goto out;
                 }
 
               new_gl_pathv = realloc (pglob->gl_pathv,
@@ -1245,7 +1255,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                   globfree (&dirs);
                   globfree (pglob);
                   pglob->gl_pathc = 0;
-                  return GLOB_NOSPACE;
+                  retval = GLOB_NOSPACE;
+                  goto out;
                 }
 
               ++pglob->gl_pathc;
@@ -1257,7 +1268,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
           else
             {
               globfree (&dirs);
-              return GLOB_NOMATCH;
+              retval = GLOB_NOMATCH;
+              goto out;
             }
         }
 
@@ -1303,7 +1315,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               flags = orig_flags;
               goto no_matches;
             }
-          return status;
+          retval = status;
+          goto out;
         }
 
       if (dirlen > 0)
@@ -1315,7 +1328,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
             {
               globfree (pglob);
               pglob->gl_pathc = 0;
-              return GLOB_NOSPACE;
+              retval = GLOB_NOSPACE;
+              goto out;
             }
         }
     }
@@ -1340,7 +1354,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               {
                 globfree (pglob);
                 pglob->gl_pathc = 0;
-                return GLOB_NOSPACE;
+                retval = GLOB_NOSPACE;
+                goto out;
               }
             strcpy (&new[len - 2], "/");
             pglob->gl_pathv[i] = new;