[Spice-devel] [spice-server 04/10] Remove IncomingHandlerInterface::{alloc, release}_msg_buf

[Jonathon Jongsma]

On Mon, 2017-02-13 at 05:10 -0500, Frediano Ziglio wrote:
> > 
> > On Wed, Feb 08, 2017 at 12:54:44PM -0600, Jonathon Jongsma wrote:
> > > On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> > > > Similarly to the previous commits, this removes an indirection
> > > > level,
> > > > IncomingHandlerInterface stores pointers to
> > > > alloc_recv_buf/release_recv_buf vfuncs, but these are always
> > > > set to
> > > > RedChannel::alloc_recv_buf/RedChannel::release_recv_buf, which
> > > > are
> > > > also
> > > > vfuncs which can be overridden if needed. This commit removes
> > > > the
> > > > indirection and directly calls the relevant methods.
> > > > 
> > > > Not sure whether the corresponding vfuncs belong in
> > > > RedChannel or if they should be moved to RedChannelClient.
> > > 
> > > They do seem more appropriate to RedChannelClient, since the
> > > first
> > > argument is RedChannelClient*. If we do move them, it could be
> > > done in
> > > a subsequent commit, though. Maybe more of this stuff should be
> > > moved
> > > to the client?
> > 
> > A bit involved to move (eg the callbacks in CommonDisplayChannel
> > uses a buffer from CommonDisplayChannelPrivate, and there is no
> > CommonDisplayChannelClient, ...), so I'd leave that as improvements
> > for
> > another time ;)
> > 
> > Christophe
> > 
> 
> Mumble mumble mumble... bug :-(
> 
> What happens with multiple clients sending data to these channels?
> The message buffer is shared between clients and so potentially
> the second client will overwrite the message coming from the first.
> Not likely to happen (only if message is read in chunk so large one
> which usually could be migration data, file transfer or usb).
> 
> Happily we don't support multiple clients (officially).
> Could this be used for exploits (like overwriting length)? I don't
> think so, messages from the same channel are handled in a single
> thread.
> 
> Frediano


I actually had the same thought when I was reviewing things (multiple
clients potentially sharing a buffer on the channel object). I was
going to investigate whether it was actually a problem, but I forgot
about it. I'm glad you mentioned it.

Jonathon