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

[Eduardo Lima (Etrunko)]

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.

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