GNU bug report logs - #76559
31.0.50; [-O3 + PGTK] Crash when 'copying as kill'/'killing word'

Previous Next

Full log

Message #98 received at 76559 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Michael Albinus <michael.albinus <at> gmx.de>,
 Iurie Marian <marian.iurie <at> gmail.com>, 76559 <at> debbugs.gnu.org
Subject: Re: bug#76559: 31.0.50;
 [-O3 + PGTK] Crash when 'copying as kill'/'killing word'
Date: Fri, 28 Feb 2025 17:05:54 +0000

"Po Lu" <luangruo <at> yahoo.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> See my other message.  I'm still puzzled as to why an unsafe cast in one
>> call path modifies the assumptions made for another call path, but I
>> guess it all falls under undefined behavior and we should simply apply
>> that patch and make sure to follow the C standard more closely.
>
> I revisited the material provisions in n1256, and the problem is that a
> `union buffered_input_event' is read from the EVENT pointer, whether or
> not the object behind the pointer is of a type compatible with the
> union.  It is permissible for an object whose type is a member of a
> union or aggregate to be accessed from a pointer whose type is the same
> union or aggregate, but apparently not more generally to access a union
> from a pointer of its type from which it is only defined to access one
> of its members, even if the latter is identical in size.

My understanding is it is safe to access an entire union at once, as the
old code did, and that GCC concluded based on the undefined behavior
elsewhere that the only possible union members were those with the holes
in the right place.

> I propose this patch:
>
> diff --git a/src/keyboard.c b/src/keyboard.c
> index b22814d702d..0c83880c491 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -3807,7 +3807,13 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
>    union buffered_input_event *next_slot = next_kbd_event (kbd_store_ptr);
>    if (kbd_fetch_ptr != next_slot)
>      {
> -      *kbd_store_ptr = *event;
> +#if defined HAVE_X11 || defined HAVE_PGTK
> +      if (event->kind == SELECTION_REQUEST_EVENT
> +	  || event->kind == SELECTION_CLEAR_EVENT)
> +	kbd_store_ptr->sie = event->sie;
> +      else
> +	kbd_store_ptr->ie = event->ie;
> +#endif /* HAVE_X11 || defined HAVE_PGTK */
>        kbd_store_ptr = next_slot;
>  #ifdef subprocesses
>        if (kbd_buffer_nr_stored () > KBD_BUFFER_SIZE / 2

While the patch does happen to invoke more undefined behavior in a way
which prevents this particular bug, I don't think we should apply it.
The old code in this specific place was perfectly okay (and the new code
contains a bug if neither HAVE_X11 nor HAVE_PGTK is defined).

I still think the patch I posted, which avoids the problem by removing
what appears to be undefined behavior, is a better idea; but I also
still don't understand how undefined behavior in one code path infects
other code paths that don't go through the function.

It seems to me that this is a situation that GCC should warn about,
rather than silently generating unintended code.  Maybe we should submit
your "reduction" case as a wishlist item for a new GCC warning.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.