bug#58327: 29.0.50; Missing staticpro for old_selected_window in window.c

[Gerd Möllmann]

This was found while investigating bug#58024.

Since this is not directly related to bug#58024, and to make sure it's
not forgotten, I'm submitting this report separately.

Below is a copy a mail describing the problem.

From: Po Lu <luangruo@yahoo.com>
Subject: bug#58042: 29.0.50; ASAN use-after-free in re_match_2_internal
To: Gerd Möllmann <gerd.moellmann@gmail.com>
Cc: Eli Zaretskii <eliz@gnu.org>, 58042@debbugs.gnu.org, Alan Third
 <alan@idiocy.org>
Date: Wed, 05 Oct 2022 20:48:25 +0800 (16 hours, 20 minutes, 2 seconds ago)
Resent-From: Po Lu <luangruo@yahoo.com>

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> I don't get an abort, but the ASAN error again

Interesting.

> ==67682==ERROR: AddressSanitizer: heap-use-after-free on address 
> 0x000107130d00 at pc 0x0001002a481c bp 0x00016fdcc3c0 sp 0x00016fdcc3b8
> READ of size 8 at 0x000107130d00 thread T0
>     #0 0x1002a4818 in PSEUDOVECTORP lisp.h:1110
>     #1 0x1002a4888 in SYMBOL_WITH_POS_P lisp.h:1122
>     #2 0x10025a338 in EQ lisp.h:1342
>     #3 0x100280eb0 in run_window_change_functions window.c:3964
>     #4 0x1000f18c4 in redisplay_internal xdisp.c:16600
>     #5 0x100107bf8 in redisplay xdisp.c:16111
>     #6 0x10089364c in -[EmacsView layoutSublayersOfLayer:] nsterm.m:8661
>     #7 0x1900a9624 in CA::Layer::layout_if_needed(CA::Transaction*)+0x224 
> (QuartzCore:arm64e+0x20624)
>     #8 0x1901f661c in CA::Context::commit_transaction(CA::Transaction*,
>     double, double*)+0x1c0 (QuartzCore:arm6
>
> frame #8: 0x0000000100280eb4 emacs`run_window_change_functions at 
> window.c:3964:7
>    3961                    (de-)selected as its frame's or the globally 
> selected
>    3962                    window.  */
>    3963                 if (((frame_selected_change
> -> 3964                       && (EQ (window, old_selected_window)
>    3965                           || EQ (window, selected_window)))
>    3966                      || (frame_selected_window_change
>    3967                          && (EQ (window, FRAME_OLD_SELECTED_WINDOW 
> (f))
>
> (lldb) p window
> (Lisp_Object) $18 = 0x00000001071c2935 (struct window *) $23 = 
> 0x00000001071c2930
> (lldb) p old_selected_window
> (Lisp_Object) $24 = 0x0000000107130d05 (struct Lisp_Vector *) $28 = 
> 0x0000000107130d00
>
> old_selected_window looks strange.  It's a global that is not
> staticpro'd

Isn't old_selected_window supposed to be kept in sync with
FRAME_OLD_SELECTED_WINDOW in old_selected_frame, with the latter being
removed once it is deleted?

Would someone who knows the window code well please take a look at this?


The proposed fix was

From: Gerd Möllmann <gerd.moellmann@gmail.com>
Subject: bug#58042: 29.0.50; ASAN use-after-free in re_match_2_internal 
To: Po Lu <luangruo@yahoo.com>
Cc: Eli Zaretskii <eliz@gnu.org>, 58042@debbugs.gnu.org, Alan Third
 <alan@idiocy.org>
Date: Wed, 05 Oct 2022 14:38:24 +0200 (16 hours, 37 minutes, 19 seconds ago)
Resent-From: Gerd Möllmann <gerd.moellmann@gmail.com>

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> old_selected_window looks strange.  It's a global that is not
> staticpro'd

And with this it works again:

diff --git a/src/window.c b/src/window.c
index 12a212a85a..da80fabe33 100644
--- a/src/window.c
+++ b/src/window.c
@@ -8213,6 +8213,8 @@ init_window_once (void)
 
   minibuf_selected_window = Qnil;
   staticpro (&minibuf_selected_window);
+  old_selected_window = Qnil;
+  staticpro (&old_selected_window);
 
   pdumper_do_now_and_after_late_load (init_window_once_for_pdumper);
 }