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

[Victor Toso]

Hi,

On Thu, Jun 08, 2017 at 02:33:58PM +0200, Christophe Fergeau wrote:
> 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 ;)

Aha! Okay

> 
> 
> > 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 :)

Ah, I liked the discusison mostly because this would be a spice <->
spice-gtk communication bug and I'm not sure either if we should go for
CRITICAL or WARNING messages in such cases.

I mean, in the past, for spice server, there was discussion about when
to assert(). The result was that only when it would be a bug inside the
component (never on client's input/configuration for instance)

Anyway, thanks for the discussion :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/2eeeeb19/attachment.sig>