[MPlayer-dev-eng] [PATCH] x11_common: XChangeProperty uses long for 32-bits

[Alexander Strasser]

Hi,

Nicolas George wrote:
> Le decadi 30 thermidor, an CCXIX, Alexander Strasser a écrit :
> >   Of course! Good catch. It is always the simplest things, that can go
> > awfully wrong. Thank you for having a close look.
> > 
> >   Looking at my comment again, we are not checking if the PID fits into
> > 32 bits, I guess this way it might happen that the X server receives a
> > partial PID. Could we use a bitwise AND operation to only assign bits
> > 0 to 31, so that we do the early return when pid contains a value that
> > doesn't fit into 32 bits? That approach might fail on sign extension
> > of a negative value of a signed 32-bit pid_t into a signed 64 bit long
> > type in some cases. But I would like to think that is more hypothetical.
> 
> A process ID is positive, while pid_t is signed: using a binary AND to mask
> only bits 0 to 30 (and not 31) should be safe.

  OK

> In the meantime, I could test the code on an UltraSparc in the GCC Compile
> Farm: the current code dies with a SIGBUS, the modified code works and sets
> _NET_WM_PID correctly. Here is the updated patch.

  Thanks!

> If there is no further remarks, I can push any time.

  A few nits, else I am fine with it. Just wait about a day or a night.
If no one objects in the mean time, please apply. I could also commit it,
but that would probably take longer as I am not at home this weekend. So
I strongly encourage you to take action.

> From a3567992271ef7254822270d0ee75fa33750ee4f Mon Sep 17 00:00:00 2001
> From: Nicolas George <nicolas.george at normalesup.org>
> Date: Mon, 15 Aug 2011 19:06:49 +0200
> Subject: [PATCH] x11_common: XChangeProperty uses long for 32-bits.
> 
> Adapted from a patch in OpenBSD's ports tree,
> with siggestions from Alexander Strasser.

  Typo: suggestions.

> ---
>  libvo/x11_common.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/libvo/x11_common.c b/libvo/x11_common.c
> index b490ded..cd62b9c 100644
> --- a/libvo/x11_common.c
> +++ b/libvo/x11_common.c
> @@ -729,12 +729,17 @@ void vo_x11_classhint(Display * display, Window window, const char *name)
>  {
>      XClassHint wmClass;
>      pid_t pid = getpid();
> +    long prop = pid & 0x7FFFFFFF;
>  
>      wmClass.res_name = vo_winname ? vo_winname : name;
>      wmClass.res_class = "MPlayer";
>      XSetClassHint(display, window, &wmClass);
> +
> +    /* PID sizes other than 32-bit are not handled by the EWMH spec */
> +    if ((pid_t)prop != pid)
> +        return;

  Could you please insert an empty line after the return statement? It
makes it IMHO easier to see the potential change of execution flow.

>      XChangeProperty(display, window, XA_NET_WM_PID, XA_CARDINAL, 32,
> -                    PropModeReplace, (unsigned char *) &pid, 1);
> +                    PropModeReplace, (unsigned char *)&prop, 1);
>  }
>  
>  Window vo_window = None;
> -- 
> 1.7.5.4

Thank you for your work,
  Alexander