Make ietf-drums-remove-comments() robust against broken header

Make ietf-drums-remove-comments() robust against broken header

[TSUCHIYA Masatoshi]

Hi,

I have just met a message including a broken references header, like:

  References: <200810150047.AA17342@mailhost.example.net>
        <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
        <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
        (=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= "'s message of "Fri, 17 Oct 2008 04:32:17 +0900")

It forces ietf-drums-remove-comments() to say:

  Debugger entered--Lisp error: (scan-error "Unbalanced parentheses" 157 258)
    scan-sexps(157 1)
    forward-sexp(1)

In order to avoid this error, I'd like to install the attached change.
Do you have any objections or better solutions?

Regards,

-- 
TSUCHIYA Masatoshi
--- ietf-drums.el	19 May 2008 08:47:42 -0000	7.16
+++ ietf-drums.el	30 Oct 2008 07:11:01 -0000
@@ -125,7 +125,10 @@
 	 ((eq c ?\")
 	  (forward-sexp 1))
 	 ((eq c ?\()
-	  (delete-region (point) (progn (forward-sexp 1) (point))))
+	  (delete-region (point)
+			 (condition-case nil
+			     (progn (forward-sexp 1) (point))
+			   (error (point-max)))))
 	 (t
 	  (forward-char 1))))
       (buffer-string))))

Re: Make ietf-drums-remove-comments() robust against broken header

[Katsumi Yamaoka]

>>>>> TSUCHIYA Masatoshi wrote:

> I have just met a message including a broken references header, like:

>   References: <200810150047.AA17342@mailhost.example.net>
>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>         (=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= "'s message of "Fri, 17 Oct 2008 04:32:17 +0900")

> It forces ietf-drums-remove-comments() to say:

>   Debugger entered--Lisp error: (scan-error "Unbalanced parentheses" 157 258)
>     scan-sexps(157 1)
>     forward-sexp(1)

> In order to avoid this error, I'd like to install the attached change.
> Do you have any objections or better solutions?

Although I don't know whether such messages often appear, I verified
a message having that header cannot be seen at all without the patch.
I think a Lisp function that uses `forward-sexp' to look for the close
parenthesis should have been made so as not to stop at any rate except
for the case where it is used only when the close parenthesis surely
exists there.  So your patch seems to be the right thing.

Regards,

Re: Make ietf-drums-remove-comments() robust against broken header

[Bjørn Mork]

TSUCHIYA Masatoshi <tsuchiya@namazu.org> writes:

> I have just met a message including a broken references header, like:
>
>   References: <200810150047.AA17342@mailhost.example.net>
>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>         (=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= "'s message of "Fri, 17 Oct 2008 04:32:17 +0900")

I fail to see how that's broken.  Care to explain?


Bjørn
-- 
No nukes

Re: Make ietf-drums-remove-comments() robust against broken header

[Katsumi Yamaoka]

>>>>> Bjørn Mork wrote:
> TSUCHIYA Masatoshi <tsuchiya@namazu.org> writes:

>> I have just met a message including a broken references header, like:
>>
>>   References: <200810150047.AA17342@mailhost.example.net>
>>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>>         (=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= "'s message of "Fri, 17 Oct 2008 04:32:17 +0900")

> I fail to see how that's broken.  Care to explain?

In the condition where `ietf-drums-syntax-table' is used,
the parentheses in the last line are unbalanced.  For instance:

(progn
  (pop-to-buffer "*testing*")
  (erase-buffer)
  (insert "(=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= \"'s message of \"Fri, 17 Oct 2008 04:32:17 +0900\")")
  (require 'ietf-drums)
  (set-syntax-table ietf-drums-syntax-table)
  (goto-char (point-min))
  (forward-sexp 1))

Regards,

Re: Make ietf-drums-remove-comments() robust against broken header

[Bjørn Mork]

Katsumi Yamaoka <yamaoka@jpl.org> writes:
>>>>>> Bjørn Mork wrote:
>> TSUCHIYA Masatoshi <tsuchiya@namazu.org> writes:
>
>>> I have just met a message including a broken references header, like:
>>>
>>>   References: <200810150047.AA17342@mailhost.example.net>
>>>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>>>         <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net>
>>>         (=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= "'s message of "Fri, 17 Oct 2008 04:32:17 +0900")
>
>> I fail to see how that's broken.  Care to explain?
>
> In the condition where `ietf-drums-syntax-table' is used,
> the parentheses in the last line are unbalanced.  For instance:
>
> (progn
>   (pop-to-buffer "*testing*")
>   (erase-buffer)
>   (insert "(=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= \"'s message of \"Fri, 17 Oct 2008 04:32:17 +0900\")")
>   (require 'ietf-drums)
>   (set-syntax-table ietf-drums-syntax-table)
>   (goto-char (point-min))
>   (forward-sexp 1))

Yes, I can see that the parsing fails, but there's nothing wrong with
the comment, is there?  Any number of "'s are allowed, including odd
numbers.

And won't the suggested patch kill something like this?:

   References: (") <200810150047.AA17342@mailhost.example.net>

which AFAICS is allowed.  



Bjørn
-- 
Ban the war on drugs

Re: Make ietf-drums-remove-comments() robust against broken header

[TSUCHIYA Masatoshi]

>> On  Thu, 30 Oct 2008 15:27:13 +0100
>> bmork@dod.no ( Bjørn Mork) said as follows:

>And won't the suggested patch kill something like this?:

>   References: (") <200810150047.AA17342@mailhost.example.net>

>which AFAICS is allowed.  

Really?  Please try the following expression.

(progn
  (defun ietf-drums-remove-comments (string)
    "Remove comments from STRING."
    (with-temp-buffer
      (let (c)
	(ietf-drums-init string)
	(while (not (eobp))
	  (setq c (char-after))
	  (cond
	   ((eq c ?\")
	    (forward-sexp 1))
	   ((eq c ?\()
	    (delete-region (point)
			   (condition-case nil
			       (progn (forward-sexp 1) (point))
			     (error (point-max)))))
	   (t
	    (forward-char 1))))
	(buffer-string))))
  (ietf-drums-remove-comments "<200810150047.AA17342@mailhost.example.net> <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net> <7B77911B-167E-47BF-A826-1C4396FEF000@foo.example.net> (=?iso-2022-jp?B?IhskQj0pTVUbKEIgGyRCTSdOSRsoQg==?= \"'s message of \"Fri, 17 Oct 2008 04:32:17 +0900\")"))

-- 
TSUCHIYA Masatoshi

Re: Make ietf-drums-remove-comments() robust against broken header

[Katsumi Yamaoka]

>>>>> Bjørn Mork wrote:
> And won't the suggested patch kill something like this?:

>    References: (") <200810150047.AA17342@mailhost.example.net>

> which AFAICS is allowed.

The patch makes the function remove not only the odd comment but
also things following it.  You meant it has to behave like the
following, didn't you?

(ietf-drums-remove-comments "(\") <200810150047.AA17342@mailhost.example.net>")
 => " <200810150047.AA17342@mailhost.example.net>"

Ok.  It is below.  Anyway I believe we need to fix the function
whatever the way, or we cannot view messages having such odd
comments.

--8<---------------cut here---------------start------------->8---
(defun ietf-drums-remove-comments (string)
  "Remove comments from STRING."
  (with-temp-buffer
    (let (c)
      (ietf-drums-init string)
      (while (not (eobp))
	(setq c (char-after))
	(cond
	 ((eq c ?\")
	  (forward-sexp 1))
	 ((eq c ?\()
	  (delete-region
	   (point)
	   (or (ignore-errors
		(forward-sexp 1)
		(point))
	       (ignore-errors
		(with-syntax-table (copy-syntax-table ietf-drums-syntax-table)
		  (modify-syntax-entry ?\" "w")
		  (forward-sexp 1)
		  (point)))
	       (point-max))))
	 (t
	  (forward-char 1))))
      (buffer-string))))
--8<---------------cut here---------------end--------------->8---

TSUCHIYA-san, WDYT?

Regards,

Re: Make ietf-drums-remove-comments() robust against broken header

[TSUCHIYA Masatoshi]

>> On Fri, 31 Oct 2008 08:33:52 +0900
>> yamaoka@jpl.org (Katsumi Yamaoka) said as follows:

>>    References: (") <200810150047.AA17342@mailhost.example.net>
>> which AFAICS is allowed.

>The patch makes the function remove not only the odd comment but
>also things following it.  You meant it has to behave like the
>following, didn't you?

>(ietf-drums-remove-comments "(\") <200810150047.AA17342@mailhost.example.net>")
> => " <200810150047.AA17342@mailhost.example.net>"

>Ok.  It is below.  Anyway I believe we need to fix the function
>whatever the way, or we cannot view messages having such odd comments.

>TSUCHIYA-san, WDYT?

I checked http://www.ietf.org/rfc/rfc2822.txt, and noticed that
double-quote is always allowed as texts of comments.

FWS             =       ([*WSP CRLF] 1*WSP) /   ; Folding white space
                        obs-FWS

ctext           =       NO-WS-CTL /     ; Non white space controls
                        %d33-39 /       ; The rest of the US-ASCII
                        %d42-91 /       ;  characters not including "(",
                        %d93-126        ;  ")", or "\"

ccontent        =       ctext / quoted-pair / comment

comment         =       "(" *([FWS] ccontent) [FWS] ")"

CFWS            =       *([FWS] comment) (([FWS] comment) / FWS)

Therefore, I think that your solution can be simplified as follows.

(defun ietf-drums-remove-comments (string)
  "Remove comments from STRING."
  (with-temp-buffer
    (let (c)
      (ietf-drums-init string)
      (while (not (eobp))
	(setq c (char-after))
	(cond
	 ((eq c ?\")
	  (forward-sexp 1))
	 ((eq c ?\()
	  (delete-region
	   (point)
	   (condition-case nil
	       (with-syntax-table (copy-syntax-table ietf-drums-syntax-table)
		 (modify-syntax-entry ?\" "w")
		 (forward-sexp 1)
		 (point))
	     (error (point-max)))))
	 (t
	  (forward-char 1))))
      (buffer-string))))

-- 
TSUCHIYA Masatoshi

Re: Make ietf-drums-remove-comments() robust against broken header

[Katsumi Yamaoka]

>>>>> TSUCHIYA Masatoshi wrote:
>>TSUCHIYA-san, WDYT?

> I checked http://www.ietf.org/rfc/rfc2822.txt, and noticed that
> double-quote is always allowed as texts of comments.

[...]

> Therefore, I think that your solution can be simplified as follows.

Looks reasonable.  Could you install it?

Regards,

Re: Make ietf-drums-remove-comments() robust against broken header

[Ted Zlatanov]

On Fri, 31 Oct 2008 10:21:20 +0900 Katsumi Yamaoka <yamaoka@jpl.org> wrote: 

>>>>>> TSUCHIYA Masatoshi wrote:
>>> TSUCHIYA-san, WDYT?

>> I checked http://www.ietf.org/rfc/rfc2822.txt, and noticed that
>> double-quote is always allowed as texts of comments.

KY> [...]

>> Therefore, I think that your solution can be simplified as follows.

KY> Looks reasonable.  Could you install it?

Because this was breaking my split methods and thus my entire Gnus
startup (because I use gnus-registry-split-fancy-with-parent), I took
the liberty of installing the change.  In addition, I put in an extra
condition-case to prevent any other such problems, assuming that at
worst we'd have bad references due to parsing bugs.

If this is not OK, please put in whatever changes you think are
appropriate, as long as they don't bring the bug back :)

Thanks
Ted

Re: Make ietf-drums-remove-comments() robust against broken header

[Katsumi Yamaoka]

>>>>> Ted Zlatanov wrote:
> Because this was breaking my split methods and thus my entire Gnus
> startup (because I use gnus-registry-split-fancy-with-parent), I took
> the liberty of installing the change.  In addition, I put in an extra
> condition-case to prevent any other such problems, assuming that at
> worst we'd have bad references due to parsing bugs.

> If this is not OK, please put in whatever changes you think are
> appropriate, as long as they don't bring the bug back :)

If the purpose of the extra `condition-case' you added is for
another `forward-sexp', isn't it better to be the following for
making the code easy to see?

--8<---------------cut here---------------start------------->8---
(defun ietf-drums-remove-comments (string)
  "Remove comments from STRING."
  (with-temp-buffer
    (let (c)
      (ietf-drums-init string)
      (while (not (eobp))
	(setq c (char-after))
	(cond
	 ((eq c ?\")
	  (condition-case nil
	      (forward-sexp 1)
	    (goto-char (point-max))))
	 ((eq c ?\()
	  (delete-region
	   (point)
	   (condition-case nil
	       (with-syntax-table (copy-syntax-table ietf-drums-syntax-table)
		 (modify-syntax-entry ?\" "w")
		 (forward-sexp 1)
		 (point))
	     (error (point-max)))))
	 (t
	  (forward-char 1))))
      (buffer-string))))
--8<---------------cut here---------------end--------------->8---

BTW, I tried the benchmark and verified that TSUCHIYA-san's change
does not slow Gnus.

Regards,

Re: Make ietf-drums-remove-comments() robust against broken header

[Katsumi Yamaoka]

>>>>> Katsumi Yamaoka wrote:
> If the purpose of the extra `condition-case' you added is for
> another `forward-sexp', isn't it better to be the following for
> making the code easy to see?
[...]
>         (cond
>          ((eq c ?\")
>           (condition-case nil
>               (forward-sexp 1)
>             (goto-char (point-max))))

Sorry, it should be:

	(cond
	 ((eq c ?\")
	  (condition-case nil
	      (forward-sexp 1)
	    (error (goto-char (point-max)))))

Re: Make ietf-drums-remove-comments() robust against broken header

[Ted Zlatanov]

On Fri, 31 Oct 2008 18:06:49 +0900 Katsumi Yamaoka <yamaoka@jpl.org> wrote: 

>>>>>> Katsumi Yamaoka wrote:
>> If the purpose of the extra `condition-case' you added is for
>> another `forward-sexp', isn't it better to be the following for
>> making the code easy to see?
KY> [...]
>> (cond
>> ((eq c ?\")
>> (condition-case nil
>> (forward-sexp 1)
>> (goto-char (point-max))))

KY> Sorry, it should be:

KY> 	(cond
KY> 	 ((eq c ?\")
KY> 	  (condition-case nil
KY> 	      (forward-sexp 1)
KY> 	    (error (goto-char (point-max)))))

That's fine too.  I made the change and tested it with a few headers.

Ted