The vnc server should send desktop resize notifications unconditionally
on a new client connect, for feature negotiation reasons.  Add a bool
flag to vnc_desktop_resize() to force sending the message even in case
there is no size change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 247e80d8f5c8..bdaf384f71a4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -664,13 +664,14 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
 }
 
 
-static void vnc_desktop_resize(VncState *vs)
+static void vnc_desktop_resize(VncState *vs, bool force)
 {
     if (vs->ioc == NULL || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
         return;
     }
     if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
-        vs->client_height == pixman_image_get_height(vs->vd->server)) {
+        vs->client_height == pixman_image_get_height(vs->vd->server) &&
+        !force) {
         return;
     }
 
@@ -800,7 +801,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
 
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
-        vnc_desktop_resize(vs);
+        vnc_desktop_resize(vs, false);
         if (vs->vd->cursor) {
             vnc_cursor_define(vs);
         }
@@ -2143,7 +2144,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             break;
         }
     }
-    vnc_desktop_resize(vs);
+    vnc_desktop_resize(vs, true);
     check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
     vnc_led_state_change(vs);
     if (vs->vd->cursor) {
-- 
2.27.0

Hi

On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> The vnc server should send desktop resize notifications unconditionally
> on a new client connect, for feature negotiation reasons.  Add a bool
> flag to vnc_desktop_resize() to force sending the message even in case
> there is no size change.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

In principle, this looks harmless. But the spec says:

"The server should only send a *DesktopSize* pseudo-rectangle when an
actual change of the framebuffer dimensions has occurred. Some clients
respond to a *DesktopSize* pseudo-rectangle in a way that could send the
system into an infinite loop if the server sent out the pseudo-rectangle
for anything other than an actual change."
(
https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
)

I can't say if sending desktop resize during the initial SetEncoding phase
is really compliant with the specification. Also, it's unclear to me if the
client is allowed to SetEncoding multiple times (in which there would be no
dimension change occurring).

What did you fix with this? Is it worth a clarification in the
specification?

thanks

---
>  ui/vnc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 247e80d8f5c8..bdaf384f71a4 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -664,13 +664,14 @@ void vnc_framebuffer_update(VncState *vs, int x, int
> y, int w, int h,
>  }
>
>
> -static void vnc_desktop_resize(VncState *vs)
> +static void vnc_desktop_resize(VncState *vs, bool force)
>  {
>      if (vs->ioc == NULL || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
>          return;
>      }
>      if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
> -        vs->client_height == pixman_image_get_height(vs->vd->server)) {
> +        vs->client_height == pixman_image_get_height(vs->vd->server) &&
> +        !force) {
>          return;
>      }
>
> @@ -800,7 +801,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
>
>      QTAILQ_FOREACH(vs, &vd->clients, next) {
>          vnc_colordepth(vs);
> -        vnc_desktop_resize(vs);
> +        vnc_desktop_resize(vs, false);
>          if (vs->vd->cursor) {
>              vnc_cursor_define(vs);
>          }
> @@ -2143,7 +2144,7 @@ static void set_encodings(VncState *vs, int32_t
> *encodings, size_t n_encodings)
>              break;
>          }
>      }
> -    vnc_desktop_resize(vs);
> +    vnc_desktop_resize(vs, true);
>      check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
>      vnc_led_state_change(vs);
>      if (vs->vd->cursor) {
> --
> 2.27.0
>
>
>

-- 
Marc-André Lureau

On Fri, Dec 04, 2020 at 03:57:23PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > The vnc server should send desktop resize notifications unconditionally
> > on a new client connect, for feature negotiation reasons.  Add a bool
> > flag to vnc_desktop_resize() to force sending the message even in case
> > there is no size change.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >
> 
> In principle, this looks harmless. But the spec says:
> 
> "The server should only send a *DesktopSize* pseudo-rectangle when an
> actual change of the framebuffer dimensions has occurred. Some clients
> respond to a *DesktopSize* pseudo-rectangle in a way that could send the
> system into an infinite loop if the server sent out the pseudo-rectangle
> for anything other than an actual change."
> (
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
> )
> 
> I can't say if sending desktop resize during the initial SetEncoding phase
> is really compliant with the specification. Also, it's unclear to me if the
> client is allowed to SetEncoding multiple times (in which there would be no
> dimension change occurring).
> 
> What did you fix with this? Is it worth a clarification in the
> specification?

Well, for ExtendedDesktopResize the spec explicitly asked for this.
But, yes, for DesktopResize this is not needed.  But it also shouldn't
cause much trouble.  It is sent before any actual display updates, so
concerns whenever the client should consider the screen content invalid
or not are moot.

I could squash this into patch #8 and do it for ExtendedDesktopResize
only ...

take care,
  Gerd

Hi

On Tue, Dec 8, 2020 at 10:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Fri, Dec 04, 2020 at 03:57:23PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > > The vnc server should send desktop resize notifications unconditionally
> > > on a new client connect, for feature negotiation reasons.  Add a bool
> > > flag to vnc_desktop_resize() to force sending the message even in case
> > > there is no size change.
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > >
> >
> > In principle, this looks harmless. But the spec says:
> >
> > "The server should only send a *DesktopSize* pseudo-rectangle when an
> > actual change of the framebuffer dimensions has occurred. Some clients
> > respond to a *DesktopSize* pseudo-rectangle in a way that could send the
> > system into an infinite loop if the server sent out the pseudo-rectangle
> > for anything other than an actual change."
> > (
> >
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
> > )
> >
> > I can't say if sending desktop resize during the initial SetEncoding
> phase
> > is really compliant with the specification. Also, it's unclear to me if
> the
> > client is allowed to SetEncoding multiple times (in which there would be
> no
> > dimension change occurring).
> >
> > What did you fix with this? Is it worth a clarification in the
> > specification?
>
> Well, for ExtendedDesktopResize the spec explicitly asked for this.
> But, yes, for DesktopResize this is not needed.  But it also shouldn't
> cause much trouble.  It is sent before any actual display updates, so
> concerns whenever the client should consider the screen content invalid
> or not are moot.
>
> I could squash this into patch #8 and do it for ExtendedDesktopResize
> only ...
>

Ok, it's probably fine (dunno), you could also capture that in the commit
message, or as code comment.


-- 
Marc-André Lureau