Subject: bug#31439: closed ([PATCH] fts: avoid a memory leak edge case)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 14 May 2018 01:52:02 +0000

Your bug report

#31439: Possible memory leak in fts.c

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
31439: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=31439
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: [PATCH] fts: avoid a memory leak edge case
From: Pádraig Brady <[email-masked]>
To: [email-masked], [email-masked], bug-gnulib <[email-masked]>
Date: Sun, 13 May 2018 18:51:02 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 12/05/18 18:50, ISE Development wrote:
> Hi,
> 
> I may be wrong but I suspect there is a corner case where fts_close()
> will not free the FTSENT structures correctly if called immediately
> after fts_open().
> 
> After fts_open(), the current entry is a dummy entry created as
> follows:
> 
> if ((sp->fts_cur = fts_alloc(sp, "", 0)) == NULL)
>         goto mem3;
> sp->fts_cur->fts_link = root;
> sp->fts_cur->fts_info = FTS_INIT;
> 
> It would normally be freed during the first invocation of fts_read().
> 
> In fts_close():
> 
> if (sp->fts_cur) {
>         for (p = sp->fts_cur; p->fts_level >= FTS_ROOTLEVEL;) {
>                 freep = p;
>                 p = p->fts_link != NULL ? p->fts_link : p->fts_parent;
>                 free(freep);
>         }
>         free(p);
> }
> 
> However, fts_alloc() does not clear or set fts_level, nor does it zero
> the entire FTSENT structure.
> 
> So as far as I can figure, it is possible for the fts_level of the
> dummy entry to be negative after fts_open() causing fts_close() not to
> free the actual root level entries.

Yes valgrind indicates that fts_level is uninitialized if you
fts_close() right after fts_open().
The attached should fix it up.

thanks!
Pádraig

From 71b6724aa2e2843da7b73151d13c678452a59c7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sun, 13 May 2018 18:42:37 -0700
Subject: [PATCH] fts: avoid a memory leak edge case

* lib/fts.c (fts_open): Set an appropriate fts_level
so that an immediate fts_close() will free the allocation.
* tests/test-fts.c (fts_dealloc): Add a test case which
will trigger under valgrind or address sanitizer.
Fixes https://bugs.gnu.org/31439
---
 ChangeLog        |  9 +++++++++
 lib/fts.c        |  1 +
 tests/test-fts.c | 23 ++++++++++++++++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 24fd4da..e1b9c7e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2018-05-13  Pádraig Brady  <[email-masked]>
+
+	fts: avoid a memory leak edge case
+	* lib/fts.c (fts_open): Set an appropriate fts_level
+	so that an immediate fts_close() will free the allocation.
+	* tests/test-fts.c (fts_dealloc): Add a test case which
+	will trigger under valgrind or address sanitizer.
+	Fixes https://bugs.gnu.org/31439
+
 2018-05-13  Bruno Haible  <[email-masked]>
 
 	nl_langinfo: Fix compilation error on Android.
diff --git a/lib/fts.c b/lib/fts.c
index d543510..1ccc78c 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -546,6 +546,7 @@ fts_open (char * const *argv,
                 goto mem3;
         sp->fts_cur->fts_link = root;
         sp->fts_cur->fts_info = FTS_INIT;
+        sp->fts_cur->fts_level = 1;
         if (! setup_dir (sp))
                 goto mem3;
 
diff --git a/tests/test-fts.c b/tests/test-fts.c
index ad15aff..a9c1dd8 100644
--- a/tests/test-fts.c
+++ b/tests/test-fts.c
@@ -38,6 +38,23 @@ perror_exit (char const *message, int status)
   exit (status);
 }
 
+/* alloc/dealloc to ensure structures initialized appropriately.  */
+static void
+fts_dealloc (void)
+{
+  static char dir[] = "./";
+  static char *const curr_dir[2] = { dir, 0 };
+  FTSENT *e;
+  FTS *ftsp = fts_open (curr_dir, FTS_NOSTAT | FTS_PHYSICAL | FTS_CWDFD, 0);
+  if (ftsp)
+    {
+      if (fts_close (ftsp) != 0)
+        perror_exit ("fts_close", 9);
+    }
+  else
+    perror_exit (base, 10);
+}
+
 /* Remove BASE and all files under it.  */
 static void
 remove_tree (void)
@@ -122,9 +139,10 @@ main (void)
     perror_exit (base, 6);
   while ((e = fts_read (ftsp)))
     needles_seen += strcmp (e->fts_name, "needle") == 0;
-  fflush (stdout);
   if (errno)
     perror_exit ("fts_read", 7);
+  if (fts_close (ftsp) != 0)
+    perror_exit (base, 8);
 
   /* Report an error if we did not find the needles.  */
   if (needles_seen != needles)
@@ -140,5 +158,8 @@ main (void)
       fprintf (stderr, "fts could not remove directory\n");
       return 1;
     }
+
+  fts_dealloc ();
+
   return 0;
 }
-- 
2.9.3

Subject: Possible memory leak in fts.c
From: ISE Development <[email-masked]>
To: [email-masked]
Date: Sun, 13 May 2018 02:50:34 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi,

I may be wrong but I suspect there is a corner case where fts_close()
will not free the FTSENT structures correctly if called immediately
after fts_open().

After fts_open(), the current entry is a dummy entry created as
follows:

if ((sp->fts_cur = fts_alloc(sp, "", 0)) == NULL)
        goto mem3;
sp->fts_cur->fts_link = root;
sp->fts_cur->fts_info = FTS_INIT;

It would normally be freed during the first invocation of fts_read().

In fts_close():

if (sp->fts_cur) {
        for (p = sp->fts_cur; p->fts_level >= FTS_ROOTLEVEL;) {
                freep = p;
                p = p->fts_link != NULL ? p->fts_link : p->fts_parent;
                free(freep);
        }
        free(p);
}

However, fts_alloc() does not clear or set fts_level, nor does it zero
the entire FTSENT structure.

So as far as I can figure, it is possible for the fts_level of the
dummy entry to be negative after fts_open() causing fts_close() not to
free the actual root level entries.

-- isedev