bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Ludovic Courtès]

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

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Dan Frumin]

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

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Dan Frumin]

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

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Mark H Weaver]

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

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Ludovic Courtès]

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

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Mark H Weaver]

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

bug#36350: [2.2.5] ‘read-headers’ blocks, thereby breaking web servers

[Ludovic Courtès]

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