From a11454602f7c4af3af5729b9e504130c660319f3 Mon Sep 17 00:00:00 2001 From: Nacho Barrientos Date: Sat, 16 Apr 2022 18:00:39 +0200 Subject: [PATCH] url-http.el: chunked response: wait for the last CRLF. As per [0], the last chunk of 0 bytes is always accompanied by a last CRLF that signals the end of the message: chunked-body = *chunk last-chunk trailer-part CRLF ^ this one chunk = chunk-size [ chunk-ext ] CRLF chunk-data CRLF chunk-size = 1*HEXDIG last-chunk = 1*("0") [ chunk-ext ] CRLF chunk-data = 1*OCTET ; a sequence of chunk-size octets `url-http-chunked-encoding-after-change-function' is able to process (and remove) that terminator IF AVAILABLE in the buffer when processing the response, however it won't wait for it if it's not yet there. In other words: | Bottom of the response buffer | Bottom of the full response | | (visible to url-http) | (to be delivered to Emacs) | | ------------------------------+-----------------------------| | 0\r\n | 0\r\n | | | \r\n | If the last chunk is processed when the bottom of the response buffer is as above (note that the whole response has not yet been delivered to Emacs), url-http will call the user callback without waiting for the final terminator to be read from the socket. This is normally not an issue when doing one-shot requests, but it's problematic when the connection is reused immediately. As there are 2 bytes from the request N that have not been dealt with, they'll be considered as part of the response of the request N+1. On top, it turns out that when processing the headers of request N+1, `url-http-wait-for-headers-change-function' will consider the request a "headerless malformed response" delivering it broken to the caller. The proposed fix implements a state in which `url-http-chunked-encoding-after-change-function` properly waits for the very last element of the message preventing the problem explained above from happening. For additional context, this bug was found when debugging magit/ghub (see [1] for details). [0] https://datatracker.ietf.org/doc/html/rfc7230#section-4.1 [1] https://github.com/magit/ghub/issues/81 --- lisp/url/url-http.el | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el index 16c3a6a1e6..dfdc03cc6c 100644 --- a/lisp/url/url-http.el +++ b/lisp/url/url-http.el @@ -36,6 +36,7 @@ (defvar url-current-object) (defvar url-http-after-change-function) (defvar url-http-chunked-counter) +(defvar url-http-chunked-last-crlf-missing nil) (defvar url-http-chunked-length) (defvar url-http-chunked-start) (defvar url-http-connection-opened) @@ -1068,7 +1069,16 @@ the callback to be triggered." Cannot give a sophisticated percentage, but we need a different function to look for the special 0-length chunk that signifies the end of the document." - (save-excursion + (if url-http-chunked-last-crlf-missing + (progn + (goto-char url-http-chunked-last-crlf-missing) + (if (not (looking-at "\r\n")) + (url-http-debug "Still spinning for the terminator of last chunk...") + (url-http-debug "Saw the last CRLF.") + (delete-region (match-beginning 0) (match-end 0)) + (if (url-http-parse-headers) + (url-http-activate-callback)))) + (save-excursion (goto-char st) (let ((read-next-chunk t) (case-fold-search t) @@ -1145,13 +1155,16 @@ the end of the document." (url-display-percentage nil nil) ;; Every chunk, even the last 0-length one, is ;; terminated by CRLF. Skip it. - (when (looking-at "\r?\n") + (if (not (looking-at "\r?\n")) + (progn + (url-http-debug "Spinning for the terminator of last chunk...") + (setq-local url-http-chunked-last-crlf-missing (point))) (url-http-debug "Removing terminator of last chunk") - (delete-region (match-beginning 0) (match-end 0))) - (if (re-search-forward "^\r?\n" nil t) - (url-http-debug "Saw end of trailers...")) - (if (url-http-parse-headers) - (url-http-activate-callback)))))))))) + (delete-region (match-beginning 0) (match-end 0)) + (if (re-search-forward "^\r?\n" nil t) + (url-http-debug "Saw end of trailers...")) + (if (url-http-parse-headers) + (url-http-activate-callback)))))))))))) (defun url-http-wait-for-headers-change-function (_st nd _length) ;; This will wait for the headers to arrive and then splice in the -- 2.35.3