bug#10768: 23.3; url-http misses data when last few bytes are in 2nd packet and content-length is used

[Christopher J. White]

Hi Andreas,

I have created a small set of files with lengths varying from
1200 to 1225, and from 2658 to 2662 and put them up on my personal web
server.  (e.g. http://www.grierwhite.com/url-http-1220.txt)

I chose lengths that just barely stretch from 1 to 2 payload packets,
and from 2 to 3 packets. 

1200-1207 -- 1 packets
1208-1225 -- 2 packets, 1-18 bytes in the 2nd
2658-2659 -- 2 packets
2660-2662 -- 3 packets, 1-3 bytes in the 3rd

Prior to the change, the results are as follows:

1200-1207 -- pass
1208-1217 -- fail
1217-1225 -- pass
2658-2662 -- pass

This indicates that the failure occurs when there are up to 10 bytes
in the 2nd packet.  This correlates with the fact that the header is 10
lines, thus will replace 10 \r characters and shorten the buffer by
that amount.  Consistent with the results I found with the original
server toodledo.com that only has 8 header lines and only has the issue
up to 8 bytes in the 2nd packet.

It is interesting to note that when there are 3 packets and just a few
bytes in the last packet, the failure does *not* occur.  I think this
may be explained by the fact that 
url-http-wait-for-headers-change-function may be called a second time,
thus all position references are recomputed.  Andreas, can you verify
this logic?  If this is correct, the problem only occurs for responses
that just border on 2 packets.

After the change, all lengths pass.  

Here is the lisp code that tests the lengths, feel free to run this as
you should be able to reproduce this from any machine that can access
the internet.  I don't have a trunk version of emacs (24), but looking
at the trunk copy of url-http.el, it appears to suffer from the same
problem.

(defun test-url-http-patch (minlen maxlen)
  (let ((passed 0) (failed 0))
    (do ((len minlen (+ 1 len))) ((> len maxlen) nil)
      (set-buffer 
       (url-retrieve-synchronously 
        (format "http://www.grierwhite.com/url-http-%d.txt"; len)))
      (goto-char (point-min))
      (re-search-forward "^\n")
      (let ((recvlen (- (point-max) (point))))
        (if (= len recvlen)
            (setq passed (1+ passed))
          (setq failed (1+ failed))
          (message "FAILED orig-len: %d, received len: %d" 
                   len recvlen))))
    (message "Passed: %d/%d" passed (+ passed failed))))

;; Run without the patch
(test-url-http-patch 1200 1225)
"Passed: 16/26"

(test-url-http-patch 2658 2662)
"Passed: 5/5"

;; Run with the patch
(test-url-http-patch 1200 1225)
"Passed: 26/26"

(test-url-http-patch 2658 2662)
"Passed: 5/5"

...cj

On Thu, 09 Feb 2012 16:43:27 +0100
Andreas Schwab <schwab@linux-m68k.org> wrote:

> Does this help?
> 
> diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
> index b43ed76..140824f 100644
> --- a/lisp/url/url-http.el
> +++ b/lisp/url/url-http.el
> @@ -352,11 +352,14 @@ request.")
>  ;; Parsing routines
>  (defun url-http-clean-headers ()
>    "Remove trailing \r from header lines.
> -This allows us to use `mail-fetch-field', etc."
> +This allows us to use `mail-fetch-field', etc.
> +Return the number of characters removed."
>    (declare (special url-http-end-of-headers))
> -  (goto-char (point-min))
> -  (while (re-search-forward "\r$" url-http-end-of-headers t)
> -    (replace-match "")))
> +  (let ((end (marker-position url-http-end-of-headers)))
> +    (goto-char (point-min))
> +    (while (re-search-forward "\r$" url-http-end-of-headers t)
> +      (replace-match ""))
> +    (- end url-http-end-of-headers)))
>  
>  (defun url-http-handle-authentication (proxy)
>    (declare (special status success url-http-method url-http-data
> @@ -1051,7 +1054,7 @@ the end of the document."
>         (setq url-http-end-of-headers (set-marker (make-marker)
>                                                   (point))
>               end-of-headers t)
> -       (url-http-clean-headers)))
> +       (setq nd (- nd (url-http-clean-headers))))
>  
>        (if (not end-of-headers)
>         ;; Haven't seen the end of the headers yet, need to wait
> 
> Andreas.
>