GNU bug report logs - #31439
Possible memory leak in fts.c

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31439 in the body.
You can then email your comments to 31439 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: ISE Development <isedev <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: Possible memory leak in fts.c
Date: Sun, 13 May 2018 02:50:34 +0100

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

Message #8 received at 31439 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: isedev <at> gmail.com, 31439 <at> debbugs.gnu.org
Subject: Re: bug#31439: Possible memory leak in fts.c
Date: Sun, 13 May 2018 17:53:46 -0700

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.

valgrind should tell us. I tweaked chmod to call fts_close()
right after xfts_open() and got:

==21011== Conditional jump or move depends on uninitialised value(s)
==21011==    at 0x4066C6: fts_close (fts.c:609)
==21011==    by 0x401B7F: process_files (chmod.c:337)
==21011==    by 0x401B7F: main (chmod.c:572)

Just as you surmised.

Patch coming up... (to gnulib)

thanks!
Pádraig

Message #13 received at 31439-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: isedev <at> gmail.com, 31439-done <at> debbugs.gnu.org,
 bug-gnulib <bug-gnulib <at> gnu.org>
Subject: [PATCH] fts: avoid a memory leak edge case
Date: Sun, 13 May 2018 18:51:02 -0700

[Message part 1 (text/plain, inline)]

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

[fts-dealloc.patch (text/x-patch, attachment)]

Message #16 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: isedev <at> gmail.com, bug-gnulib <bug-gnulib <at> gnu.org>, bug-coreutils <at> gnu.org,
 31439-done <at> debbugs.gnu.org
Subject: Re: bug#31439: [PATCH] fts: avoid a memory leak edge case
Date: Mon, 14 May 2018 10:06:53 +0200

On Monday, May 14, 2018 3:51:02 AM CEST Pádraig Brady wrote:
> @@ -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)

Why are you removing fflush (stdout) from the test without any explanation?

Kamil

Message #22 received at 31439-done <at> debbugs.gnu.org (full text, mbox):

From: Bruno Haible <bruno <at> clisp.org>
To: bug-gnulib <at> gnu.org
Cc: isedev <at> gmail.com, Kamil Dudka <kdudka <at> redhat.com>, bug-coreutils <at> gnu.org,
 Pádraig Brady <P <at> draigbrady.com>,
 31439-done <at> debbugs.gnu.org
Subject: Re: bug#31439: [PATCH] fts: avoid a memory leak edge case
Date: Mon, 14 May 2018 10:28:53 +0200

Kamil Dudka wrote:
> Why are you removing fflush (stdout) from the test without any explanation?

Yes, fflush(stdout) statements are extremely important if you want to
understand/debug test failures on native Windows.

Bruno

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.