[Spice-devel] [PATCH] remove dandling pointer for RedCharDeviceVDIPort

[Uri Lublin]

On 05/04/2016 11:51 AM, Frediano Ziglio wrote:
>>
>> On 05/03/2016 01:53 PM, Frediano Ziglio wrote:
>>>>
>>>> On 05/02/2016 11:25 AM, Frediano Ziglio wrote:
>>>>> This was caused by commit 1cec1c5118b65124de6bc6f984f376ff4e297bfb
>>>>> ("reds: Make VDIPortState a GObject") as the lifespan of RedCharDevice
>>>>> was changed.
>>>>>
>>>>> This could be reproduced with:
>>>>> - start rhel7 machine
>>>>> - connect remote viewer (RV)
>>>>> - RV: login
>>>>> - connect ssh
>>>>> - SSH: stop agent
>>>>> - disconnect RV
>>>>> - SSH: start agent
>>>>> - connect to RV
>>>>>
>>>>> and caused (using address sanitizer):
>>>> (snipped)
>>>>>
>>>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>>>> ---
>>>>>  server/reds.c | 10 ++++------
>>>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/server/reds.c b/server/reds.c
>>>>> index efd1429..67c262a 100644
>>>>> --- a/server/reds.c
>>>>> +++ b/server/reds.c
>>>>> @@ -603,12 +603,10 @@ void reds_client_disconnect(RedsState *reds,
>>>>> RedClient *client)
>>>>>          reds_mig_remove_wait_disconnect_client(reds, client);
>>>>>      }
>>>>>
>>>>
>>>> Hi Frediano,
>>>>
>>>> Shouldn't the patch fix "agent_attached"  value instead of ignoring it ?
>>>> Why ignore its value here and not in other places ?
>>>>
>>>> Regards,
>>>>      Uri.
>>>>
>>>
>>> Honestly more I look at the patch and this fix and more I think it's all
>>> a big bug...
>>> What I know for sure is that this patch fix a dandling pointer.
>>
>> That's true.
>> But I think we should have a better fix that does not ignore the value
>> of agent_attached, or at least we should verify that other uses of this
>> variable are correct and this is the only place needs fixing.
>>
>
> Don't be confused by the check removal. Previously (before the offending
> commit) the check was checking the existence of RedCharDevice. Now
> RedCharDevice is always present so the check should be removed.
>
> I don't see any reason why this patch should not be merged (beside the
> comment typo fix).
>
> All other reasoning came from looking the current state of the related
> code.


The specific fix looks fine to me -- when a client disconnects remove it
from the list of clients connected to the spice char-device.

My concern is that earlier patch(es) replaced
(reds->agent_state.base == NULL) with the boolean agent_attached.

This patch ignores the value of agent_attached in this specific
case of client disconnection, so we found a place where the
earlier patch's assumption does not hold.

I'm fine with applying this patch.

We should audit the code to find if there are more cases
when agent_attached value is incorrect/irrelevant.

Thanks,
     Uri.