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

[Frediano Ziglio]

> 
> Hi,
> 
> On Thu, Jun 08, 2017 at 10:39:53AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Thu, Jun 08, 2017 at 03:55:11PM +0200, Victor Toso wrote:
> > > > > 
> > > > > 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 :)
> > > 
> > > To tie this in with the logging discussion, you can read the latest
> > > comment from ebassi in this blog post about g_log_structured ;)
> > > 
> > > https://blog.gtk.org/2017/05/04/logging-and-more/
> > > 
> > > Christophe
> > > 
> > 
> > Wrong thread? Looks like the structured logging would be more
> > appropriate in Chistophe D thread.
> > 
> > About CRITICAL or WARNING personally I think if you are detecting
> > that server for some reason is sending garbage telling the used on
> > the UI and closing the connection/program is a good solution for
> > an user application.
> 
> Not really garbage but likely a bug. In this specific case we should
> drop the stream and server should handle it gracefully too, no need to
> assert or quit the application because of that.
> 
> Both warning and critical are fine, I guess. Documentation says:
> 
>   "g_return_val_if_reached()) should only be used for programming
>   errors, a typical use case is checking for invalid parameters at the
>   beginning of a public function. They should not be used if you just
>   mean "if (error) return""
> 
> I would say that this can be checked as programming error and not just
> any-type-of-error, for that reason I'm still keeping the
> g_return_if_fail()
> 
>   toso
> 
> > For the server is a completely different case as the termination
> > cause a DoS of the entire VM and also we don't have a user to
> > communicate to.
> >

I won't say is a programmer error if you are checking a value from
the network. Maybe the problem is just that the value came straight
from the network while it should be validated a bit before.

Then I would put a check on display_handle_stream_create. Surely
in that function I won't handle the invalid value as a programmer
error. You can either report the error to the server or threat like
a protocol error and close the connection.

Frediano