X-Authentication-Warning: delorie.com: mailnull set sender to djgpp-workers-bounces using -f
Sender: rich AT phekda DOT freeserve DOT co DOT uk
Message-ID: <3C3218C0.9F9BC198@phekda.freeserve.co.uk>
Date: Tue, 01 Jan 2002 20:14:56 +0000
From: Richard Dawe <rich AT phekda DOT freeserve DOT co DOT uk>
X-Mailer: Mozilla 4.77 [en] (X11; U; Linux 2.2.19 i586)
X-Accept-Language: de,fr
MIME-Version: 1.0
To: djgpp-workers AT delorie DOT com
Subject: Re: Memory leaks fixes
References: <319B464B DOT 652FB9D9 DOT 09ACFA57 AT netscape DOT net>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Reply-To: djgpp-workers AT delorie DOT com

Hello.

Alexander Aganichev wrote:
[snip]
> I've made two fixes for memory leaks in glob and fsext. I almost sure
> that no one cares about these leaks since they are almost impossible,
> but... :-)
[snip]
> diff -rup E:\Sources\djgpp\src\libc/fsext/fsext.c libc/fsext/fsext.c
> --- E:\Sources\djgpp\src\libc/fsext/fsext.c Thu Jun 29 11:37:10 2000
> +++ libc/fsext/fsext.c  Sat Dec 29 14:04:20 2001
> @@ -88,11 +88,17 @@ grow_table(int _fd)
> 
>    if (num_fds <= _fd)
>    {
> +    __FSEXT_entry *temp;
>      int old_fds = num_fds, i;
>      num_fds = (_fd+256) & ~255;
> -    fsext_list = (__FSEXT_entry *)realloc(fsext_list, num_fds * sizeof(__FSEXT_entry));
> -    if (fsext_list == 0)
> +    temp = (__FSEXT_entry *)realloc(fsext_list, num_fds * sizeof(__FSEXT_entry));
> +    if (temp == 0)
> +    {
> +      free(fsext_list);
> +      fsext_list = NULL;
>        return 1;
> +    }
> +    fsext_list = temp;
>      for (i=old_fds; i<num_fds; i++)
>      {
>        fsext_list[i].function = 0;

Why do you free fsext_list? Surely we should not free fsext_list. For
instance, say we have 256 FDs handled by various FSEXT handlers. We now
want to add another one, so grow_table() is called. But there is not
enough memory. If we free fsext_list, then these FDs may not be cleaned up
properly, when the program exits. This is because the clean up code
(__FSEXT_close_all()) will not be able to call the FSEXT handler close
functions for all the hooked FDs.

I think the code should also set num_fds back to old_fds, if the realloc
fails. If fsext_list is not freed, then num_fds should contain the correct
number of FDs in the list.

--------------------------------------------------------------------------
> diff -rup E:\Sources\djgpp\src\libc/posix/glob/glob.c libc/posix/glob/glob.c
> --- E:\Sources\djgpp\src\libc/posix/glob/glob.c Wed Oct 17 08:08:40 2001
> +++ libc/posix/glob/glob.c  Sat Dec 29 12:55:44 2001
> @@ -394,9 +394,14 @@ glob(const char *_pattern, int _flags, i
> 
>    if (flags & GLOB_APPEND)
>    {
> -    _pglob->gl_pathv = (char **)realloc(_pglob->gl_pathv, (l_ofs + _pglob->gl_pathc + save_count + 1) * sizeof(char *));
> -    if (_pglob->gl_pathv == 0)
> +    char **temp = (char **)realloc(_pglob->gl_pathv, (l_ofs + _pglob->gl_pathc + save_count + 1) * sizeof(char *));
> +    if (temp == 0)
> +    {
> +      free(_pglob->gl_pathv);
> +      _pglob->gl_pathv = NULL;
>        return GLOB_NOSPACE;
> +    }
> +    _pglob->gl_pathv = temp;
>      l_ptr = l_ofs + _pglob->gl_pathc;
>    }
>    else

This looks good to me.

A quick audit of the code shows that realloc is used unsafely (loosing
pointer to original memory block on failure) in the following files:

* src/makemake.c
* src/libc/posix/regex/regcomp.c
* src/libm/math/chew.c
* src/debug/common/dbgredir.c (doesn't even check realloc() return value)
* src/debug/edebug/unassmbl.c (doesn't even check realloc() return value)
* src/debug/fsdb/unassmbl.c (doesn't even check realloc() return value)
* src/stub/djasm.y
* zoneinfo/src/date.c
* zoneinfo/src/strftime.c
* src/mkdoc/mkdoc.cc
* src/utils/texi2ps/ifset.c
* tests/libc/ansi/string/strcspn.c (exit() on realloc() failure, so OK
really)
* tests/libclink/objs.cc
* tests/libclink/slist.cc

Thanks, bye, Rich =]

PS: Thanks for using -p in diff options. 8)

-- 
Richard Dawe [ http://www.phekda.freeserve.co.uk/richdawe/ ]