bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: 73838@debbugs.gnu.org
>> Date: Wed, 16 Oct 2024 21:03:15 +0200
>> 
>> > Thus, IMO your suggestion is in a sense a step back, because it
>> > assumes that TTY frames can never have these decorations and can never
>> > have different cursor types. So my suggestion would be to do the
>> > opposite: understand why FRAME_OUTPUT_DATA segfaults when dereferenced
>> > on TTY frames, and fix that so that it doesn't.
>> 
>> But the current situation is that we follow from the presence of an
>> internal border that it's a window system frame. We're using
>> FRAME_OUTPUT_DATA. If that would segv it would be a good thing. It
>> doesn't do that, it just silently accesses some unrelated memory (in my
>> case this is equivalent to casting the actual output_data contents to
>> (struct ns_output *) regardless of what it actually is.
>> 
>> I've just dragged the FRAME_WINDOW_P out of this stuff because the
>> whole if-statement is concerned with cursor = ... using FRAME_OUTPUT_DATA.
>
> My suggestion is to extend 'struct tty_display_info' so that
> FRAME_OUTPUT_DATA on TTY frames will not access unrelated memory, when
> these macros/inline functions are called.  Alternatively, we could have
> the macros/functions (FRAME_INTERNAL_BORDER etc.) test for TTY frame
> and DTRT.

FRAME_OUTPUT_DATA is meaningful only for window system frames. Each
window system's "term" header defines it. For example

  xterm.h:
  #define FRAME_X_OUTPUT(f) ((f)->output_data.x)
  #define FRAME_OUTPUT_DATA(f) FRAME_X_OUTPUT (f)

  nsterm.h:
  #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)

and so on. So using FRAME_OUTPUT_DATA is per se only valid if
FRAME_WINDOW_P. Which is equivalent to FRAME_NS_P in my case, or
whatever someone uses.

  #ifdef HAVE_X_WINDOWS
  #define FRAME_WINDOW_P(f) FRAME_X_P (f)
  #endif
  #ifdef HAVE_NS
  #define FRAME_WINDOW_P(f) FRAME_NS_P(f)
  #endif
  #ifndef FRAME_WINDOW_P
  #define FRAME_WINDOW_P(f) ((void) (f), false)
  #endif

I think changing that would be a major surgery. It's probably easier to
add checks like I did in the patch to FRAME_OUTPUT_DATA if the frame in
questioin is indeed a window system frame. It can be decided at run-time
only anyway.

The other idea is, IIUC, is to make code using FRAME_OUTPUT_DATA like
this one

  if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0
      && !NILP (get_frame_param (f, Qdrag_internal_border)))
    {
      enum internal_border_part part = frame_internal_border_part (f, x, y);

      switch (part)
        {
        case INTERNAL_BORDER_NONE:
          if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
            /* Reset cursor.  */
            cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

work by making sure that their if-conditions can't be true, if there
any. In the above case by making FRAME_INTERNAL_BORDER_WIDTH return 0 if the
frame is not FRAME_WINDOW_P. In other cases like this one

  if (EQ (window, f->tool_bar_window))
    {
      note_tool_bar_highlight (f, x, y);
      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

or here

  if (part == ON_MODE_LINE || part == ON_HEADER_LINE || part == ON_TAB_LINE
      || part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
    {
      note_mode_line_or_margin_highlight (window, x, y, part);

#ifdef HAVE_WINDOW_SYSTEM
      if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
        {
          cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
          /* Show non-text cursor (Bug#16647).  */
          goto set_cursor;
        }
      else
#endif
        return;
    }

by doing something else.

I have to admit that I don't like that. I don't understand what is wrong
to check FRAME_WINDOW_P before using something (using FRAME_OUTPUT_DATA)
that requires FRAME_WINDOW_P to be valid.