Mail Archives: djgpp-workers/2002/01/06/05:43:13
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/ ]
- Raw text -