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

DoneSubmitted by Ludovic Courtès.
Details
3 participants
  • Dan Frumin
  • Ludovic Courtès
  • Mark H Weaver
Owner
unassigned
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 & wget -O - http://localhost:8080
You’ll notice that ‘wget’ hangs (never receives a response) because theserver 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 oneline. 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 acorrect fix would look like. Mark, WDYT?
I also noticed that there are no unit tests for (web server), which weshould 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, andreturn 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 EOFis 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
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 importantquit
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 HTTPcontinuation lines, before my commit referenced above, although itneglects 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 commite1225d013ed8673382d6d8f9300dd6b175c8b820 on the stable-2.2 branch.I tried leaving the new test in place, but it failed due to the lack ofwhitespace 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 usethat 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 simplebut would catch regressions like this one.
Ludo’.
Closed
?
Your comment

This issue is archived.

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