[Spice-devel] [PATCH spice-gtk v3 1/6] display-gst: check codec type before creating decoder

[Christophe Fergeau]

On Thu, Jun 08, 2017 at 02:23:41PM +0200, Victor Toso wrote:
> On Thu, Jun 08, 2017 at 12:43:53PM +0200, Christophe Fergeau wrote:
> > On Thu, Jun 08, 2017 at 12:36:49PM +0200, Victor Toso wrote:
> > > > In this case, it seems the user could trigger this warning by sending
> > > > an invalid codec type in a SpiceMsgDisplayStreamCreate message?
> > >
> > > Wouldn't that be a bug? As client has capabilities to explicit say to
> > > Spice which video codecs it can handle Spice shouldn't try to create a
> > > video stream with unsupported video codec.
> >
> > A bug in which component?
> 
> - spice-gtk if it was not clear about its video-codec capabilities
> - spice if it knew client can't handle video-codec but tried to create a
>   stream anyway
> 
> > I consider data coming from the network as "user data", as a
> > well-behaved client should not do that, but we could be fed anything
> > from buggy, hostile, ... clients.
> 
> Sorry, I did not understand you here.

Sorry, I was answering this as a spice-server patch, not spice-gtk :(
No surprise I confused you ;)


> There could be a valid spice message but with content that ignores
> settings that were set.
> 
> > If spice-server code does not enforce that the data in this message is
> > valid before this g_return_if_fail(), then imo the g_return_if_fail()
> > can be triggered by user-provided data.
> 
> Still not following you. This is a client-side patch that is taking
> spice-server data with valid message's content but with a codec_value
> that can't be used... In this case, we should be loud about it so we can
> check if it is spice-gtk or spice bug... Either way, I would see this a
> bug and hence the critical.

I would not use g_return_if_fail() to report buggy server behaviour,
only if this is something under spice-gtk control which should never
happen because of the way spice-gtk code is written.

People in the past have mentioned this from g_return_if_fail()
documentation "If G_DISABLE_CHECKS is defined then the check is not
performed. You should therefore not depend on any side effects of expr."
though I don't think anyone is doing this :)

Not really worth all that discussion, your patch is ok :)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/498c63b3/attachment-0001.sig>