Change branch for HTTP chunking bug?

Change branch for HTTP chunking bug?

[Magnus Henoch]

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

I recently committed the attached patch to trunk, fixing bug #42,
"Superfluous CR from HTTP chunked download":
http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=42

Should this be changed on the Emacs 22 branch as well?

Magnus


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: url-http.el.patch --]
[-- Type: text/x-patch, Size: 707 bytes --]

diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 7b29eba..c9cecea 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -948,7 +948,11 @@ the end of the document."
 		  (url-http-debug "Saw end of stream chunk!")
 		  (setq read-next-chunk nil)
 		  (url-display-percentage nil nil)
-		  (goto-char (match-end 1))
+		  ;; Every chunk, even the last 0-length one, is
+		  ;; terminated by CRLF.  Skip it.
+		  (when (looking-at "\r?\n")
+		    (url-http-debug "Removing terminator of last chunk")
+		    (delete-region (match-beginning 0) (match-end 0)))
 		  (if (re-search-forward "^\r*$" nil t)
 		      (url-http-debug "Saw end of trailers..."))
 		  (if (url-http-parse-headers)

Re: Change branch for HTTP chunking bug?

[Chong Yidong]

Magnus Henoch <mange@freemail.hu> writes:

> I recently committed the attached patch to trunk, fixing bug #42,
> "Superfluous CR from HTTP chunked download":
> http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=42
>
> Should this be changed on the Emacs 22 branch as well?
>
> Magnus
>
> diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
> index 7b29eba..c9cecea 100644
> --- a/lisp/url/url-http.el
> +++ b/lisp/url/url-http.el
> @@ -948,7 +948,11 @@ the end of the document."
>  		  (url-http-debug "Saw end of stream chunk!")
>  		  (setq read-next-chunk nil)
>  		  (url-display-percentage nil nil)
> -		  (goto-char (match-end 1))
> +		  ;; Every chunk, even the last 0-length one, is
> +		  ;; terminated by CRLF.  Skip it.
> +		  (when (looking-at "\r?\n")
> +		    (url-http-debug "Removing terminator of last chunk")
> +		    (delete-region (match-beginning 0) (match-end 0)))
>  		  (if (re-search-forward "^\r*$" nil t)
>  		      (url-http-debug "Saw end of trailers..."))
>  		  (if (url-http-parse-headers)

Looks OK.  Please check it into Emacs 22 (assuming it's been tested).

Re: Change branch for HTTP chunking bug?

[Stefan Monnier]

> I recently committed the attached patch to trunk, fixing bug #42,
> "Superfluous CR from HTTP chunked download":
> http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=42

> Should this be changed on the Emacs 22 branch as well?

It seems that the bug is also present on the 22 branch.  I'm not sure
how serious it is compared to the risk of introducing more bugs.
How confident are you that this is the right fix and that it will not
break anything currently working?


        Stefan


> diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
> index 7b29eba..c9cecea 100644
> --- a/lisp/url/url-http.el
> +++ b/lisp/url/url-http.el
> @@ -948,7 +948,11 @@ the end of the document."
>  		  (url-http-debug "Saw end of stream chunk!")
>  		  (setq read-next-chunk nil)
>  		  (url-display-percentage nil nil)
> -		  (goto-char (match-end 1))
> +		  ;; Every chunk, even the last 0-length one, is
> +		  ;; terminated by CRLF.  Skip it.
> +		  (when (looking-at "\r?\n")
> +		    (url-http-debug "Removing terminator of last chunk")
> +		    (delete-region (match-beginning 0) (match-end 0)))
>  		  (if (re-search-forward "^\r*$" nil t)
>  		      (url-http-debug "Saw end of trailers..."))
>  		  (if (url-http-parse-headers)

Re: Change branch for HTTP chunking bug?

[Magnus Henoch]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It seems that the bug is also present on the 22 branch.  I'm not sure
> how serious it is compared to the risk of introducing more bugs.
> How confident are you that this is the right fix

Quite confident; the previous code didn't do what the standard spells
out, but with my fix it does.  I have also tested it with downloads from
Emacswiki.

> and that it will not break anything currently working?

It eats whitespace at a point where it isn't significant, so nothing
should break.

[It would be nice if there were a test suite for the URL package.  It
seems to be that the easiest way to implement that would be if it were
possible to make pipes (as in the system call) that behave as pairs of
Lisp processes, so that the test suite can perform a fake conversation
with the client code...]

Magnus

Re: Change branch for HTTP chunking bug?

[Thien-Thi Nguyen]

() Magnus Henoch <mange@freemail.hu>
() Tue, 11 Mar 2008 13:19:39 +0100

   make pipes (as in the system call) that behave as pairs of Lisp
   processes, so that the test suite can perform a fake
   conversation with the client code...]

Check out server.el, and more fundamentally, make-network-process,
which accepts :server.

thi