[Spice-devel] [PATCH] Make sure g_object_new receive the correct data

[Frediano Ziglio]

> 
> On Fri, Jun 03, 2016 at 12:00:24PM +0100, Frediano Ziglio wrote:
> > g_object_new is a variadic function which take property values.
> > As compiler cannot check if these property values are correct
> > make sure they are using casts.
> > This actully fix a crash in reds.c for 32 bit architectures.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/main-dispatcher.c | 2 +-
> >  server/reds.c            | 4 ++--
> >  server/smartcard.c       | 4 ++--
> >  server/spicevmc.c        | 4 ++--
> >  4 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> > index bc0de24..93de440 100644
> > --- a/server/main-dispatcher.c
> > +++ b/server/main-dispatcher.c
> > @@ -292,7 +292,7 @@ MainDispatcher* main_dispatcher_new(RedsState *reds,
> > SpiceCoreInterfaceInternal
> >      MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
> >                                          "spice-server", reds,
> >                                          "core-interface", core,
> > -                                        "max-message-type",
> > MAIN_DISPATCHER_NUM_MESSAGES,
> > +                                        "max-message-type", (guint)
> > MAIN_DISPATCHER_NUM_MESSAGES,
> >                                          NULL);
> 
> The MAIN_DISPATCHER_NUM_MESSAGES message comes from an enum, so technically
> it could be as small as a 'char', but in practice it'll always be an int.
> In any case var-args will always promote types small that int, to be the
> size of an int.
> 
> IOW, this cast shouldn't be needed, since it'll already be past as an
> int by the compiler.
> 

Agreed, not needed. Just does not hurt as hint to check the type
if changes.

> 
> > diff --git a/server/reds.c b/server/reds.c
> > index 4fd1d35..5ec73b7 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -4379,8 +4379,8 @@ static RedCharDeviceVDIPort
> > *red_char_device_vdi_port_new(RedsState *reds)
> >  {
> >      return g_object_new(RED_TYPE_CHAR_DEVICE_VDIPORT,
> >                          "spice-server", reds,
> > -                        "client-tokens-interval", REDS_TOKENS_TO_SEND,
> > -                        "self-tokens", REDS_NUM_INTERNAL_AGENT_MESSAGES,
> > +                        "client-tokens-interval", (guint64)
> > REDS_TOKENS_TO_SEND,
> > +                        "self-tokens", (guint64)
> > REDS_NUM_INTERNAL_AGENT_MESSAGES,
> >                          "opaque", reds,
> >                          NULL);
> >  }
> 
> This all make total sense though, since you're forcing use of a larger type
> than the compiler would otherwise use - it would use int by default,but you
> want a 64-bit type.
> 

Yes, this is the bug. Variadic are promoted to int/unsigned int. In case of
64 bit platform usually is promoted to 64 bit as stated in the ABI so this
is not a problem there but for 32 bit (at least x86) the 64 bit will
be read from the 2 32 bit passes, the value and the next string pointer so
the REDS_NUM_INTERNAL_AGENT_MESSAGES will be interpreted as the next
property name!

> 
> > diff --git a/server/smartcard.c b/server/smartcard.c
> > index 872aa1d..9b1b3d6 100644
> > --- a/server/smartcard.c
> > +++ b/server/smartcard.c
> > @@ -261,8 +261,8 @@ static RedCharDeviceSmartcard
> > *smartcard_device_new(RedsState *reds, SpiceCharDe
> >      char_dev = g_object_new(RED_TYPE_CHAR_DEVICE_SMARTCARD,
> >                              "sin", sin,
> >                              "spice-server", reds,
> > -                            "client-tokens-interval", 0ULL,
> > -                            "self-tokens", ~0ULL,
> > +                            "client-tokens-interval", (guint64) 0ULL,
> > +                            "self-tokens", (guint64) ~0ULL,
> >                              NULL);
> >  
> >      g_object_set(char_dev, "opaque", char_dev, NULL);
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index b662d94..a863e39 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance
> > *sin,
> >      return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> >                          "sin", sin,
> >                          "spice-server", reds,
> > -                        "client-tokens-interval", 0ULL,
> > -                        "self-tokens", ~0ULL,
> > +                        "client-tokens-interval", (guint64) 0ULL,
> > +                        "self-tokens", (guint64) ~0ULL,
> >                          "opaque", opaque,
> >                          NULL);
> 
> AFAICT, these are all redundant since the ULL suffice on the constant
> should already ensure it is passed as a 64-bit type.
> 

Not really portable assumption, in theory could be 128 bit or 256 in
a remote future. I think Gcc already support 128 bit integers.

> Regards,
> Daniel

Frediano