bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Michał Kondraciuk]

When I evaluate this, Emacs becomes unresponsive and keeps
consuming 100% of my cpu:

(progn
   (setq mode-line-format '(:propertize "a" face))
   (force-mode-line-update))

Repository revision: ee3e432300054ca488896e39fca57b10d733330a

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Jonathan Kyle Mitchell]

Michał Kondraciuk <k.michal@zoho.com> writes:

> When I evaluate this, Emacs becomes unresponsive and keeps
> consuming 100% of my cpu:
>
> (progn
>   (setq mode-line-format '(:propertize "a" face))
>   (force-mode-line-update))
>
> Repository revision: ee3e432300054ca488896e39fca57b10d733330a

I can also reproduce this issue on Emacs 26.1.

--
Jonathan Kyle Mitchell

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Jonathan Kyle Mitchell]

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


Jonathan Kyle Mitchell <kyle@jonathanmitchell.org> writes:

> Michał Kondraciuk <k.michal@zoho.com> writes:
>
>> When I evaluate this, Emacs becomes unresponsive and keeps
>> consuming 100% of my cpu:
>>
>> (progn
>>   (setq mode-line-format '(:propertize "a" face))
>>   (force-mode-line-update))
>>
>> Repository revision: ee3e432300054ca488896e39fca57b10d733330a
>
> I can also reproduce this issue on Emacs 26.1.

I compiled a debug build of Emacs 27.0.50 and took the attached
backtrace with a breakpoing on `error'.  Emacs stopped in
`validate_plist' as called from `set_text_properties' with the message
"Odd length text property list".


[-- Attachment #2: bug#32038 set_text_propery backtrace --]
[-- Type: text/plain, Size: 7081 bytes --]

Current directory is /home/jonathan/emacs/src/
GNU gdb (GDB) Fedora 8.1-18.fc28
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /home/jonathan/emacs/src/emacs...done.
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from terminal]
DISPLAY = :0
TERM = dumb
(gdb) break error
Breakpoint 3 at 0x5c0f36: file eval.c, line 1913.
(gdb) start
Temporary breakpoint 4 at 0x52f4b9: file emacs.c, line 679.
Starting program: /home/jonathan/emacs/src/emacs -Q
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
warning: Loadable section ".note.gnu.property" outside of ELF segments
[New Thread 0x7fffdc233700 (LWP 23217)]

Thread 1 "emacs" hit Temporary breakpoint 4, main (argc=2, argv=0x7fffffffe538) at emacs.c:679
679	{
(gdb) cont
Continuing.
[Switching to thread 2 (Thread 0x7fffdc233700 (LWP 23217))](running)
[New Thread 0x7fffcecb6700 (LWP 23218)]
warning: Loadable section ".note.gnu.property" outside of ELF segments
[New Thread 0x7fffce118700 (LWP 23219)]
[New Thread 0x7fffcd787700 (LWP 23220)]

Thread 1 "emacs" hit Breakpoint 3, error (m=m@entry=0x6a75a0 "Odd length text property list") at eval.c:1913
1913	{
(gdb) bt
#0  0x00000000005c0f36 in error (m=m@entry=0x6a75a0 "Odd length text property list") at eval.c:1913
#1  0x00000000006264aa in validate_plist (list=...) at textprop.c:212
#2  0x0000000000629bbc in set_text_properties (start=make_number(0), end=make_number(1), properties=..., object=..., coherent_change_p=..., coherent_change_p@entry=XIL(0xc750)) at textprop.c:1357
#3  0x0000000000629e6c in Fset_text_properties (start=..., start@entry=make_number(0), end=..., properties=..., properties@entry=XIL(0x13a3513), object=..., object@entry=XIL(0x3078304)) at lisp.h:942
#4  0x000000000045f626 in display_mode_element (it=it@entry=0x7fffffff8f70, depth=2, depth@entry=1, field_width=field_width@entry=0, precision=precision@entry=0, elt=XIL(0x3078304), props=props@entry=XIL(0x13a3513), risky=false) at lisp.h:1031
#5  0x000000000045fec8 in display_mode_element (it=it@entry=0x7fffffff8f70, depth=1, depth@entry=0, field_width=field_width@entry=0, precision=precision@entry=0, elt=..., elt@entry=XIL(0x13a36e3), props=..., props@entry=XIL(0), risky=false) at lisp.h:1302
#6  0x00000000004610b9 in display_mode_line (w=w@entry=0x1522c30 <bss_sbrk_buffer+8240528>, face_id=MODE_LINE_FACE_ID, format=format@entry=XIL(0x13a36e3)) at lisp.h:942
#7  0x00000000004614ea in display_mode_lines (w=w@entry=0x1522c30 <bss_sbrk_buffer+8240528>) at xdisp.c:23390
#8  0x000000000047c34d in redisplay_window (window=XIL(0x1522c35), just_this_one_p=just_this_one_p@entry=false) at xdisp.c:17559
#9  0x000000000047cf31 in redisplay_window_0 (window=..., window@entry=XIL(0x1522c35)) at xdisp.c:14939
#10 0x00000000005bf066 in internal_condition_case_1 (bfun=bfun@entry=0x47cf02 <redisplay_window_0>, arg=..., arg@entry=XIL(0x1522c35), handlers=..., hfun=hfun@entry=0x43973e <redisplay_window_error>) at eval.c:1373
#11 0x000000000044396b in redisplay_windows (window=XIL(0x1522c35)) at xdisp.c:14919
#12 0x000000000046b7fa in redisplay_internal () at xdisp.c:14402
#13 0x000000000046ce49 in redisplay () at xdisp.c:13612
#14 0x0000000000540290 in read_char (commandflag=1, map=..., map@entry=XIL(0x13a1d03), prev_event=XIL(0), used_mouse_menu=used_mouse_menu@entry=0x7fffffffe11b, end_time=end_time@entry=0x0) at keyboard.c:2451
#15 0x0000000000541ce6 in read_key_sequence (keybuf=keybuf@entry=0x7fffffffe1e0, prompt=XIL(0), dont_downcase_last=dont_downcase_last@entry=false, can_return_switch_frame=can_return_switch_frame@entry=true, fix_current_buffer=fix_current_buffer@entry=true, prevent_redisplay=prevent_redisplay@entry=false) at keyboard.c:9104
#16 0x00000000005433df in command_loop_1 () at lisp.h:942
#17 0x00000000005befc1 in internal_condition_case (bfun=bfun@entry=0x54317e <command_loop_1>, handlers=..., handlers@entry=XIL(0x54c0), hfun=hfun@entry=0x537bea <cmd_error>) at eval.c:1349
#18 0x0000000000535344 in command_loop_2 (ignore=..., ignore@entry=XIL(0)) at lisp.h:942
#19 0x00000000005bef09 in internal_catch (tag=..., func=func@entry=0x53532c <command_loop_2>, arg=..., arg@entry=XIL(0)) at eval.c:1114
#20 0x0000000000532c56 in command_loop () at lisp.h:942
#21 0x000000000053742f in recursive_edit_1 () at keyboard.c:703
#22 0x000000000053794b in Frecursive_edit () at keyboard.c:774
#23 0x00000000005302b5 in main (argc=<optimized out>, argv=0x7fffffffe538) at emacs.c:1720

Lisp Backtrace:
"redisplay_internal (C function)" (0x0)
(gdb) info reg
rax            0x0	0
rbx            0x0	0
rcx            0x3078304	50823940
rdx            0x13a3513	20591891
rsi            0x6	6
rdi            0x6a75a0	6976928
rbp            0x13a3513	0x13a3513 <bss_sbrk_buffer+6669939>
rsp            0x7fffffff8d88	0x7fffffff8d88
r8             0xc750	51024
r9             0x13a3513	20591891
r10            0x10fd	4349
r11            0x31558b8	51730616
r12            0x0	0
r13            0x6	6
r14            0x2	2
r15            0xc750	51024
rip            0x5c0f36	0x5c0f36 <error>
eflags         0x293	[ CF AF SF IF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0
(gdb) 

[-- Attachment #3: Type: text/plain, Size: 27 bytes --]


--
Jonathan Kyle Mitchell

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Jonathan Kyle Mitchell]

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

found 32038 26.1
tags 32038 + patch
quit

I think I found a way to make redisplay ignore any malformed property list by
putting a single check around Fset_text_properties in xdisp.c. The text of the
modeline is still set according to the provided string, but the property list
is ignored if it doesn't have an even number of elements. It doesn't
infinitely loop anymore given a malformed property list.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: verify plist before use patch --]
[-- Type: text/x-patch, Size: 1263 bytes --]

From 17eab01494e92e4f9582c56e73ae49278158b016 Mon Sep 17 00:00:00 2001
From: Jonathan Kyle Mitchell <kyle@jonathanmitchell.org>
Date: Tue, 3 Jul 2018 21:44:21 -0500
Subject: [PATCH] Verify plist is a plist before use

Do not blindly use a plist that came from Lisp to set text properties.  This
causes Fset_text_properties to go into an infinite loop during redisplay when
it catches the error in the global error handler (bug#32038).
* src/xdisp.c (display_mode_element):
---
 src/xdisp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 9b4febdd61..5f52a5ba6c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -23610,8 +23610,12 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 			= Fdelq (aelt, mode_line_proptrans_alist);
 
 		    elt = Fcopy_sequence (elt);
-		    Fset_text_properties (make_number (0), Flength (elt),
-					  props, elt);
+		    if (EQ (Fmod (Flength (props), make_number (2)),
+			    make_number (0)))
+		      {
+			Fset_text_properties (make_number (0), Flength (elt),
+					      props, elt);
+		      }
 		    /* Add this item to mode_line_proptrans_alist.  */
 		    mode_line_proptrans_alist
 		      = Fcons (Fcons (elt, props),
-- 
2.17.1


[-- Attachment #3: Type: text/plain, Size: 27 bytes --]


--
Jonathan Kyle Mitchell

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Eli Zaretskii]

> From: Jonathan Kyle Mitchell <kyle@jonathanmitchell.org>
> Date: Tue, 03 Jul 2018 22:12:57 -0500
> Cc: 32038@debbugs.gnu.org
> 
> I think I found a way to make redisplay ignore any malformed property list by
> putting a single check around Fset_text_properties in xdisp.c. The text of the
> modeline is still set according to the provided string, but the property list
> is ignored if it doesn't have an even number of elements. It doesn't
> infinitely loop anymore given a malformed property list.

Thanks, but I think we should log the error in *Messages*, because
otherwise the error will go unnoticed.

> +		    if (EQ (Fmod (Flength (props), make_number (2)),
> +			    make_number (0)))

We are on the C level, so it is easier/simpler to do this instead:

  ptrdiff_t seqlen = XFASTINT (Flength (props));
  if (seqlen % 2 == 0)
    Fset_text_properties (...);

More importantly, Flength can signal an error if PROPS is too long, so
I'm not sure the idea of your patch is 100% correct, because the code
you propose can still signal an error.  An alternative would be to
call Fset_text_properties via internal_condition_case_n, like we do in
safe__call.

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Jonathan Kyle Mitchell]

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

On Wed, 2018-07-04 at 18:07 +0300, Eli Zaretskii wrote:
> > From: Jonathan Kyle Mitchell <kyle@jonathanmitchell.org>
> > Date: Tue, 03 Jul 2018 22:12:57 -0500
> > Cc: 32038@debbugs.gnu.org
> > 
> > I think I found a way to make redisplay ignore any malformed
> > property list by
> > putting a single check around Fset_text_properties in xdisp.c. The
> > text of the
> > modeline is still set according to the provided string, but the
> > property list
> > is ignored if it doesn't have an even number of elements. It
> > doesn't
> > infinitely loop anymore given a malformed property list.
> 
> Thanks, but I think we should log the error in *Messages*, because
> otherwise the error will go unnoticed.
> 
> > +		    if (EQ (Fmod (Flength (props), make_number
> > (2)),
> > +			    make_number (0)))
> 
> We are on the C level, so it is easier/simpler to do this instead:
> 
>   ptrdiff_t seqlen = XFASTINT (Flength (props));
>   if (seqlen % 2 == 0)
>     Fset_text_properties (...);
> 
> More importantly, Flength can signal an error if PROPS is too long,
> so
> I'm not sure the idea of your patch is 100% correct, because the code
> you propose can still signal an error.  An alternative would be to
> call Fset_text_properties via internal_condition_case_n, like we do
> in
> safe__call.

That makes sense. I added one function to call Fset_text_properties
through internal_condition_case_n in the attached revised patch. The
error gets caught and safe_eval_handler appends an error message to the
*Messages* buffer.

The error message is put in *Messages* on the first time only though,
subsequent (force-mode-line-update) calls just append nil. I don't know
if that's expected for redisplay's internal messaging routines.

Thanks for reviewing the patch.

--
Jonathan Kyle Mitchell

[-- Attachment #2: 0001-Call-Fset_text_properties-through-internal_condition.patch --]
[-- Type: text/x-patch, Size: 1706 bytes --]

From 621bc3df19eaf2258c9a2ec0c72004ea8336ce0f Mon Sep 17 00:00:00 2001
From: Jonathan Kyle Mitchell <kyle@jonathanmitchell.org>
Date: Wed, 4 Jul 2018 22:38:29 -0500
Subject: [PATCH] Call Fset_text_properties through internal_condition_case_n

* xdisp.c (display_mode_element, safe_set_text_properties):
---
 src/xdisp.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 9b4febdd61..fb8fc905b9 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -23516,6 +23516,17 @@ move_elt_to_front (Lisp_Object elt, Lisp_Object list)
   return list;
 }
 
+/* Subroutine to call Fset_text_properties through
+   internal_condition_case_n.  ARGS are the arguments of
+   Fset_text_properties, in order. */
+
+static Lisp_Object
+safe_set_text_properties (ptrdiff_t nargs, Lisp_Object *args)
+{
+  eassert (nargs == 4);
+  return Fset_text_properties (args[0], args[1], args[2], args[3]);
+}
+
 /* Contribute ELT to the mode line for window IT->w.  How it
    translates into text depends on its data type.
 
@@ -23610,8 +23621,14 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 			= Fdelq (aelt, mode_line_proptrans_alist);
 
 		    elt = Fcopy_sequence (elt);
-		    Fset_text_properties (make_number (0), Flength (elt),
-					  props, elt);
+		    internal_condition_case_n (safe_set_text_properties,
+					       4,
+					       ((Lisp_Object [])
+					       {make_number (0),
+						   Flength (elt),
+						   props,
+						   elt}),
+					       Qt, safe_eval_handler);
 		    /* Add this item to mode_line_proptrans_alist.  */
 		    mode_line_proptrans_alist
 		      = Fcons (Fcons (elt, props),
-- 
2.17.1

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Eli Zaretskii]

> From: Jonathan Kyle Mitchell <kyle@jonathanmitchell.org>
> Cc: k.michal@zoho.com, 32038@debbugs.gnu.org
> Date: Wed, 04 Jul 2018 23:14:01 -0500
> 
> That makes sense. I added one function to call Fset_text_properties
> through internal_condition_case_n in the attached revised patch. The
> error gets caught and safe_eval_handler appends an error message to the
> *Messages* buffer.
> 
> The error message is put in *Messages* on the first time only though,
> subsequent (force-mode-line-update) calls just append nil. I don't know
> if that's expected for redisplay's internal messaging routines.
> 
> Thanks for reviewing the patch.

Thanks, I pushed this to the master branch, and I'm marking the bug
done.

> From 621bc3df19eaf2258c9a2ec0c72004ea8336ce0f Mon Sep 17 00:00:00 2001
> From: Jonathan Kyle Mitchell <kyle@jonathanmitchell.org>
> Date: Wed, 4 Jul 2018 22:38:29 -0500
> Subject: [PATCH] Call Fset_text_properties through internal_condition_case_n
> 
> * xdisp.c (display_mode_element, safe_set_text_properties):

The header line and the log message are sub-optimal/incomplete, and
don't mention the bug number.  Please see how I fixed that in the
actual commit, and try following this example in the future.

bug#32038: 27.0.50; Emacs hangs when using :propertize mode line construct and not providing a property value

[Jonathan Kyle Mitchell]

On Sat, 2018-07-14 at 14:30 +0300, Eli Zaretskii wrote:
> The header line and the log message are sub-optimal/incomplete, and
> don't mention the bug number.  Please see how I fixed that in the
> actual commit, and try following this example in the future.

Noted, and thanks for reviewing and pushing the patch.
 
--
Jonathan Kyle Mitchell

bug#32237: 27.0.50; Function in before-change-functions is called with first argument greater than the second

[Michał Kondraciuk]

If I eveluate the sexp below with M-: in emacs -Q, I get the message
"before change 4 1", which is unexpected.  The second argument should be
greater than the first one, according to the documentation.

(with-current-buffer "*scratch*"
   (erase-buffer)
   (insert "foo")
   (add-hook 'before-change-functions
             (lambda (beg end)
               (message "before change %d %d" beg end))
             nil t)
   (with-temp-buffer
     (insert "foo")
     (let ((temp (current-buffer)))
       (with-current-buffer "*scratch*"
         (replace-buffer-contents temp)))))


Repository revision: 2c242cb1a2956ecfa894d0110b837d4ecc43a69c

bug#32237: 27.0.50; Function in before-change-functions is called with first argument greater than the second

[Eli Zaretskii]

> From: Michał Kondraciuk <k.michal@zoho.com>
> Date: Sat, 21 Jul 2018 18:39:26 +0200
> 
> If I eveluate the sexp below with M-: in emacs -Q, I get the message
> "before change 4 1", which is unexpected.  The second argument should be
> greater than the first one, according to the documentation.

Thanks, fixed on the emacs-26 branch.