Re: gputils: FTBFS on hurd-i386 (for review)

[Guillem Jover <guillem@debian.org>]

On Wed, 2017-04-05 at 16:19:28 +0200, Svante Signell wrote:
> Thank you for the review. Attached is an updated patch. Is it OK now? I still
> have to check return values of snprintf and realloc and maybe also switch to
> using alloca (not POSIX though).

I've got the feeling we have covered similar ground in the past, but
let's see. And I'd keep away from alloca().

> Index: gputils-1.4.0/gplink/gplink.c
> ===================================================================
> --- gputils-1.4.0.orig/gplink/gplink.c
> +++ gputils-1.4.0/gplink/gplink.c
> @@ -321,17 +321,23 @@ gplink_open_coff(const char *name)
>    gp_object_type *object;
>    gp_archive_type *archive;
>    FILE *coff;
> -  char file_name[PATH_MAX + 1];
> -
> -  strncpy(file_name, name, sizeof(file_name));
> +  char *file_name = NULL;

The variable is unconditionally assigned almost immediately, there's
IMO no point in this assignment. Ditto other instances.

> +  int len = 0;
>  
> +  file_name = strdup(name);
> +  if (file_name == NULL) {
> +    perror(name);
> +    exit(1);
> +  }
>    coff = fopen(file_name, "rb");
>    if ((coff == NULL) && (strchr(file_name, PATH_CHAR) == 0)) {
>      /* If no "/" in name, try searching include pathes. */
>      int i;
>  
>      for (i = 0; i < state.numpaths; i++) {
> -      snprintf(file_name, sizeof(file_name), "%s" COPY_CHAR "%s", state.paths[i], name);
> +      len = snprintf(NULL, 0, "%s" COPY_CHAR "%s", state.paths[i], name) + 1;
> +      file_name = realloc(file_name, len);

If you are going to keep using realloc, then it seems like a waste to
resize the buffer, when we need less memory for example. Ditto other
instances.

> +      snprintf(file_name, len, "%s" COPY_CHAR "%s", state.paths[i], name);
>        coff = fopen(file_name, "rb");
>  
>        if (coff != NULL) {
> @@ -341,6 +347,8 @@ gplink_open_coff(const char *name)
>    }
>  
>    if (coff == NULL) {
> +    if (file_name)
> +      free(file_name);

There's never a need to check whether a pointer is NULL before a free().
Ditto other instances.

>      perror(name);
>      exit(1);
>    }

Thanks,
Guillem