Re: heap-use-after-free in rpl_glob

[Bruno Haible <bruno@clisp.org>]

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

Hi Paul,

> It seems that your patch is incomplete, as there's a memory leak in the 
> unlikely case that the malloc call fails.

Oops, indeed. Thanks for the review!

> Something like the attached patch instead, perhaps?

Hmm, this patch is freeing the same object through the variable 'dirname'
on one branch and through the variable 'previous_dirname' on the other
branch. A bit hard to understand, and thus likely to introduce more bugs
in the future, I would say. I prefer the attached one. Pushed.

Bruno


[-- Attachment #2: 0001-glob-Fix-use-after-free-bug.patch --]
[-- Type: text/x-patch, Size: 2740 bytes --]

From 717766da8926e36cf86015c4a49554baa854e8e6 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Fri, 17 Jan 2020 21:56:01 +0100
Subject: [PATCH] glob: Fix use-after-free bug.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Reported by Tim Rühsen <tim.ruehsen@gmx.de> in
<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00102.html>.

* lib/glob.c (__glob): Delay freeing dirname until after the use of
end_name.
---
 ChangeLog  | 9 +++++++++
 lib/glob.c | 8 ++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 643dba3..4f4718a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-01-17  Bruno Haible  <bruno@clisp.org>
+            Paul Eggert  <eggert@cs.ucla.edu>
+
+	glob: Fix use-after-free bug.
+	Reported by Tim Rühsen <tim.ruehsen@gmx.de> in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00102.html>.
+	* lib/glob.c (__glob): Delay freeing dirname until after the use of
+	end_name.
+
 2020-01-16  Siddhesh Poyarekar  <siddhesh@gotplt.org>
 
 	vcs-to-changelog: Fix parsing of fndecl without args.
diff --git a/lib/glob.c b/lib/glob.c
index a67cbb6..add5d93 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -843,10 +843,11 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               {
                 size_t home_len = strlen (p->pw_dir);
                 size_t rest_len = end_name == NULL ? 0 : strlen (end_name);
+                /* dirname contains end_name; we can't free it now.  */
+                char *prev_dirname =
+                  (__glibc_unlikely (malloc_dirname) ? dirname : NULL);
                 char *d;
 
-                if (__glibc_unlikely (malloc_dirname))
-                  free (dirname);
                 malloc_dirname = 0;
 
                 if (glob_use_alloca (alloca_used, home_len + rest_len + 1))
@@ -857,6 +858,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                     dirname = malloc (home_len + rest_len + 1);
                     if (dirname == NULL)
                       {
+                        free (prev_dirname);
                         scratch_buffer_free (&pwtmpbuf);
                         retval = GLOB_NOSPACE;
                         goto out;
@@ -868,6 +870,8 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                   d = mempcpy (d, end_name, rest_len);
                 *d = '\0';
 
+                free (prev_dirname);
+
                 dirlen = home_len + rest_len;
                 dirname_modified = 1;
               }
-- 
2.7.4