X-Authentication-Warning: delorie.com: mailnull set sender to djgpp-workers-bounces using -f
Date: Sat, 05 Jan 2002 16:34:36 +0200
From: "Eli Zaretskii" <eliz AT is DOT elta DOT co DOT il>
Sender: halo1 AT zahav DOT net DOT il
To: Richard Dawe <rich AT phekda DOT freeserve DOT co DOT uk>
Message-Id: <6551-Sat05Jan2002163435+0200-eliz@is.elta.co.il>
X-Mailer: emacs 21.1.50 (via feedmail 8 I) and Blat ver 1.8.9
CC: djgpp-workers AT delorie DOT com
In-reply-to: <3C36F39E.D32EDD38@phekda.freeserve.co.uk> (message from Richard
	Dawe on Sat, 05 Jan 2002 12:37:50 +0000)
Subject: Re: Memory leaks fixes
References: <319B464B DOT 652FB9D9 DOT 09ACFA57 AT netscape DOT net> <3C3218C0 DOT 9F9BC198 AT phekda DOT freeserve DOT co DOT uk> <3C36F39E DOT D32EDD38 AT phekda DOT freeserve DOT co DOT uk>
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

> Date: Sat, 05 Jan 2002 12:37:50 +0000
> From: Richard Dawe <rich AT phekda DOT freeserve DOT co DOT uk>
> 
> Please find below a diff that fixes a couple of realloc memory leaks in:
> 
> src/libc/fsext/fsext.c
> src/libc/posix/glob/glob.c
> src/libc/posix/regex/regcomp.c

Thanks.

> The debuggers seem to behave the same way as stock 2.03, at least in a
> small test with a zippo debugging binary using 'whereis' in edebug32 or
> ALT+W in fsdb.

The changes in dbgredir cannot be tested in anything but GDB, since
GDB is the only debugger that can redirect the debuggee's standard
handles.

More generally, to test this code, you would need to trigger a
condition where realloc returns NULL.  Did you do that?

>    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)
> +    {

Why do you only call realloc if fsext_list is NULL?  AFAICS, it
should be called unconditionally, since this code expands the table,
not only creates it from scratch.  Am I missing something?

> +    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;

Wow! isn't that a bit too harsh: you nuke the entire data just
because the array couldn't be expanded.  Isn't it better to return
what you have so far, even if it's incomplete?

> --- src/debug/common/dbgredir.c 1999/06/14 16:20:41     1.3
> +++ src/debug/common/dbgredir.c 2002/01/05 12:27:57
> @@ -452,8 +452,10 @@ redir_cmdline_parse (const char *args, c
>         }
>      }
>    *d = '\0';
> -  /* Free unused space.  */
> -  cmd->command = (char *) realloc (cmd->command, cmd_len + 1);
> +  /* Free unused space, if we can.  */
> +  d = (char *) realloc (cmd->command, cmd_len + 1);
> +  if (d)
> +    cmd->command = d;

No, please don't!  This code compacts the string, and thus the result
will always be _smaller_ than the original.  So if realloc fails here,
the right thing to do would be to return the original string.

I didn't make allowances for realloc's failure because reducing the
buffer size should always succeed.  If you think the call to realloc
could fail in this case, please tell what kind of situation could
cause that.

In any case, if such a situation is possible, I'd prefer to have a
safer code which will not lose the command line due to compaction.