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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel