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

[Miquel Llobet]

Thanks for the review and git tips! I made all the changes you suggested.

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?

I think so too, it's easier to understand this way.

Miquel Llobet



On Thu, Apr 2, 2015 at 3:01 PM, Giuseppe Scrivano <address@hidden> wrote:

> 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
>

0001-Fixed-44628-honoring-RFC-6266-content-disposition.patch
Description: Binary data