Re: master 242a4b49cb: Minor cleanups to X drag-and-drop code

Re: master 242a4b49cb: Minor cleanups to X drag-and-drop code

[Jashank Jeremy]

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

> diff --git a/src/xterm.c b/src/xterm.c
> index a8745894eb2..a3b7c4ac257 100644
> --- a/src/xterm.c
> +++ b/src/xterm.c
> [...]
> @@ -1977,6 +2012,9 @@ xm_read_drag_receiver_info (struct x_display_info *dpyinfo,
>        rec->byteorder = XM_BYTE_ORDER_CUR_FIRST;
>      }
>
> +  if (data[1] > XM_DRAG_PROTOCOL_VERSION)
> +    rc = 0;
> +
>    if (tmp_data)
>      XFree (tmp_data);

Whilst chasing down the build failure against e465ea816d, I observed
some compiler warnings related to this patch:

    .../emacs/src/src/xterm.c:1994:7: warning: variable 'data' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
      if (rc)
	  ^~
    .../emacs/src/src/xterm.c:2016:7: note: uninitialized use occurs here
      if (data[1] > XM_DRAG_PROTOCOL_VERSION)
	  ^~~~
    .../emacs/src/src/xterm.c:1994:3: note: remove the 'if' if its condition is always true
      if (rc)
      ^~~~~~~
    .../emacs/src/src/xterm.c:1977:16: note: initialize the variable 'data' to silence this warning
      uint8_t *data;
		   ^
		    = NULL

This looks like a plausible issue --- XGetWindowProperty looks to be
fallible, and in the event it fails we're potentially dereferencing an
uninitialised value.  I suspect the check introduced in this commit
should be within the immediately preceding region; however, I do not
know what the implications of such a change would be, and thus won't
concretely suggest one.

    ~jashank

[-- Attachment #2: OpenPGP Digital Signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: master 242a4b49cb: Minor cleanups to X drag-and-drop code

[Po Lu]

Jashank Jeremy <jashank@rulingia.com.au> writes:

> Whilst chasing down the build failure against e465ea816d, I observed
> some compiler warnings related to this patch:
>
>     .../emacs/src/src/xterm.c:1994:7: warning: variable 'data' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>       if (rc)
> 	  ^~
>     .../emacs/src/src/xterm.c:2016:7: note: uninitialized use occurs here
>       if (data[1] > XM_DRAG_PROTOCOL_VERSION)
> 	  ^~~~
>     .../emacs/src/src/xterm.c:1994:3: note: remove the 'if' if its condition is always true
>       if (rc)
>       ^~~~~~~
>     .../emacs/src/src/xterm.c:1977:16: note: initialize the variable 'data' to silence this warning
>       uint8_t *data;
> 		   ^
> 		    = NULL
>
> This looks like a plausible issue --- XGetWindowProperty looks to be
> fallible, and in the event it fails we're potentially dereferencing an
> uninitialised value.  I suspect the check introduced in this commit
> should be within the immediately preceding region; however, I do not
> know what the implications of such a change would be, and thus won't
> concretely suggest one.

Silly compiler.  If rc is true, tmp_data is always initialized.

Re: master 242a4b49cb: Minor cleanups to X drag-and-drop code

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Silly compiler.  If rc is true, tmp_data is always initialized.

Nevermind, should be fixed now.  Thanks.

Re: master 242a4b49cb: Minor cleanups to X drag-and-drop code

[Eli Zaretskii]

> branch: master
> commit 242a4b49cb8397ae4eae0a3d7941e9d5afa16cd7
> Author: Po Lu <luangruo@yahoo.com>
> Commit: Po Lu <luangruo@yahoo.com>
> 
>     Minor cleanups to X drag-and-drop code
>     
>     * src/xterm.c (struct x_client_list_window): Write comments
>     describing the meaning of each field.

Thanks for improving the comments to the code.

> +  /* The event mask for the window before Emacs selected for events on
> +     it.  */
>    long previous_event_mask;

There's something wrong with this comment: I cannot parse that
sentence.

Re: master 242a4b49cb: Minor cleanups to X drag-and-drop code

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> +  /* The event mask for the window before Emacs selected for events on
>> +     it.  */
>>    long previous_event_mask;

> There's something wrong with this comment: I cannot parse that
> sentence.

It's the bitmask of input events that Emacs asked (selected) for
previous to it being changed in `x_dnd_compute_toplevels'.  I see the
problem with the comment now, and will correct it.  Thanks.