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: <3C38331A.4D044C9A@phekda.freeserve.co.uk>
Date: Sun, 06 Jan 2002 11:20:58 +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> <3C3218C0 DOT 9F9BC198 AT phekda DOT freeserve DOT co DOT uk> <3C36F39E DOT D32EDD38 AT phekda DOT freeserve DOT co DOT uk> <6551-Sat05Jan2002163435+0200-eliz AT is DOT elta DOT co DOT il>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Reply-To: djgpp-workers AT delorie DOT com

Hello.

Eli Zaretskii wrote:
[snip]
> 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?

No, but I'll test using these tips.

[snip]
> 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?

With the patch grow_table() is as below. We do realloc() unconditionally.
This may not have been clear from the diff.

static int
grow_table(int _fd)
{
  init();

  if (_fd < 0)
    return 1;

  if (num_fds <= _fd)
  {
    __FSEXT_entry *temp;
    int old_fds = num_fds, i;

    num_fds = (_fd+256) & ~255;
    temp = (__FSEXT_entry *)realloc(fsext_list, num_fds *
sizeof(__FSEXT_entry));
    if (temp == 0)
    {
      /* Keep the current fsext_list, so that we can tidy the FSEXTs
         up properly. */
      num_fds = old_fds;
      return 1;
    }
    fsext_list = temp;
    for (i=old_fds; i<num_fds; i++)
    {
      fsext_list[i].function = 0;
      fsext_list[i].data = 0;
    }
  }
  return 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;
> 
> 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?

Yes, you are right. The Unix98 spec (aka SUSv2) says this on its page for
glob() & globfree():

"Note that gl_pathc and gl_pathv have meaning even if glob() fails. This
allows glob() to report partial results in the event of an error. However,
if gl_pathc is 0, gl_pathv is unspecified even if glob() did not return an
error."

I'll make the code just return GLOB_NOSPACE on failure, with the previous
results intact.

> > --- 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.

Do you mean 'args' by 'the original string'? I don't see why we can return
that - it could have quotes and redirection symbols in it. Surely it's
better to return the cmd->command buffer we originally allocated, if we
can't shrink it? (This is what the patch does.)
 
> 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.
[snip]

It seems unlikely that this code will fail, due to memory exhaustion. But
I am concerned that the return code of realloc() was not checked.

Hmmm, perhaps it is better to free cmd->command, if realloc() fails, and
return -1? This is how redir_cmdline_parse() copes, if the allocation of
cmd->command fails in the first place.

As always, thanks for the help!

Bye, Rich =]

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