Re: dwarves-dfsg: FTBFS on hurd-i386 (for review)
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!
> > --- 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.
--- a/dwarves.c
+++ b/dwarves.c
@@ -1365,33 +1365,46 @@ int cus__load_dir(struct cus *self, stru
err = 0;
while ((entry = readdir(dir)) != NULL) {
- char pathname[PATH_MAX];
+ char *pathname;
+ size_t len;
struct stat st;
if (strcmp(entry->d_name, ".") == 0 ||
strcmp(entry->d_name, "..") == 0)
continue;
- snprintf(pathname, sizeof(pathname), "%s/%s",
+ len = strlen(dirname) + 1 + strlen(entry->d_name) + 1;
+ pathname = malloc(len);
+ if (pathname == NULL)
+ break;
+ snprintf(pathname, len, "%s/%s",
dirname, entry->d_name);
err = lstat(pathname, &st);
- if (err != 0)
+ if (err != 0) {
+ free(pathname);
break;
+ }
if (S_ISDIR(st.st_mode)) {
- if (!recursive)
+ if (!recursive) {
+ free(pathname);
continue;
-
+ }
err = cus__load_dir(self, conf, pathname,
filename_mask, recursive);
- if (err != 0)
+ if (err != 0) {
+ free(pathname);
break;
+ }
} else if (fnmatch(filename_mask, entry->d_name, 0) == 0) {
err = cus__load_file(self, conf, pathname);
- if (err != 0)
+ if (err != 0) {
+ free(pathname);
break;
+ }
}
+ free(pathname);
}
if (err == -1)
--- a/libctf.c
+++ b/libctf.c
@@ -723,8 +723,15 @@ found_SUNW_ctf_str:
gelf_update_shdr(newscn, &shdr_mem);
elf_flagshdr(newscn, ELF_C_SET, ELF_F_DIRTY);
#else
- char pathname[PATH_MAX];
- snprintf(pathname, sizeof(pathname), "%s.SUNW_ctf", self->filename);
+ size_t len;
+ char *pathname;
+ static const char *ctf_name = ".SUNW_ctf";
+
+ len = strlen(self->filename) + strlen(ctf_name) + 1;
+ pathname = malloc(len);
+ if (pathname == NULL)
+ goto out_close;
+ snprintf(pathname, len, "%s%s", self->filename, ctf_name);
fd = creat(pathname, S_IRUSR | S_IWUSR);
if (fd == -1) {
fprintf(stderr, "%s: open(%s) failed!\n", __func__, pathname);
@@ -736,13 +743,22 @@ found_SUNW_ctf_str:
if (close(fd) < 0)
goto out_unlink;
- char cmd[PATH_MAX];
- snprintf(cmd, sizeof(cmd), "objcopy --add-section .SUNW_ctf=%s %s",
+ char *cmd;
+ static const char *ctf_command = "objcopy --add-section ";
+
+ len = strlen(ctf_command) + strlen(ctf_name) + 1 +
+ strlen(pathname) + 1 + strlen(self->filename) + 1;
+ cmd = malloc(len);
+ if (cmd == NULL)
+ goto out_close;
+ snprintf(cmd, len, "%s%s=%s %s", ctf_command, ctf_name,
pathname, self->filename);
if (system(cmd) == 0)
err = 0;
+ free(cmd);
out_unlink:
unlink(pathname);
+ free(pathname);
return err;
#endif
out_update:
@@ -758,6 +774,7 @@ out_update:
elf_end(elf);
err = 0;
out_close:
+ free(pathname);
if (bf != self->buf)
free(bf);
close(fd);
--- a/ctracer.c
+++ b/ctracer.c
@@ -965,13 +965,14 @@ static struct argp ctracer__argp = {
int main(int argc, char *argv[])
{
int remaining, err;
+ size_t len;
struct tag *class;
struct cu *cu;
char *filename;
- char functions_filename[PATH_MAX];
- char methods_filename[PATH_MAX];
- char collector_filename[PATH_MAX];
- char classes_filename[PATH_MAX];
+ char *functions_filename = NULL;
+ char *methods_filename = NULL;
+ char *collector_filename = NULL;
+ char *classes_filename = NULL;
struct structure *pos;
FILE *fp_functions;
int rc = EXIT_FAILURE;
@@ -1048,8 +1049,15 @@ failure:
goto out;
}
- snprintf(functions_filename, sizeof(functions_filename),
- "%s/%s.functions", src_dir, class__name(tag__class(class), cu));
+ static const char *functions_ext = ".functions";
+ len = strlen(src_dir) + 1 +
+ strlen(class__name(tag__class(class), cu)) +
+ strlen(functions_ext) + 1;
+ functions_filename = malloc(len);
+ if (functions_filename == NULL)
+ goto out;
+ snprintf(functions_filename, len, "%s/%s%s",
+ src_dir, class__name(tag__class(class), cu), functions_ext);
fp_functions = fopen(functions_filename, "w");
if (fp_functions == NULL) {
fprintf(stderr, "ctracer: couldn't create %s\n",
@@ -1057,8 +1065,13 @@ failure:
goto out;
}
- snprintf(methods_filename, sizeof(methods_filename),
- "%s/ctracer_methods.stp", src_dir);
+ static const char *methods_name = "ctracer_methods.stp";
+ len = strlen(src_dir) + 1 + strlen(methods_name) + 1;
+ methods_filename = malloc(len);
+ if (methods_filename == NULL)
+ goto out;
+ snprintf(methods_filename, len,
+ "%s/%s", src_dir, methods_name);
fp_methods = fopen(methods_filename, "w");
if (fp_methods == NULL) {
fprintf(stderr, "ctracer: couldn't create %s\n",
@@ -1066,8 +1079,13 @@ failure:
goto out;
}
- snprintf(collector_filename, sizeof(collector_filename),
- "%s/ctracer_collector.c", src_dir);
+ static const char *collector_name = "ctracer_collector.c";
+ len = strlen(src_dir) + 1 + strlen(collector_name) + 1;
+ collector_filename = malloc(len);
+ if (collector_filename == NULL)
+ goto out;
+ snprintf(collector_filename, len,
+ "%s/%s", src_dir, collector_name);
fp_collector = fopen(collector_filename, "w");
if (fp_collector == NULL) {
fprintf(stderr, "ctracer: couldn't create %s\n",
@@ -1075,8 +1093,13 @@ failure:
goto out;
}
- snprintf(classes_filename, sizeof(classes_filename),
- "%s/ctracer_classes.h", src_dir);
+ static const char *classes_name = "ctracer_classes.h";
+ len = strlen(src_dir) + 1 + strlen(classes_name) + 1;
+ classes_filename = malloc(len);
+ if (classes_filename == NULL)
+ goto out;
+ snprintf(classes_filename, len,
+ "%s/%s", src_dir, classes_name);
fp_classes = fopen(classes_filename, "w");
if (fp_classes == NULL) {
fprintf(stderr, "ctracer: couldn't create %s\n",
@@ -1144,6 +1167,10 @@ failure:
rc = EXIT_SUCCESS;
out:
+ free(functions_filename);
+ free(methods_filename);
+ free(collector_filename);
+ free(classes_filename);
cus__delete(methods_cus);
dwarves__exit();
return rc;
Reply to: