bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Jan Chaloupka]

In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of current_kboard is Qnil for the first invocation.
If NILP (append) is false, current_kboard->kbd_macro_ptr has random value (in our case 0x5353535353535353),
which after CHECK_VECTOR_OR_STRING failure (invocation of wrong_type_argument) results in garbage collecting.
During gc, marking of objects is processed and mark_kboards (keyboard.c) is invoked. Following for loop is fired:

for (p = kb->kbd_macro_buffer; p < kb->kbd_macro_ptr; p++)
                   mark_object (*p);

Since kb->kbd_macro_ptr is set to 0x5353535353535353, mark_object (*p) is trying to mark object on address
out of memory space (or memory that cannot be accessed). Thus resulting in SIGSEGV signal.

Solution is to check for Qnil before calling CHECK_VECTOR_OR_STRING and set len to 0 if Qnil occurs.

https://bugzilla.redhat.com/show_bug.cgi?id=1104012

Signed-off-by: Jan Chaloupka <jchaloup@redhat.com>
---
  src/macros.c |   18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/macros.c b/src/macros.c
index 4730a8b..219eb39 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -85,7 +85,23 @@ macro before appending to it.  */)
        bool cvt;
  
        /* Check the type of last-kbd-macro in case Lisp code changed it.  */
-      len = CHECK_VECTOR_OR_STRING (KVAR (current_kboard, Vlast_kbd_macro));
+      /* If Vlast_kbd_macro is Qnil, skip the check and set len to 0.
+       * Flength returns 0 for Qnil, CHECK_VECTOR_OR_STRING has to do the same.
+       * Otherwise CHECK_VECTOR_OR_STRING fails and results in garbage collecting,
+       * which results in (keyboard.c, mark_kboards(void))
+       *
+       * for (p = kb->kbd_macro_buffer; p < kb->kbd_macro_ptr; p++)
+       * 	  mark_object (*p);
+       *
+       * Here, kb->kbd_macro_ptr is not initialized and can contain address
+       * 0x5353535353535353, which results in SIGSEGV trying to access the address.
+       *
+       * https://bugzilla.redhat.com/show_bug.cgi?id=1104012
+       */
+      if (!NILP (KVAR (current_kboard, Vlast_kbd_macro) ))
+        len = CHECK_VECTOR_OR_STRING (KVAR (current_kboard, Vlast_kbd_macro));
+      else
+        len = 0;
  
        /* Copy last-kbd-macro into the buffer, in case the Lisp code
  	 has put another macro there.  */

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Jan Chaloupka]

Changelog:
	line wrapping to 80 characters

In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of current_kboard is
Qnil for the first invocation. If NILP (append) is false,
current_kboard->kbd_macro_ptr has random value (in our case
0x5353535353535353), which after CHECK_VECTOR_OR_STRING failure (invocation
of wrong_type_argument) results in garbage collecting.
During gc, marking of objects is processed and mark_kboards (keyboard.c) is
invoked. Following for loop is fired:

for (p = kb->kbd_macro_buffer; p < kb->kbd_macro_ptr; p++)
                   mark_object (*p);

Since kb->kbd_macro_ptr is set to 0x5353535353535353, mark_object (*p) is
trying to mark object on address out of memory space (or memory that
cannot be accessed). Thus resulting in SIGSEGV signal.

Solution is to check for Qnil before calling CHECK_VECTOR_OR_STRING and set
len to 0 if Qnil occurs.

https://bugzilla.redhat.com/show_bug.cgi?id=1104012

Signed-off-by: Jan Chaloupka <jchaloup@redhat.com>
---
  src/macros.c |   20 +++++++++++++++++++-
  1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/macros.c b/src/macros.c
index 4730a8b..4fd6cb1 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -85,7 +85,25 @@ macro before appending to it.  */)
        bool cvt;
  
        /* Check the type of last-kbd-macro in case Lisp code changed it.  */
-      len = CHECK_VECTOR_OR_STRING (KVAR (current_kboard, Vlast_kbd_macro));
+      /* If Vlast_kbd_macro is Qnil, skip the check and set len to 0.
+       * Flength returns 0 for Qnil, CHECK_VECTOR_OR_STRING has to do the same.
+       * Otherwise CHECK_VECTOR_OR_STRING fails and results in garbage
+       * collecting, which results in (keyboard.c, mark_kboards(void)).
+       * Among others, mark_kboards it executes:
+       *
+       * for (p = kb->kbd_macro_buffer; p < kb->kbd_macro_ptr; p++)
+       * 	  mark_object (*p);
+       *
+       * Here, kb->kbd_macro_ptr is not initialized and can contain address
+       * 0x5353535353535353, which results in SIGSEGV trying to access
+       * the address.
+       *
+       * https://bugzilla.redhat.com/show_bug.cgi?id=1104012
+       */
+      if (!NILP (KVAR (current_kboard, Vlast_kbd_macro) ))
+        len = CHECK_VECTOR_OR_STRING (KVAR (current_kboard, Vlast_kbd_macro));
+      else
+        len = 0;
  
        /* Copy last-kbd-macro into the buffer, in case the Lisp code
  	 has put another macro there.  */

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Andreas Schwab]

Jan Chaloupka <jchaloup@redhat.com> writes:

> Changelog:
> 	line wrapping to 80 characters
>
> In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of current_kboard is
> Qnil for the first invocation. If NILP (append) is false,
> current_kboard->kbd_macro_ptr has random value (in our case
> 0x5353535353535353), which after CHECK_VECTOR_OR_STRING failure (invocation
> of wrong_type_argument) results in garbage collecting.
> During gc, marking of objects is processed and mark_kboards (keyboard.c) is
> invoked. Following for loop is fired:
>
> for (p = kb->kbd_macro_buffer; p < kb->kbd_macro_ptr; p++)
>                   mark_object (*p);
>
> Since kb->kbd_macro_ptr is set to 0x5353535353535353, mark_object (*p) is
> trying to mark object on address out of memory space (or memory that
> cannot be accessed). Thus resulting in SIGSEGV signal.

So the correct solution is to initialize kbd_macro_ptr together with
kbd_macro_buffer.  Otherwise the same situation can still happen any
time garbage collection is called.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Andreas Schwab]

Fixed for emacs 24.4.  Thanks for the report.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Jan Chaloupka]

On 07/29/2014 09:52 AM, Andreas Schwab wrote:
> Jan Chaloupka <jchaloup@redhat.com> writes:
>
>> Changelog:
>> 	line wrapping to 80 characters
>>
>> In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of current_kboard is
>> Qnil for the first invocation. If NILP (append) is false,
>> current_kboard->kbd_macro_ptr has random value (in our case
>> 0x5353535353535353), which after CHECK_VECTOR_OR_STRING failure (invocation
>> of wrong_type_argument) results in garbage collecting.
>> During gc, marking of objects is processed and mark_kboards (keyboard.c) is
>> invoked. Following for loop is fired:
>>
>> for (p = kb->kbd_macro_buffer; p < kb->kbd_macro_ptr; p++)
>>                    mark_object (*p);
>>
>> Since kb->kbd_macro_ptr is set to 0x5353535353535353, mark_object (*p) is
>> trying to mark object on address out of memory space (or memory that
>> cannot be accessed). Thus resulting in SIGSEGV signal.
> So the correct solution is to initialize kbd_macro_ptr together with
> kbd_macro_buffer.  Otherwise the same situation can still happen any
> time garbage collection is called.
Yes, for garbage collector. However, Vlast_kbd_macro will continue being 
Qnil.
The patch is still valid (just without comment about random value of
kbd_macro_ptr and garbage collection):

In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of 
current_kboard is
Qnil for the first invocation. If NILP (append) is false
CHECK_VECTOR_OR_STRING fails (invocation
of wrong_type_argument resulting in emacs_abort). However, it has to pass.

> Andreas.
>

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Andreas Schwab]

Jan Chaloupka <jchaloup@redhat.com> writes:

> Yes, for garbage collector. However, Vlast_kbd_macro will continue being
> Qnil.

Why is that a problem?  That is the default value.

> In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of current_kboard
> is
> Qnil for the first invocation. If NILP (append) is false
> CHECK_VECTOR_OR_STRING fails (invocation
> of wrong_type_argument resulting in emacs_abort).

Where does it call emacs_abort?

> However, it has to pass.

Why?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Jan Chaloupka]

On 07/29/2014 10:37 AM, Andreas Schwab wrote:
> Jan Chaloupka <jchaloup@redhat.com> writes:
>
>> Yes, for garbage collector. However, Vlast_kbd_macro will continue being
>> Qnil.
> Why is that a problem?  That is the default value.
So is it correct if append and Vlast_kbd_macro are both Qnil resulting 
in CHECK_VECTOR_OR_STRING fail?
The bug from BZ is use-case where emacs crashes at startup, loading 
.emacs.desktop file.
>> In function Fstart_kbd_macro (macros.c), Vlast_kbd_macro of current_kboard
>> is
>> Qnil for the first invocation. If NILP (append) is false
>> CHECK_VECTOR_OR_STRING fails (invocation
>> of wrong_type_argument resulting in emacs_abort).
> Where does it call emacs_abort?
Because Vlast_kbd_macro is not VECTOR nor STRING
>
>> However, it has to pass.
> Why?
My first question in this response.
>
> Andreas.
>

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Andreas Schwab]

Jan Chaloupka <jchaloup@redhat.com> writes:

> So is it correct if append and Vlast_kbd_macro are both Qnil resulting in
> CHECK_VECTOR_OR_STRING fail?

Sure, that's the point of the check.  last-kbd-macro is a lisp-level
variable, so it must be checked.

> Because Vlast_kbd_macro is not VECTOR nor STRING

That just calls error, but not emacs_abort.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Jan Chaloupka]

On 07/29/2014 11:04 AM, Andreas Schwab wrote:
> Jan Chaloupka <jchaloup@redhat.com> writes:
>
>> So is it correct if append and Vlast_kbd_macro are both Qnil resulting in
>> CHECK_VECTOR_OR_STRING fail?
> Sure, that's the point of the check.  last-kbd-macro is a lisp-level
> variable, so it must be checked.

Yes, I agree it has to be check. Having .emacs.desktop file with series
of the folkowing kbd macro definitions:

(desktop-create-buffer 206
     ...
     '(defining-kbd-macro global-auto-revert-mode)
     ...
)

append argument of start-kbd-macro is false. But because there is no 
last kbd macro,
check fails. I guess then .emacs.desktop is incorectly written. Thus 
resulting in
proper check fail.

>> Because Vlast_kbd_macro is not VECTOR nor STRING
> That just calls error, but not emacs_abort.
CHECK_VECTOR_OR_STRING -> wrong_type_argument -> xsignal2 -> xsignal

void
xsignal (Lisp_Object error_symbol, Lisp_Object data)
{
   Fsignal (error_symbol, data);
   emacs_abort ();
}

emacs_abort then has to be called after Fsignal finished. Or is there a 
back jmp back to main loop or somewhere else?
> Andreas.
>

bug#18140: [PATCH] macros.c: CHECK_VECTOR_OR_STRING invokes wrong_type_argument for Qnil instead of return 0

[Andreas Schwab]

Jan Chaloupka <jchaloup@redhat.com> writes:

> emacs_abort then has to be called after Fsignal finished.

Fsignal never returns.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."