bug#20498: 25.0.50; PATCH: break potential infinite loop in (line-move-to-column)

bug#20498: 25.0.50; PATCH: break potential infinite loop in (line-move-to-column)

[Dima Kogan]

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

(line-move-to-column) has a loop that can become infinite:

  (while (and ...)
    (goto-char (previous-char-property-change (point) line-beg)))

If (= (point) line-beg) then the (goto-char) does nothing, and the
condition in the while never changes. This patch adds a check to break
out of the while when this happens:

  (while (and ... (/= (point) line-beg))
    (goto-char (previous-char-property-change (point) line-beg)))

I'm seeing this in the wild with ERC and erc-fill-mode disabled. Simply
moving around an ERC buffer can hit this.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-simple.el-line-move-to-column-break-potential-i.patch --]
[-- Type: text/x-diff, Size: 1456 bytes --]

From 4214ce56af49f506729b0240ea1dbb9c588f6215 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Sun, 3 May 2015 12:31:13 -0700
Subject: [PATCH] * lisp/simple.el (line-move-to-column): break potential
 infinite loop

(line-move-to-column) has a loop that can become infinite:

  (while (and ...)
    (goto-char (previous-char-property-change (point) line-beg)))

If (= (point) line-beg) then the (goto-char) does nothing, and the condition in
the while never changes. This patch adds a check to break out of the while when this happens:

  (while (and ... (/= (point) line-beg))
    (goto-char (previous-char-property-change (point) line-beg)))

I'm seeing this in the wild with ERC and erc-fill-mode disabled. Simply moving
around an ERC buffer can hit this.
---
 lisp/simple.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 31efe38..4873ebd 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5962,7 +5962,8 @@ and `current-column' to be able to ignore invisible text."
 	;; but with a more reasonable buffer position.
 	(goto-char normal-location)
 	(let ((line-beg (line-beginning-position)))
-	  (while (and (not (bolp)) (invisible-p (1- (point))))
+	  (while (and (not (bolp)) (invisible-p (1- (point)))
+                      (/= (point) line-beg))
 	    (goto-char (previous-char-property-change (point) line-beg))))))))
 
 (defun move-end-of-line (arg)
-- 
2.1.4

bug#20498: 25.0.50; PATCH: break potential infinite loop in (line-move-to-column)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Date: Sun, 03 May 2015 12:32:33 -0700
> 
> (line-move-to-column) has a loop that can become infinite:
> 
>   (while (and ...)
>     (goto-char (previous-char-property-change (point) line-beg)))
> 
> If (= (point) line-beg) then the (goto-char) does nothing, and the
> condition in the while never changes.

The full fragment is this:

	(let ((line-beg (line-beginning-position)))
	  (while (and (not (bolp)) (invisible-p (1- (point))))
	    (goto-char (previous-char-property-change (point) line-beg))))))))

So if (= (point) line-beg), then 'bolp' will return t, and we will
break from the loop.  Am I missing something?

> I'm seeing this in the wild with ERC and erc-fill-mode disabled.

Can you dig deeper into the problem, and tell the details about this
infloop?

bug#20498: 25.0.50; PATCH: break potential infinite loop in (line-move-to-column)

[Dima Kogan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dima Kogan <dima@secretsauce.net>
>> Date: Sun, 03 May 2015 12:32:33 -0700
>> 
>> (line-move-to-column) has a loop that can become infinite:
>> 
>>   (while (and ...)
>>     (goto-char (previous-char-property-change (point) line-beg)))
>> 
>> If (= (point) line-beg) then the (goto-char) does nothing, and the
>> condition in the while never changes.
>
> The full fragment is this:
>
> 	(let ((line-beg (line-beginning-position)))
> 	  (while (and (not (bolp)) (invisible-p (1- (point))))
> 	    (goto-char (previous-char-property-change (point) line-beg))))))))
>
> So if (= (point) line-beg), then 'bolp' will return t, and we will
> break from the loop.  Am I missing something?
>
>> I'm seeing this in the wild with ERC and erc-fill-mode disabled.
>
> Can you dig deeper into the problem, and tell the details about this
> infloop?

Hi. Thank you very much for double-checking. It indeed looks like (bolp)
should cover this case. It doesn't however. When ERC misbehaves in this
way I observe both

 (= (line-beginning-position) (point))  --> t
 (bolp)                                 --> nil

This sounds wrong. Looking at the buffer with my eyes, the point is not
at the beginning of the line, so it LOOKS like the (bolp) result is
correct.

The docs for (line-beginning-position) state that this function respects
field boundaries. If I ignore those explicitly then I see the correct
behavior:

 (let ((inhibit-field-text-motion t))
       (line-beginning-position))   ---> correct start of line

So the correct patch would replace (line-beginning-position) to the
above expression. This is trivial, and I'm not actually attaching a
patch. If you want me to, tell me.

(line-beginning-position) is used in quite a few places in emacs, and
I'm wondering if in many of those uses the intent is to ignore fields.

bug#20498: 25.0.50; PATCH: break potential infinite loop in (line-move-to-column)

[Eli Zaretskii]

> From: Dima Kogan <dima@secretsauce.net>
> Cc: 20498@debbugs.gnu.org
> Date: Tue, 05 May 2015 01:29:14 -0700
> 
> Hi. Thank you very much for double-checking. It indeed looks like (bolp)
> should cover this case. It doesn't however. When ERC misbehaves in this
> way I observe both
> 
>  (= (line-beginning-position) (point))  --> t
>  (bolp)                                 --> nil
> 
> This sounds wrong. Looking at the buffer with my eyes, the point is not
> at the beginning of the line, so it LOOKS like the (bolp) result is
> correct.
> 
> The docs for (line-beginning-position) state that this function respects
> field boundaries. If I ignore those explicitly then I see the correct
> behavior:
> 
>  (let ((inhibit-field-text-motion t))
>        (line-beginning-position))   ---> correct start of line
> 
> So the correct patch would replace (line-beginning-position) to the
> above expression. This is trivial, and I'm not actually attaching a
> patch. If you want me to, tell me.

Thanks, I pushed such a change.

> (line-beginning-position) is used in quite a few places in emacs, and
> I'm wondering if in many of those uses the intent is to ignore fields.

I don't know, but I guess they will discover this in due time.