Re: [PATCH] http module is not checking correctly HTTP headers

[Glenn Washburn <development@efficientek.com>]

On Thu, 13 Jan 2022 23:14:54 +0100
Javier Moragon <jamofer@gmail.com> wrote:

> I'm sorry, it's my first time using a mailing list for publishing
> patches so I sent you the message directly instead of to grub-devel
> and my mail client wrapped the patch lines. I attached the patch
> Instead of pasting the diff, I hope this time the lines don't get
> wrapped.

Its okay, I had a few hiccups my first several times sending patches to
the list. Sending the patch as an attachment does fix the line wrapping
issue, but it creates another one. Its makes it more difficult for
reviewers to review it. This list is accustomed to having the patch
sent inline (in the text of the email). The advantage of this is that
reviewers get to comment inline on specific lines of the patch (like I
did for your original patch). Most people use git-format-patch and
git-send for creating and sending patches to the list.

Also, I've noticed that you changed the indentation of the second change
of this latest patch. Could you please create a new patch with the
indention exactly as it was? Your editor is probably changing the
indentation with out you realizing it. To be clear, the "sizeof" should
be preceeded by a string of 2 tabs followed by 2 spaces.

Glenn

> 
> El jue, 13 ene 2022 a las 5:08, Glenn Washburn
> (<development@efficientek.com>) escribió:
> >
> > On Wed, 12 Jan 2022 23:54:58 +0100
> > Javier Moragon <jamofer@gmail.com> wrote:
> >
> > > According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
> > > shall be case insensitive and we are now forced to read headers like
> > > `Content-Length` capitalized.
> > >
> > > The problem with that is when a HTTP server responds with a
> > > `content-length` header in lowercase GRUB gets stuck because HTTP
> > > module doesn't know the length of the transmision and the call never
> > > ends. I've been able to reproduce it and after ignoring the text case
> > > it worked perfectly.
> > >
> > > Here is it my patch proposal:
> > >
> > > diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> > > index b616cf40b..570fa3934 100644
> > > --- a/grub-core/net/http.c
> > > +++ b/grub-core/net/http.c
> > > @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
> > > char *ptr, grub_size_t len)
> > >        data->first_line_recv = 1;
> > >        return GRUB_ERR_NONE;
> > >      }
> > > -  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> > > +  if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
> > > ("Content-Length: ") )
> >
> > I don't think there should be a new line here. And why change to
> > grub_strlen? Now the length is calculated everytime at runtime instead
> > of once at compile time.
> >
> > >        == 0 && !data->size_recv)
> > >      {
> > >        ptr += sizeof ("Content-Length: ") - 1;
> > > @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
> > > char *ptr, grub_size_t len)
> > >        data->size_recv = 1;
> > >        return GRUB_ERR_NONE;
> > >      }
> > > -  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> > > -    sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> > > +  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> > > +    grub_strlen ("Transfer-Encoding: chunked") ) == 0)
> >
> > Ditto on the grub_strlen. I also don't like the original indentation of
> > this line and think that it should align with "ptr".
> >
> > >      {
> > >        data->chunked = 1;
> > >        return GRUB_ERR_NONE;
> >
> > Otherwise, it seems like good patch.
> >
> > Glenn
> >