Re: [Bug-wget] [Patch] fix bug #44628 not honoring RFC 6266 in --content-disposition

[Giuseppe Scrivano]

Miquel Llobet <address@hidden> writes:

> From d71c7cc43689fc752dedb2a3500673f9981f7fc9 Mon Sep 17 00:00:00 2001
> From: Miquel Llobet <address@hidden>
> Date: Wed, 1 Apr 2015 17:32:50 +0200
> Subject: [PATCH] Fixed #44628 honoring RFC 6266 content-disposition
>
> src/http.c (parse_content_disposition): stores filename* and filename
> separately and choses filename* if available
> src/http.c (test_parse_content_disposition): added new tests for the
> reported bu

this line seems incomplete.

> ---
>  src/http.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/src/http.c b/src/http.c
> index 9994d13..1f98e5e 100644
> --- a/src/http.c
> +++ b/src/http.c
> @@ -1202,6 +1202,7 @@ parse_content_disposition (const char *hdr, char 
> **filename)
>    bool is_url_encoded = false;
>  
>    *filename = NULL;
> +  char *encodedFilename = NULL;

declaration after statement, please move it one line up.

>    for ( ; extract_param (&hdr, &name, &value, ';', &is_url_encoded);
>          is_url_encoded = false)
>      {
> @@ -1218,17 +1219,27 @@ parse_content_disposition (const char *hdr, char 
> **filename)
>            if (value.b == value.e)
>              continue;
>  
> -          if (*filename)
> -            append_value_to_filename (filename, &value, is_url_encoded);
> +          /* Check if the name is "filename*" as specified in RFC 6266. 

trailing whitespace here.

I find useful to have these lines

[color]
        branch = auto
        diff = auto
        interactive = auto
        status = auto

in my ~/.gitconfig file.


> +           * Since "filename" could be broken up as "filename*N" (RFC 2231),
> +           * a check is needed to make sure this is not the case */
> +          int isEncodedFilename = *name.e == '*'&& !c_isdigit(*(name.e + 1));

Please use a "bool" instead of "int".

Space before && and after the function name: c_isdigit (*(name.e + 1));

> +          char **outFilename = isEncodedFilename ? &encodedFilename : 
> filename;

both isEncodedFilename and outFilename must be declared immediately
after the '{' before any code statement.


> +          if (*outFilename)
> +            append_value_to_filename (outFilename, &value, is_url_encoded);
>            else
>              {
> -              *filename = strdupdelim (value.b, value.e);
> +              *outFilename = strdupdelim (value.b, value.e);
>                if (is_url_encoded)
> -                url_unescape (*filename);
> +                url_unescape (*outFilename);
>              }
>          }
>      }
>  
> +  if (encodedFilename)
> +    {
> +      xfree (*filename);
> +      *filename = encodedFilename;
> +    }

using filename as a temporary in case !encodedFilename is fine, but
maybe we can make it clearer and use two different local temporary
values (encodedFilename and unencodedFilename) and set *filename
according to which one is available?  What do you think?

if (encodedFilename)
  {
    xfree (unencodedfilename);
    *filename = encodedFilename;
  }
else
  {
    xfree (encodedfilename);
    *filename = unencodedFilename;
  }

Regards,
Giuseppe