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

DoneSubmitted by Ludovic Courtès.
L
Ludovic Courtès wrote on Mon Jun 24 12:32:18+0200 2019
(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 &
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.

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
Dan Frumin wrote on Mon Jun 24 14:05:57+0200 2019
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


[1]: https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html
[2]: https://tools.ietf.org/html/rfc7230#section-3.2.4
L
Ludovic Courtès wrote on Mon Jun 24 14:31:31+0200 2019
control message for bug #36350
(address . control@debbugs.gnu.org)
87imsvb1wc.fsf@gnu.org
severity 36350 important
quit
D
Dan Frumin wrote on Mon Jun 24 14:44:58+0200 2019
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
Mark H Weaver wrote on Mon Jun 24 11:36:41-0400 2019
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:

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

> 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
Ludovic Courtès wrote on Mon Jun 24 20:56:46+0200 2019
(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:

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

> 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
Mark H Weaver wrote on Tue Jun 25 03:22:55-0400 2019
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 36350@debbugs.gnu.org)
875zoucemt.fsf@netris.org
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.

Mark
L
Ludovic Courtès wrote on Sun Jun 30 21:50:17+0200 2019
(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:

> 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

Comments via the web interface are not currently supported. To comment on this conversation send email to 36350@debbugs.gnu.org

3 participants
  • Dan Frumin
  • Ludovic Courtès
  • Mark H Weaver
Owner
unassigned
Severity
important
Status
Done