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

[Eduardo Lima (Etrunko)]

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!

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