[Qemu-devel] [PATCH v1] ui: correctly advance output buffer when writing SASL data

[Qemu-devel] [PATCH v1] ui: correctly advance output buffer when writing SASL data

[Daniel P. Berrangé]

In this previous commit:

  commit 8f61f1c5a6bc06438a1172efa80bc7606594fa07
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Mon Dec 18 19:12:20 2017 +0000

    ui: track how much decoded data we consumed when doing SASL encoding

I attempted to fix a flaw with tracking how much data had actually been
processed when encoding with SASL. With that flaw, the VNC server could
mistakenly discard queued data that had not been sent.

The fix was not quite right though, because it merely decremented the
vs->output.offset value. This is effectively to discarding data from the
end of the pending output buffer. We actually need to discard data from
the start of the pending output buffer. We also want to free memory that
is no longer required. The correct way to handle this is to use the
buffer_advance() helper method instead of directly manipulating the
offset value.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 74a5f513f2..fbccca8c8a 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -84,7 +84,7 @@ size_t vnc_client_write_sasl(VncState *vs)
         } else {
             vs->force_update_offset -= vs->sasl.encodedRawLength;
         }
-        vs->output.offset -= vs->sasl.encodedRawLength;
+        buffer_advance(&vs->output, vs->sasl.encodedRawLength);
         vs->sasl.encoded = NULL;
         vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;
     }
-- 
2.14.3

Re: [Qemu-devel] [PATCH v1] ui: correctly advance output buffer when writing SASL data

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On 02/01/2018 09:58 AM, Daniel P. Berrangé wrote:
> In this previous commit:
> 
>   commit 8f61f1c5a6bc06438a1172efa80bc7606594fa07
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Mon Dec 18 19:12:20 2017 +0000
> 
>     ui: track how much decoded data we consumed when doing SASL encoding
> 
> I attempted to fix a flaw with tracking how much data had actually been
> processed when encoding with SASL. With that flaw, the VNC server could
> mistakenly discard queued data that had not been sent.
> 
> The fix was not quite right though, because it merely decremented the
> vs->output.offset value. This is effectively to discarding data from the

s/to //

> end of the pending output buffer. We actually need to discard data from
> the start of the pending output buffer. We also want to free memory that
> is no longer required. The correct way to handle this is to use the
> buffer_advance() helper method instead of directly manipulating the
> offset value.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  ui/vnc-auth-sasl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1] ui: correctly advance output buffer when writing SASL data

[Laszlo Ersek]

On 02/01/18 16:58, Daniel P. Berrangé wrote:
> In this previous commit:
> 
>   commit 8f61f1c5a6bc06438a1172efa80bc7606594fa07
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Mon Dec 18 19:12:20 2017 +0000
> 
>     ui: track how much decoded data we consumed when doing SASL encoding
> 
> I attempted to fix a flaw with tracking how much data had actually been
> processed when encoding with SASL. With that flaw, the VNC server could
> mistakenly discard queued data that had not been sent.
> 
> The fix was not quite right though, because it merely decremented the
> vs->output.offset value. This is effectively to discarding data from the
> end of the pending output buffer. We actually need to discard data from
> the start of the pending output buffer. We also want to free memory that
> is no longer required. The correct way to handle this is to use the
> buffer_advance() helper method instead of directly manipulating the
> offset value.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  ui/vnc-auth-sasl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index 74a5f513f2..fbccca8c8a 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -84,7 +84,7 @@ size_t vnc_client_write_sasl(VncState *vs)
>          } else {
>              vs->force_update_offset -= vs->sasl.encodedRawLength;
>          }
> -        vs->output.offset -= vs->sasl.encodedRawLength;
> +        buffer_advance(&vs->output, vs->sasl.encodedRawLength);
>          vs->sasl.encoded = NULL;
>          vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;
>      }
> 

With the typo pointed out by Eric fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thank you!
Laszlo

Re: [Qemu-devel] [PATCH v1] ui: correctly advance output buffer when writing SASL data

[Gerd Hoffmann]

On Thu, Feb 01, 2018 at 10:03:23AM -0600, Eric Blake wrote:
> On 02/01/2018 09:58 AM, Daniel P. Berrangé wrote:
> > In this previous commit:
> > 
> >   commit 8f61f1c5a6bc06438a1172efa80bc7606594fa07
> >   Author: Daniel P. Berrange <berrange@redhat.com>
> >   Date:   Mon Dec 18 19:12:20 2017 +0000
> > 
> >     ui: track how much decoded data we consumed when doing SASL encoding
> > 
> > I attempted to fix a flaw with tracking how much data had actually been
> > processed when encoding with SASL. With that flaw, the VNC server could
> > mistakenly discard queued data that had not been sent.
> > 
> > The fix was not quite right though, because it merely decremented the
> > vs->output.offset value. This is effectively to discarding data from the
> 
> s/to //

Patch queued, fix applied.

thanks,
  Gerd