[2.2.5] ‘read-headers’ blocks, thereby breaking web servers

  • Done
  • quality assurance status badge
Details
3 participants
  • Dan Frumin
  • Ludovic Courtès
  • Mark H Weaver
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
important
L
L
Ludovic Courtès wrote on 24 Jun 2019 12:32
(address . bug-guile@gnu.org)(name . Mark H Weaver)(address . mhw@netris.org)
87pnn3b7f1.fsf@gnu.org
Hello,

In Guile 2.2.5, if you run:

./meta/guile examples/web/hello.scm &

You’ll notice that ‘wget’ hangs (never receives a response) because the
server is actually stuck in a read(2) call that will never complete, in
‘read-headers’.

Reverting 73cde5ed7218a090ecee888870908af5445796f0 solves the problem.

AIUI, before that commit, ‘read-header-line’ would read exactly one
line. After this change, it calls ‘lookahead-char’, which can block,
and that’s exactly what’s happening here.

I don’t know how HTTP continuation lines work, so I’m not sure what a
correct fix would look like. Mark, WDYT?

I also noticed that there are no unit tests for (web server), which we
should probably address while we’re at it. :-)

Thanks,
Ludo’.
D
D
Dan Frumin wrote on 24 Jun 2019 14:05
Re: [2.2.5] ‘read-headers’ blocks, the reby breaking web servers
(address . 36350@debbugs.gnu.org)
9061114e-67e8-7ec2-1d09-b1600ab07582@cs.ru.nl
I believe that `(lookahead-char port)` really blocks when the client has finished sending the request and there is no more data from `port` to consume.
If I understand it correctly, then per HTTP/1.1 [1] the request ends with CRLF at the last line, and then comes the message. So I we have read an
empty string, then we shouldn't proceed with further lookaheads.

Specifically, the following code works out for me:


(define (read-header-line port)
"Read an HTTP header line, including any continuation lines, and
return the combined string without its final CRLF or LF. Raise a
'bad-header' exception if the line does not end in CRLF or LF, or if EOF
is reached."
(format #t "Reading header line now: ")
(match (%read-line port)
(((? string? line) . #\newline)
;; '%read-line' does not consider #\return a delimiter; so if it's
;; there, remove it. We are more tolerant than the RFC in that we
;; tolerate LF-only endings.
(let ((line (if (string-suffix? "\r" line)
(string-drop-right line 1)
line)))
;; If the next character is a space or tab, then there's at least
;; one continuation line. Read the continuation lines by calling
;; 'read-header-line' recursively, and append them to this header
;; line, folding the leading spaces and tabs to a single space.
(if (and (not (string-null? line))
(space-or-tab? (lookahead-char port)))
(string-append line " " (string-trim (read-header-line port)
spaces-and-tabs))
line)))
((line . _) ;EOF or missing delimiter
(bad-header 'read-header-line line))))

Moreover, the continuation lines in general have been deprecated: [2].
I have to say I would be in favor of removing support for continuation lines in general.

Best regards,
-Dan


L
L
Ludovic Courtès wrote on 24 Jun 2019 14:31
control message for bug #36350
(address . control@debbugs.gnu.org)
87imsvb1wc.fsf@gnu.org
severity 36350 important
quit
D
D
Dan Frumin wrote on 24 Jun 2019 14:44
Re: [2.2.5] ‘read-headers’ blocks, the reby breaking web servers
(address . 36350@debbugs.gnu.org)
7d306db9-87d9-0b1d-8ecb-7486ae982349@cs.ru.nl
By the way, I've just tested the web server in Google Chrome, and it works fine!

I was told that a (potential) reason for that is that Chrome sends TCP FIN to the server, which closes the socket for reading, and then
`lookahead-char` sees eof.

Best,
Dan
M
M
Mark H Weaver wrote on 24 Jun 2019 17:36
Re: bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 36350@debbugs.gnu.org)
87h88f0zbv.fsf@netris.org
Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (13 lines)
> ./meta/guile examples/web/hello.scm &
> wget -O - http://localhost:8080
>
> You’ll notice that ‘wget’ hangs (never receives a response) because the
> server is actually stuck in a read(2) call that will never complete, in
> ‘read-headers’.
>
> Reverting 73cde5ed7218a090ecee888870908af5445796f0 solves the problem.
>
> AIUI, before that commit, ‘read-header-line’ would read exactly one
> line. After this change, it calls ‘lookahead-char’, which can block,
> and that’s exactly what’s happening here.

Gah, indeed!

Also, I see now that there was already some existing code to handle HTTP
continuation lines, before my commit referenced above, although it
neglects to fold the whitespace after the CRLF into a single SP octet.
I'm not sure how I failed to notice that.

The commit should simply be reverted, I think. Pushed as commit
e1225d013ed8673382d6d8f9300dd6b175c8b820 on the stable-2.2 branch.
I tried leaving the new test in place, but it failed due to the lack of
whitespace folding in the previous code.

I really messed up here, sorry.

Toggle quote (3 lines)
> I also noticed that there are no unit tests for (web server), which we
> should probably address while we’re at it. :-)

Yes, that would be great. I won't be able to do it anytime soon,
though.

Mark
L
L
Ludovic Courtès wrote on 24 Jun 2019 20:56
(name . Mark H Weaver)(address . mhw@netris.org)(address . 36350@debbugs.gnu.org)
87lfxq95ht.fsf@gnu.org
Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (27 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> ./meta/guile examples/web/hello.scm &
>> wget -O - http://localhost:8080
>>
>> You’ll notice that ‘wget’ hangs (never receives a response) because the
>> server is actually stuck in a read(2) call that will never complete, in
>> ‘read-headers’.
>>
>> Reverting 73cde5ed7218a090ecee888870908af5445796f0 solves the problem.
>>
>> AIUI, before that commit, ‘read-header-line’ would read exactly one
>> line. After this change, it calls ‘lookahead-char’, which can block,
>> and that’s exactly what’s happening here.
>
> Gah, indeed!
>
> Also, I see now that there was already some existing code to handle HTTP
> continuation lines, before my commit referenced above, although it
> neglects to fold the whitespace after the CRLF into a single SP octet.
> I'm not sure how I failed to notice that.
>
> The commit should simply be reverted, I think. Pushed as commit
> e1225d013ed8673382d6d8f9300dd6b175c8b820 on the stable-2.2 branch.
> I tried leaving the new test in place, but it failed due to the lack of
> whitespace folding in the previous code.

OK, reverting sounds good to me.

Toggle quote (2 lines)
> I really messed up here, sorry.

No problem. Perhaps we should consider releasing 2.0.6 soon and use
that in Guix on ‘core-updates’.

Thanks,
Ludo’.
M
M
Mark H Weaver wrote on 25 Jun 2019 09:22
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 36350@debbugs.gnu.org)
875zoucemt.fsf@netris.org
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (3 lines)
> Perhaps we should consider releasing 2.0.6 soon and use that in Guix
> on ‘core-updates’.

Sure, sounds like a good idea.

Mark
L
L
Ludovic Courtès wrote on 30 Jun 2019 21:50
(name . Mark H Weaver)(address . mhw@netris.org)(address . 36350-done@debbugs.gnu.org)
878stisvie.fsf@gnu.org
Hi,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (6 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>> Perhaps we should consider releasing 2.0.6 soon and use that in Guix
>> on ‘core-updates’.
>
> Sure, sounds like a good idea.

Done in a152a67d3865cc6e7f9d7abd8f17a6e905b8e841. The test is simple
but would catch regressions like this one.

Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 36350@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 36350
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch