Re: dwarves-dfsg: FTBFS on hurd-i386 (for review)
Hi!
A quick review follows:
On Wed, 2014-04-02 at 16:00:27 +0200, Svante Signell wrote:
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -1365,33 +1365,42 @@ int cus__load_dir(struct cus *self, stru
>
> err = 0;
> while ((entry = readdir(dir)) != NULL) {
> - char pathname[PATH_MAX];
> + char *pathname;
> + int len;
The correct type for memory sizes is size_t, but if the existing
codebase uses len, then just use that I guess.
> 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);
Error from malloc() not checked.
> + snprintf(pathname, len, "%s/%s",
> dirname, entry->d_name);
>
> err = lstat(pathname, &st);
> - if (err != 0)
> + if (err != 0) {
> + free(pathname);
> break;
> -
> + }
Pedantic, but probably you want to leave the blank line there.
> if (S_ISDIR(st.st_mode)) {
> if (!recursive)
> continue;
Leaks here.
>
> 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,11 @@ 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);
> + int len;
size_t?
> + 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.
> + pathname = malloc(len);
Error check.
> + snprintf(pathname, len, "%s.SUNW_ctf", self->filename);
> fd = creat(pathname, S_IRUSR | S_IWUSR);
> if (fd == -1) {
> fprintf(stderr, "%s: open(%s) failed!\n", __func__, pathname);
> @@ -736,13 +739,16 @@ found_SUNW_ctf_str:
> if (close(fd) < 0)
> goto out_unlink;
>
> - char cmd[PATH_MAX];
> + char *cmd;
> + len = 32 + strlen(pathname) + 1 + strlen(self->filename) + 1;
Missing allocation for cmd? Same comment for hardcoded string length.
> snprintf(cmd, sizeof(cmd), "objcopy --add-section .SUNW_ctf=%s %s",
> pathname, self->filename);
> if (system(cmd) == 0)
> err = 0;
> + free(cmd);
> out_unlink:
> unlink(pathname);
> + free(pathname);
> return err;
> #endif
> out_update:
> @@ -758,6 +764,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
> @@ -964,14 +964,14 @@ static struct argp ctracer__argp = {
>
> int main(int argc, char *argv[])
> {
> - int remaining, err;
> + int remaining, err, len;
size_t?
> 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;
> + char *methods_filename;
> + char *collector_filename;
> + char *classes_filename;
> struct structure *pos;
> FILE *fp_functions;
> int rc = EXIT_FAILURE;
> @@ -1048,7 +1048,10 @@ failure:
> goto out;
> }
>
> - snprintf(functions_filename, sizeof(functions_filename),
> + 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.
Actually, isn't there any usage of asprintf in the codebase? if there
is, just use that instead.
> + snprintf(functions_filename, len,
> "%s/%s.functions", src_dir, class__name(tag__class(class), cu));
> fp_functions = fopen(functions_filename, "w");
> if (fp_functions == NULL) {
> @@ -1057,7 +1060,9 @@ failure:
> goto out;
> }
>
> - snprintf(methods_filename, sizeof(methods_filename),
> + len = strlen(src_dir) + 20 + 1;
> + methods_filename = malloc(len);
> + snprintf(methods_filename, len,
> "%s/ctracer_methods.stp", src_dir);
> fp_methods = fopen(methods_filename, "w");
> if (fp_methods == NULL) {
> @@ -1066,7 +1071,9 @@ failure:
> goto out;
> }
>
> - snprintf(collector_filename, sizeof(collector_filename),
> + len = strlen(src_dir) + 20 + 1;
> + collector_filename = malloc(len);
> + snprintf(collector_filename, len,
> "%s/ctracer_collector.c", src_dir);
> fp_collector = fopen(collector_filename, "w");
> if (fp_collector == NULL) {
> @@ -1075,7 +1082,9 @@ failure:
> goto out;
> }
>
> - snprintf(classes_filename, sizeof(classes_filename),
> + len = strlen(src_dir) + 18 + 1;
> + classes_filename = malloc(len);
> + snprintf(classes_filename, len,
> "%s/ctracer_classes.h", src_dir);
> fp_classes = fopen(classes_filename, "w");
> if (fp_classes == NULL) {
> @@ -1144,6 +1153,10 @@ failure:
>
> rc = EXIT_SUCCESS;
> 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.
> cus__delete(methods_cus);
> dwarves__exit();
> return rc;
Thanks,
Guillem
Reply to: