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

[Victor Toso]

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.

[0] http://www.spice-space.org/static/docs/spice_style.pdf

cheers,
  toso

>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etrunko at redhat.com