* [PATCH] http module is not checking correctly HTTP headers
@ 2022-01-12 22:54 Javier Moragon
2022-01-13 4:08 ` Glenn Washburn
0 siblings, 1 reply; 6+ messages in thread
From: Javier Moragon @ 2022-01-12 22:54 UTC (permalink / raw)
To: grub-devel
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: ") )
== 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)
{
data->chunked = 1;
return GRUB_ERR_NONE;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] http module is not checking correctly HTTP headers
2022-01-12 22:54 [PATCH] http module is not checking correctly HTTP headers Javier Moragon
@ 2022-01-13 4:08 ` Glenn Washburn
2022-01-13 22:14 ` Javier Moragon
0 siblings, 1 reply; 6+ messages in thread
From: Glenn Washburn @ 2022-01-13 4:08 UTC (permalink / raw)
To: Javier Moragon; +Cc: The development of GNU GRUB
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] http module is not checking correctly HTTP headers
2022-01-13 4:08 ` Glenn Washburn
@ 2022-01-13 22:14 ` Javier Moragon
2022-01-13 23:05 ` Jamo
2022-01-13 23:10 ` Glenn Washburn
0 siblings, 2 replies; 6+ messages in thread
From: Javier Moragon @ 2022-01-13 22:14 UTC (permalink / raw)
To: development; +Cc: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]
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.
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
>
[-- Attachment #2: patch.patch --]
[-- Type: application/x-patch, Size: 1007 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] http module is not checking correctly HTTP headers
2022-01-13 22:14 ` Javier Moragon
@ 2022-01-13 23:05 ` Jamo
2022-01-14 3:26 ` Glenn Washburn
2022-01-13 23:10 ` Glenn Washburn
1 sibling, 1 reply; 6+ messages in thread
From: Jamo @ 2022-01-13 23:05 UTC (permalink / raw)
To: jamofer; +Cc: The development of GNU GRUB
From: Javier Moragon <jamofer@gmail.com>
I applied the last suggestion in this patch and I've finally realized
how to use git send-email.
I'm sorry for the unnecesary mails and I hope for next contributions
I won't make the same mistakes, Thank you!
---
grub-core/net/http.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index b616cf40b..7a56ec7ef 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: ", sizeof ("Content-Length: ") - 1)
== 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",
+ sizeof ("Transfer-Encoding: chunked") - 1) == 0)
{
data->chunked = 1;
return GRUB_ERR_NONE;
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] http module is not checking correctly HTTP headers
2022-01-13 23:05 ` Jamo
@ 2022-01-14 3:26 ` Glenn Washburn
0 siblings, 0 replies; 6+ messages in thread
From: Glenn Washburn @ 2022-01-14 3:26 UTC (permalink / raw)
To: Jamo; +Cc: The development of GNU GRUB
On Fri, 14 Jan 2022 00:05:59 +0100
Jamo <jamofer@gmail.com> wrote:
> From: Javier Moragon <jamofer@gmail.com>
>
> I applied the last suggestion in this patch and I've finally realized
> how to use git send-email.
>
> I'm sorry for the unnecesary mails and I hope for next contributions
> I won't make the same mistakes, Thank you!
No worries, we all had to learn at some point. There are still a couple
issues. One is that this space is for the commit message. For instance,
based on your first email it might be:
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.
Another issue is that when you update a patch, you should update its
version number. See the "-v" option in git-format-patch. Also, the list
convention is to start a new thread when submitting an updated patch.
So no need to do have an "In-Reply-To" header on the next update.
>
> ---
If you want to have some text that will not get recorded in the commit,
like the text after your email above, you can put that here.
> grub-core/net/http.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..7a56ec7ef 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: ", sizeof ("Content-Length: ") - 1)
> == 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",
> + sizeof ("Transfer-Encoding: chunked") - 1) == 0)
Almost there. The code style guidelines for this project use a TAB
character for every 8 spaces. It looks like you used all spaces to do
the indent.
> {
> data->chunked = 1;
> return GRUB_ERR_NONE;
Glenn
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] http module is not checking correctly HTTP headers
2022-01-13 22:14 ` Javier Moragon
2022-01-13 23:05 ` Jamo
@ 2022-01-13 23:10 ` Glenn Washburn
1 sibling, 0 replies; 6+ messages in thread
From: Glenn Washburn @ 2022-01-13 23:10 UTC (permalink / raw)
To: Javier Moragon; +Cc: The development of GNU GRUB
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-14 3:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 22:54 [PATCH] http module is not checking correctly HTTP headers Javier Moragon
2022-01-13 4:08 ` Glenn Washburn
2022-01-13 22:14 ` Javier Moragon
2022-01-13 23:05 ` Jamo
2022-01-14 3:26 ` Glenn Washburn
2022-01-13 23:10 ` Glenn Washburn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.