Gerd Hoffmann <kra...@redhat.com> writes:
> Spotted by Coverity:
>
> 876 static int vnc_update_client_sync(VncState *vs, int has_dirty)
> 877 {
>
> (1) Event freed_arg: "vnc_update_client(VncState *, int)" frees "vs".
> [details]
> Also see events: [deref_arg]
>
> 878 int ret = vnc_update_client(vs, has_dirty);
>
> (2) Event deref_arg: Calling "vnc_jobs_join(VncState *)" dereferences
> freed pointer "vs". [details]
> Also see events: [freed_arg]
>
> 879 vnc_jobs_join(vs);
> 880 return ret;
> 881 }
>
> Remove vnc_update_client_sync wrapper, replace it with an additional
> argument to vnc_update_client, so we can so the sync properly in
> vnc_update_client (i.e. skip it in case of a client disconnect).
>
> Cc: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> ui/vnc.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 5601cc3..b0efb1f 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -416,8 +416,7 @@ out_error:
> 3) resolutions > 1024
> */
>
> -static int vnc_update_client(VncState *vs, int has_dirty);
> -static int vnc_update_client_sync(VncState *vs, int has_dirty);
> +static int vnc_update_client(VncState *vs, int has_dirty, bool sync);
> static void vnc_disconnect_start(VncState *vs);
>
> static void vnc_colordepth(VncState *vs);
> @@ -750,7 +749,7 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
> QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
> vs->force_update = 1;
> - vnc_update_client_sync(vs, 1);
> + vnc_update_client(vs, 1, true);
> /* vs might be free()ed here */
> }
> }
> @@ -873,14 +872,7 @@ static int find_and_clear_dirty_height(struct VncState
> *vs,
> return h;
> }
>
> -static int vnc_update_client_sync(VncState *vs, int has_dirty)
> -{
> - int ret = vnc_update_client(vs, has_dirty);
> - vnc_jobs_join(vs);
This is the problematic use, and you move it...
> - return ret;
> -}
> -
> -static int vnc_update_client(VncState *vs, int has_dirty)
> +static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
> {
> if (vs->need_update && vs->csock != -1) {
> VncDisplay *vd = vs->vd;
> @@ -939,8 +931,11 @@ static int vnc_update_client(VncState *vs, int has_dirty)
> return n;
> }
>
> - if (vs->csock == -1)
> + if (vs->csock == -1) {
> vnc_disconnect_finish(vs);
> + } else if (sync) {
> + vnc_jobs_join(vs);
> + }
... here, under a "not freed" guard.
>
> return 0;
> }
> @@ -2749,7 +2744,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> vnc_unlock_display(vd);
>
> QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> - rects += vnc_update_client(vs, has_dirty);
> + rects += vnc_update_client(vs, has_dirty, false);
> /* vs might be free()ed here */
> }
Reviewed-by: Markus Armbruster <arm...@redhat.com>
