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

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

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

severity 36350 important

quit

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

Hi Ludovic,

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

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.

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

though.

Mark

Hi Mark,

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

OK, reverting sounds good to me.

No problem. Perhaps we should consider releasing 2.0.6 soon and use

that in Guix on ‘core-updates’.

Thanks,

Ludo’.

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

Sure, sounds like a good idea.

Mark

Hi,

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

Done in a152a67d3865cc6e7f9d7abd8f17a6e905b8e841. The test is simple

but would catch regressions like this one.

Ludo’.