Re: [Groff] Re: groff 1.17: pre-grohtml's unsafe temporary file handling & other improvements

[Ralph Corderoy]

Hi Gaius,

> here are some patches which include:

More great work -- where do you get the time!

> @@ -2929,6 +2935,6 @@
>  
>  static void usage(FILE *stream)
>  {
> -  fprintf(stream, "usage: %s [-vld?n] [-F dir] [files ...]\n",
> +  fprintf(stream, "usage: %s [-vld?n] [-I image_stem] [-F dir] [files 
> ...]\n",
>         program_name);
>  }

Should `-D dir' be in here too?

> -void makeFileName ()
> +static void makeFileName (void)
>  {
>    char buffer[8192];
>  
> -  sprintf(buffer, "grohtml-%d", (int)getpid());
> +  if (image_dir == NULL) {
> +    image_dir = (char *)malloc(1);
> +    image_dir[0] = (char)0;
> +  } else if ((strlen(image_dir)>1) && (image_dir[strlen(image_dir)-1] != 
> '/')) {
> +    sprintf(buffer, "%s/", image_dir);
> +    image_dir = strdup(buffer);
> +  }
> +
> +  if (image_template == NULL)
> +    sprintf(buffer, "%sgrohtml-%d", image_dir, (int)getpid());
> +  else
> +    sprintf(buffer, "%s%s", image_dir, image_template);
> +
>    strcat(buffer, "-%d");
>    image_template = (char *)malloc(strlen(buffer)+1);
>    strcpy(image_template, buffer);
>  }

Apologies, I haven't looked at the code outside the patch so this might
be normal but buffer may overflow when sprintf'd to.

The return value of `malloc(1)' isn't checked, given that image_dir is
never freed it might be simpler to say

    image_dir = "";

and avoid the malloc.

Likewise, strdup's return value isn't checked.

Also, the `strlen(image_dir)>1' seems to suggest I can't specify `-D x'
because strlen("x") == 1.  Does it want to be `*image_dir' instead?

malloc and strcpy are used at the end of the routine, would strdup be
simpler and equivalent?

Sorry to just butt in and comment on your patch without lifting a
finger myself.  Please don't think I'm ungrateful.


Ralph.