[PATCH 1/2] web: Handle ending CRLF (\r\n) for chunked input and output ports.

[PATCH 1/2] web: Handle ending CRLF (\r\n) for chunked input and output ports.

[Christopher Baines]

The chunked transfer encoding specifies the chunked body ends with
CRLF. This is in addition to the CRLF at the end of the last chunk, so
there should be CRLF twice at the end of the chunked body:

  https://datatracker.ietf.org/doc/html/rfc2616#section-3.6.1

* module/web/http.scm (make-chunked-input-port): Read two extra bytes at
the end of the chunked input.
(make-chunked-output-port): Write the missing \r\n when closing the
port.
* test-suite/tests/web-http.test (chunked encoding): Add missing \r\n to
test data.
---
 module/web/http.scm            | 3 ++-
 test-suite/tests/web-http.test | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/module/web/http.scm b/module/web/http.scm
index 4276e1744..163c52176 100644
--- a/module/web/http.scm
+++ b/module/web/http.scm
@@ -1977,6 +1977,7 @@ closed it will also close PORT, unless the KEEP-ALIVE? is true."
                (cond
                 ((zero? size)
                  (set! finished? #t)
+                 (get-bytevector-n port 2) ; \r\n follows the last chunk
                  num-read)
                 (else
                  (loop to-read num-read)))))
@@ -2031,7 +2032,7 @@ KEEP-ALIVE? is true."
         (put-string port "\r\n"))))
   (define (close)
     (flush)
-    (put-string port "0\r\n")
+    (put-string port "0\r\n\r\n")
     (force-output port)
     (unless keep-alive?
       (close-port port)))
diff --git a/test-suite/tests/web-http.test b/test-suite/tests/web-http.test
index 63377349c..86fbc0e1a 100644
--- a/test-suite/tests/web-http.test
+++ b/test-suite/tests/web-http.test
@@ -421,7 +421,7 @@
                  '((basic (realm . "guile")))))
 
 (with-test-prefix "chunked encoding"
-  (let* ((s "5\r\nFirst\r\nA\r\n line\n Sec\r\n8\r\nond line\r\n0\r\n")
+  (let* ((s "5\r\nFirst\r\nA\r\n line\n Sec\r\n8\r\nond line\r\n0\r\n\r\n")
          (p (make-chunked-input-port (open-input-string s))))
     (pass-if-equal
         "First line\n Second line"
@@ -497,4 +497,4 @@
            (force-output out-chunked)
            (display "Third chunk" out-chunked)
            (close-port out-chunked))))
-      "b\r\nFirst chunk\r\nc\r\nSecond chunk\r\nb\r\nThird chunk\r\n0\r\n"))
+      "b\r\nFirst chunk\r\nc\r\nSecond chunk\r\nb\r\nThird chunk\r\n0\r\n\r\n"))
-- 
2.36.1

[PATCH 2/2] web: Don't hide missing data in the chunked input port.

[Christopher Baines]

This port is of limited use if it cannot be used reliably. Rather than
behaving as if the input has finished when it ends unexpectedly, instead
raise an exception.

* module/web/http.scm (make-chunked-input-port): Raise an exception on
premature termination.
(&chunked-input-ended-prematurely): New exception type.
(chunked-input-ended-prematurely-error?): New procedure.
* test-suite/tests/web-http.test (pass-if-named-exception): Rename to
pass-if-named-exception.
(pass-if-named-exception): New syntax.
("Exception on premature chunk end"): New test for this behaviour.
---
 doc/ref/web.texi               |  3 +++
 module/web/http.scm            | 18 ++++++++++++++++--
 test-suite/tests/web-http.test | 25 +++++++++++++++++--------
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/doc/ref/web.texi b/doc/ref/web.texi
index 93cd0214f..b04d328b7 100644
--- a/doc/ref/web.texi
+++ b/doc/ref/web.texi
@@ -1117,6 +1117,9 @@ Returns a new port, that transparently reads and decodes chunk-encoded
 data from @var{port}. If no more chunk-encoded data is available, it
 returns the end-of-file object. When the port is closed, @var{port} will
 also be closed, unless @var{keep-alive?} is true.
+
+If the chunked input ends prematurely, a
+@code{&chunked-input-ended-promaturely} exception will be raised.
 @end deffn
 
 @example
diff --git a/module/web/http.scm b/module/web/http.scm
index 163c52176..b3ea076ae 100644
--- a/module/web/http.scm
+++ b/module/web/http.scm
@@ -38,6 +38,7 @@
   #:use-module (ice-9 q)
   #:use-module (ice-9 binary-ports)
   #:use-module (ice-9 textual-ports)
+  #:use-module (ice-9 exceptions)
   #:use-module (rnrs bytevectors)
   #:use-module (web uri)
   #:export (string->header
@@ -67,6 +68,8 @@
             read-response-line
             write-response-line
 
+            &chunked-input-error-prematurely
+            chunked-input-ended-prematurely-error?
             make-chunked-input-port
             make-chunked-output-port
 
@@ -1935,6 +1938,17 @@ treated specially, and is just returned as a plain string."
 
 
 ;; Chunked Responses
+(define &chunked-input-ended-prematurely
+  (make-exception-type '&chunked-input-error-prematurely
+                       &external-error
+                       '()))
+
+(define make-chunked-input-ended-prematurely-error
+  (record-constructor &chunked-input-ended-prematurely))
+
+(define chunked-input-ended-prematurely-error?
+  (record-predicate &chunked-input-ended-prematurely))
+
 (define (read-chunk-header port)
   "Read a chunk header from PORT and return the size in bytes of the
 upcoming chunk."
@@ -1987,8 +2001,8 @@ closed it will also close PORT, unless the KEEP-ALIVE? is true."
                                                 ask-for)))
                (cond
                 ((eof-object? read)     ;premature termination
-                 (set! finished? #t)
-                 num-read)
+                 (raise-exception
+                  (make-chunked-input-ended-prematurely-error)))
                 (else
                  (let ((left (- remaining read)))
                    (set! remaining left)
diff --git a/test-suite/tests/web-http.test b/test-suite/tests/web-http.test
index 86fbc0e1a..796c44dd9 100644
--- a/test-suite/tests/web-http.test
+++ b/test-suite/tests/web-http.test
@@ -28,16 +28,19 @@
   #:use-module (test-suite lib))
 
 
-(define-syntax pass-if-named-exception
+(define-syntax pass-if-expected-exception
   (syntax-rules ()
-    ((_ name k pat exp)
+    ((_ name exception-predicate? exp)
      (pass-if name
-       (catch 'k
-         (lambda () exp (error "expected exception" 'k))
-         (lambda (k message args)
-           (if (string-match pat message)
-               #t
-               (error "unexpected exception" message args))))))))
+       (with-exception-handler
+           (lambda (exn)
+             (if (exception-predicate? exn)
+                 #t
+                 (error "unexpected exception" exn)))
+         (lambda ()
+           exp
+           #f)
+         #:unwind? #t)))))
 
 (define-syntax pass-if-only-parse
   (syntax-rules ()
@@ -486,6 +489,12 @@
            (port (make-chunked-input-port (open-input-string str))))
       (get-string-all port)))
 
+  (pass-if-expected-exception "Exception on premature chunk end"
+      chunked-input-ended-prematurely-error?
+    (let* ((str  "b\r\nFirst chunk\r\nc\r\nSecond chun")
+           (port (make-chunked-input-port (open-input-string str))))
+      (get-string-all port)))
+
   (pass-if-equal
       (call-with-output-string
        (lambda (out-raw)
-- 
2.36.1

Re: [PATCH 1/2] web: Handle ending CRLF (\r\n) for chunked input and output ports.

[Ludovic Courtès]

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> The chunked transfer encoding specifies the chunked body ends with
> CRLF. This is in addition to the CRLF at the end of the last chunk, so
> there should be CRLF twice at the end of the chunked body:
>
>   https://datatracker.ietf.org/doc/html/rfc2616#section-3.6.1
>
> * module/web/http.scm (make-chunked-input-port): Read two extra bytes at
> the end of the chunked input.
> (make-chunked-output-port): Write the missing \r\n when closing the
> port.
> * test-suite/tests/web-http.test (chunked encoding): Add missing \r\n to
> test data.

[...]

> This port is of limited use if it cannot be used reliably. Rather than
> behaving as if the input has finished when it ends unexpectedly, instead
> raise an exception.
>
> * module/web/http.scm (make-chunked-input-port): Raise an exception on
> premature termination.
> (&chunked-input-ended-prematurely): New exception type.
> (chunked-input-ended-prematurely-error?): New procedure.
> * test-suite/tests/web-http.test (pass-if-named-exception): Rename to
> pass-if-named-exception.
> (pass-if-named-exception): New syntax.
> ("Exception on premature chunk end"): New test for this behaviour.

Applied, thanks!

Are there servers out there where you would hit this bug?  We were
“lucky” not to notice before I guess.

Ludo’.

Re: [PATCH 1/2] web: Handle ending CRLF (\r\n) for chunked input and output ports.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]


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

> Christopher Baines <mail@cbaines.net> skribis:
>
>> The chunked transfer encoding specifies the chunked body ends with
>> CRLF. This is in addition to the CRLF at the end of the last chunk, so
>> there should be CRLF twice at the end of the chunked body:
>>
>>   https://datatracker.ietf.org/doc/html/rfc2616#section-3.6.1
>>
>> * module/web/http.scm (make-chunked-input-port): Read two extra bytes at
>> the end of the chunked input.
>> (make-chunked-output-port): Write the missing \r\n when closing the
>> port.
>> * test-suite/tests/web-http.test (chunked encoding): Add missing \r\n to
>> test data.
>
> [...]
>
>> This port is of limited use if it cannot be used reliably. Rather than
>> behaving as if the input has finished when it ends unexpectedly, instead
>> raise an exception.
>>
>> * module/web/http.scm (make-chunked-input-port): Raise an exception on
>> premature termination.
>> (&chunked-input-ended-prematurely): New exception type.
>> (chunked-input-ended-prematurely-error?): New procedure.
>> * test-suite/tests/web-http.test (pass-if-named-exception): Rename to
>> pass-if-named-exception.
>> (pass-if-named-exception): New syntax.
>> ("Exception on premature chunk end"): New test for this behaviour.
>
> Applied, thanks!

Awesome :)

> Are there servers out there where you would hit this bug?  We were
> “lucky” not to notice before I guess.

I'm unsure. I think I first hit this on the make-chunked-input-port side
with the Guix Build Coordinator. I can't remember the details, but I was
doing some testing with curl at least, and that behaved differently:

  https://git.cbaines.net/guix/build-coordinator/commit/?id=3ee79ba7b683275ef066f4d61b1ce50b64bd19ac

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]