[Spice-devel] [PATCH] [OT] channel: do not free rcc->stream in red_channel_client_disconnect

[Frediano Ziglio]

> 
> On 03/30/2016 06:44 AM, Victor Toso wrote:
> > On Tue, Mar 29, 2016 at 09:35:12AM -0300, Eduardo Lima (Etrunko) wrote:
> >> On 01/19/2016 08:14 AM, Victor Toso wrote:
> >>> Hi,
> >>>
> >>> On Tue, Jan 19, 2016 at 09:52:24AM +0000, Frediano Ziglio wrote:
> >>>> This fixes a crash if red_channel_client disconnect is called
> >>>> handling a message.
> >>>> This can happen for instance handling SPICE_MSGC_ACK which calls
> >>>> red_channel_client_push which try to write items to socket detecting
> >>>> writing errors (for instance socket disconnection).
> >>>> Messages are read in a loop and the red_channel_client_disconnect
> >>>> would cause rcc->stream to be NULL which will turn in a use-after-free
> >>>> problem (stream in red_peer_handle_incoming will use cached stream
> >>>> value).
> >>>>
> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> >>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> >>>> ---
> >>>>  server/red-channel.c | 38 ++++++++++++++++++++++----------------
> >>>>  1 file changed, 22 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/server/red-channel.c b/server/red-channel.c
> >>>> index 306c87d..51e8958 100644
> >>>> --- a/server/red-channel.c
> >>>> +++ b/server/red-channel.c
> >>>> @@ -1231,22 +1231,28 @@ void red_channel_client_ref(RedChannelClient
> >>>> *rcc)
> >>>>  
> >>>>  void red_channel_client_unref(RedChannelClient *rcc)
> >>>>  {
> >>>> -    if (!--rcc->refs) {
> >>>> -        spice_debug("destroy rcc=%p", rcc);
> >>>> -        if (rcc->send_data.main.marshaller) {
> >>>> -            spice_marshaller_destroy(rcc->send_data.main.marshaller);
> >>>> -        }
> >>>> +    if (--rcc->refs != 0) {
> >>>
> >>> My opnion is that doing --(var) on if() does not make the code easy
> >>> readable. rcc->refs will always be decreased so it should be outside
> >>> the conditional.
> >>>
> >>> I see this in multiple places in spice-server but my vote is to change
> >>> that at least for new code :)
> >>>
> >>
> >> IMHO (not entering in the merits of the commit) it is just a matter of
> >> getting used to the features of the programming language. I don't have
> >> any problems with either proposals, but maybe it is just me.
> >>
> >> My point is, let's not restrict ourselves in not exploring all the
> >> features of an already too restricted programming language as C.
> > 
> > Logic negation in a ref counter which is also being decrease is not
> > really a feature of any language to me. I usually take twice as much
> > time to understand and think about logic implications when there is too
> > much in a single line. Maybe it is just me.
> > 
> > I said what I think before but I wouldn't nack a patch because of that.
> > It was more like a suggestion. If we decide to be picky about this
> > things we should (re)define code style [0] and then trying to stick
> > with it... My vote in general is for code readability and let the
> > compiler do this small optimizations.
> 
> Please no, as I said, lets not restrict it too much. As it already
> happened other times on this list (or maybe virt-tools), I would vote
> simply ask for opinion from others when questions like this one rise.
> 
> Cheers!
> 

I think this is going a bit OT.
Not against a style thread but I think the original question from Christophe
was if the patch was worth the backport effort.

Frediano