Re: dwarves-dfsg: FTBFS on hurd-i386 (for review)
OK to submit?
On Thu, 2014-04-03 at 12:42 +0200, Svante Signell wrote:
> On Wed, 2014-04-02 at 18:07 +0200, Guillem Jover wrote:
> > Hi!
> >
> > A quick review follows:
> >
> > On Wed, 2014-04-02 at 16:00:27 +0200, Svante Signell wrote:
> > > --- a/dwarves.c
> > > +++ b/dwarves.c
>
> > > + int len;
> >
> > The correct type for memory sizes is size_t, but if the existing
> > codebase uses len, then just use that I guess.
>
> Changed to size_t
>
> > > + pathname = malloc(len);
> >
> > Error from malloc() not checked.
>
> Fixed
>
>
> > > -
> > > + }
> >
> > Pedantic, but probably you want to leave the blank line there.
>
> Fixed
>
> > > if (S_ISDIR(st.st_mode)) {
> > > if (!recursive)
> > > continue;
> >
> > Leaks here.
>
> Fixed
>
> > > --- a/libctf.c
> > > +++ b/libctf.c
>
> > > + int len;
> >
> > size_t?
>
> Yes
>
> > > + char *pathname;
> > > + len = strlen(self->filename) + 9 + 1;
> >
> > To make this future-proof, you might want to either use a macro or a
> > string constant (char foo[] = "";) for the string and use strlen() on
> > it (which should get constant folded by the compiler), instead of
> > hardcoding the string length 9.
>
> Fixed
>
> > > + pathname = malloc(len);
> >
> > Error check.
>
> Fixed
>
> > >
> > > + char *cmd;
> > > + len = 32 + strlen(pathname) + 1 + strlen(self->filename) + 1;
> >
> > Missing allocation for cmd? Same comment for hardcoded string length.
>
> Yes, thanks!
>
> > > snprintf(cmd, sizeof(cmd), "objcopy --add-section .SUNW_ctf=%s %s",
>
> You missed this: sizeof(cmd) -> len! (me too)
>
> > > --- a/ctracer.c
> > > +++ b/ctracer.c
>
> > > + int remaining, err, len;
> >
> > size_t?
>
> Yes
>
> > > + len = strlen(src_dir) + 1 +
> > > + strlen(class__name(tag__class(class), cu)) + 10 + 1;
> > > + functions_filename = malloc(len);
> >
> > Error check for this and all other allocations. Same comment here
> > about hardcoded string length.
>
> Done
>
> > Actually, isn't there any usage of asprintf in the codebase? if there
> > is, just use that instead.
>
> No, unfortunately not.
>
> > > out:
> > > + free(functions_filename);
> > > + free(methods_filename);
> > > + free(collector_filename);
> > > + free(classes_filename);
> >
> > This might try to free() garbage, as early exits will have these
> > variables uninitialized.
>
> All initialized to NULL. freeing a NULL pointer is a noop: If ptr is
> NULL, no operation is performed.
>
> Thanks Guillem, updated patch attached.
>
Reply to: