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@xxxxxxxxxx>
---
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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel