[XEN PATCH v2 0/3] libxl: Fixes for libxl_xenconsole_readline()

[XEN PATCH v2 0/3] libxl: Fixes for libxl_xenconsole_readline()

[Javi Merino]

Fix nul-termination of the return value of
libxl_xen_console_read_line().  While at it, remove unneeded memset()s
to the buffer and improve the documentation of the function.

Changes since v1[0]:
  - Add Fixes: line to the first patch
  - Remove cr->count from the struct and make it a local variable in
    libxl_xen_console_read_line()
  - Improve the documentation of libxl_xen_console_read_line()

[0] https://lore.kernel.org/xen-devel/ad7c89bbae34155566ae7c9ca2cb501f21c7d585.1724330921.git.javi.merino@cloud.com/

Javi Merino (3):
  libxl: Fix nul-termination of the return value of
    libxl_xen_console_read_line()
  libxl: Remove unnecessary buffer zeroing and zalloc()
  libxl: Update the documentation of libxl_xen_console_read_line()

 tools/libs/light/libxl_console.c  | 40 ++++++++++++++++++-------------
 tools/libs/light/libxl_internal.h |  1 -
 2 files changed, 24 insertions(+), 17 deletions(-)

-- 
2.45.2

[XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Javi Merino]

When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)"
call in main_dmesg().  ASAN reports a heap buffer overflow: an
off-by-one access to cr->buffer.

The readconsole sysctl copies up to count characters into the buffer,
but it does not add a null character at the end.  Despite the
documentation of libxl_xen_console_read_line(), line_r is not
nul-terminated if 16384 characters were copied to the buffer.

Fix this by asking xc_readconsolering() to fill the buffer up to size
- 1.  As the number of characters in the buffer is only needed in
libxl_xen_console_read_line(), make it a local variable there instead
of part of the libxl__xen_console_reader struct.

Fixes: 4024bae739cc ("xl: Add subcommand 'xl dmesg'")
Reported-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Javi Merino <javi.merino@cloud.com>
---
 tools/libs/light/libxl_console.c  | 14 ++++++++++----
 tools/libs/light/libxl_internal.h |  1 -
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index a563c9d3c7f9..012fd996fba9 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -774,12 +774,14 @@ libxl_xen_console_reader *
 {
     GC_INIT(ctx);
     libxl_xen_console_reader *cr;
-    unsigned int size = 16384;
+    /* We want xen to fill the buffer in as few hypercalls as
+     * possible, but xen will not nul-terminate it.  Leave one byte at
+     * the end for the null */
+    unsigned int size = 16384 + 1;
 
     cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader));
     cr->buffer = libxl__zalloc(NOGC, size);
     cr->size = size;
-    cr->count = size;
     cr->clear = clear;
     cr->incremental = 1;
 
@@ -800,10 +802,14 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
                                 char **line_r)
 {
     int ret;
+    /* number of chars to copy into the buffer.  xc_readconsolering()
+     * does not add a null character at the end, so leave a space for
+     * us to add it. */
+    unsigned int nr_chars = cr->size - 1;
     GC_INIT(ctx);
 
     memset(cr->buffer, 0, cr->size);
-    ret = xc_readconsolering(ctx->xch, cr->buffer, &cr->count,
+    ret = xc_readconsolering(ctx->xch, cr->buffer, &nr_chars,
                              cr->clear, cr->incremental, &cr->index);
     if (ret < 0) {
         LOGE(ERROR, "reading console ring buffer");
@@ -811,7 +817,7 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
         return ERROR_FAIL;
     }
     if (!ret) {
-        if (cr->count) {
+        if (nr_chars) {
             *line_r = cr->buffer;
             ret = 1;
         } else {
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 3b58bb2d7f43..96d14f5746e1 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2077,7 +2077,6 @@ _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 struct libxl__xen_console_reader {
     char *buffer;
     unsigned int size;
-    unsigned int count;
     unsigned int clear;
     unsigned int incremental;
     unsigned int index;
-- 
2.45.2

[XEN PATCH v2 2/3] libxl: Remove unnecessary buffer zeroing and zalloc()

[Javi Merino]

When reading the console, xen overwrites the contents of the buffer,
so there is no need to zero the buffer before passing it to xen.
Instead, add a NULL at the end of the buffer.

While we are at it, change the zalloc() of the buffer back to
malloc() as it was before bdf4131 (libxl: don't leak buf in
libxl_xen_console_read_start error handling, 2013-12-03).  The comment
in that commit message says that the intent of the commit was to
change malloc+memset to zalloc(), but only for the
libxl_xen_console_reader struct, not for the buffer.

Signed-off-by: Javi Merino <javi.merino@cloud.com>
---
 tools/libs/light/libxl_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index 012fd996fba9..f42f6a51ee6f 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -780,7 +780,7 @@ libxl_xen_console_reader *
     unsigned int size = 16384 + 1;
 
     cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader));
-    cr->buffer = libxl__zalloc(NOGC, size);
+    cr->buffer = libxl__malloc(NOGC, size);
     cr->size = size;
     cr->clear = clear;
     cr->incremental = 1;
@@ -808,7 +808,6 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
     unsigned int nr_chars = cr->size - 1;
     GC_INIT(ctx);
 
-    memset(cr->buffer, 0, cr->size);
     ret = xc_readconsolering(ctx->xch, cr->buffer, &nr_chars,
                              cr->clear, cr->incremental, &cr->index);
     if (ret < 0) {
@@ -818,6 +817,7 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
     }
     if (!ret) {
         if (nr_chars) {
+            cr->buffer[nr_chars] = '\0';
             *line_r = cr->buffer;
             ret = 1;
         } else {
-- 
2.45.2

[XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Javi Merino]

Despite its name, libxl_xen_console_read_line() does not read a line,
it fills the buffer with as many characters as fit.  Update the
documentation to reflect the real behaviour of the function.  Rename
line_r to avoid confusion since it is a pointer to an array of
characters.

Signed-off-by: Javi Merino <javi.merino@cloud.com>
---
 tools/libs/light/libxl_console.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index f42f6a51ee6f..652897e4ef6d 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -789,17 +789,19 @@ libxl_xen_console_reader *
     return cr;
 }
 
-/* return values:                                          *line_r
- *   1          success, whole line obtained from buffer    non-0
- *   0          no more lines available right now           0
- *   negative   error code ERROR_*                          0
- * On success *line_r is updated to point to a nul-terminated
+/* Copy part of the console ring into a buffer
+ *
+ * Return values:
+ *   1: Success, the buffer obtained from the console ring an
+ *   0: No more lines available right now
+ *   -ERROR_* on error
+ *
+ * On success, *line_r is updated to point to a nul-terminated
  * string which is valid until the next call on the same console
- * reader.  The libxl caller may overwrite parts of the string
- * if it wishes. */
+ * reader. */
 int libxl_xen_console_read_line(libxl_ctx *ctx,
                                 libxl_xen_console_reader *cr,
-                                char **line_r)
+                                char **buff)
 {
     int ret;
     /* number of chars to copy into the buffer.  xc_readconsolering()
@@ -818,10 +820,10 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
     if (!ret) {
         if (nr_chars) {
             cr->buffer[nr_chars] = '\0';
-            *line_r = cr->buffer;
+            *buff = cr->buffer;
             ret = 1;
         } else {
-            *line_r = NULL;
+            *buff = NULL;
             ret = 0;
         }
     }
-- 
2.45.2

Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Andrew Cooper]

On 23/08/2024 6:05 pm, Javi Merino wrote:
> diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> index f42f6a51ee6f..652897e4ef6d 100644
> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -789,17 +789,19 @@ libxl_xen_console_reader *
>      return cr;
>  }
>  
> -/* return values:                                          *line_r
> - *   1          success, whole line obtained from buffer    non-0
> - *   0          no more lines available right now           0
> - *   negative   error code ERROR_*                          0
> - * On success *line_r is updated to point to a nul-terminated
> +/* Copy part of the console ring into a buffer
> + *
> + * Return values:
> + *   1: Success, the buffer obtained from the console ring an
> + *   0: No more lines available right now
> + *   -ERROR_* on error
> + *
> + * On success, *line_r is updated to point to a nul-terminated

*buff.

Also this really wants to say somewhere "despite the function name,
there can be multiple lines in the returned buffer" or something to this
effect.

>   * string which is valid until the next call on the same console
> - * reader.  The libxl caller may overwrite parts of the string
> - * if it wishes. */
> + * reader. */

While I don't want to derail this patch further, what happens if there
happens to be an embedded \0 in Xen's console?  The hypercall is works
by a count of chars, and AFACIT, in this function we're assuming that
there's no \0 earlier in the string.

~Andrew

Re: [XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Anthony PERARD]

On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote:
> diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> index a563c9d3c7f9..012fd996fba9 100644
> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -774,12 +774,14 @@ libxl_xen_console_reader *
>  {
>      GC_INIT(ctx);
>      libxl_xen_console_reader *cr;
> -    unsigned int size = 16384;
> +    /* We want xen to fill the buffer in as few hypercalls as
> +     * possible, but xen will not nul-terminate it.  Leave one byte at
> +     * the end for the null */
> +    unsigned int size = 16384 + 1;

This comment doesn't really explain why the size choosen is 16k+1, it
kind of explain the +1 but that's about it.

16k seems to be the initial size
    https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L110
But then, it is changed to depend on $(nproc) and loglevel
    https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1095
with 16k been the minimum it seems:
    https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1061

So, I think a useful comment here would reflect that 16k is default size
of Xen's console buffer.

Also, multi-line comments are normally expected to be with begin and end
markers on separated lines:
    /*
     * Comments.
     */


It be nice to fix both comments, but otherwise the patch looks good:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH v2 2/3] libxl: Remove unnecessary buffer zeroing and zalloc()

[Anthony PERARD]

On Fri, Aug 23, 2024 at 06:05:04PM +0100, Javi Merino wrote:
> When reading the console, xen overwrites the contents of the buffer,
> so there is no need to zero the buffer before passing it to xen.
> Instead, add a NULL at the end of the buffer.
> 
> While we are at it, change the zalloc() of the buffer back to
> malloc() as it was before bdf4131 (libxl: don't leak buf in
> libxl_xen_console_read_start error handling, 2013-12-03).  The comment
> in that commit message says that the intent of the commit was to
> change malloc+memset to zalloc(), but only for the
> libxl_xen_console_reader struct, not for the buffer.
> 
> Signed-off-by: Javi Merino <javi.merino@cloud.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Anthony PERARD]

On Fri, Aug 23, 2024 at 06:05:05PM +0100, Javi Merino wrote:
> Despite its name, libxl_xen_console_read_line() does not read a line,
> it fills the buffer with as many characters as fit.  Update the
> documentation to reflect the real behaviour of the function.  Rename
> line_r to avoid confusion since it is a pointer to an array of
> characters.
> 
> Signed-off-by: Javi Merino <javi.merino@cloud.com>
> ---
>  tools/libs/light/libxl_console.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> index f42f6a51ee6f..652897e4ef6d 100644
> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -789,17 +789,19 @@ libxl_xen_console_reader *
>      return cr;
>  }
>  
> -/* return values:                                          *line_r
> - *   1          success, whole line obtained from buffer    non-0
> - *   0          no more lines available right now           0
> - *   negative   error code ERROR_*                          0
> - * On success *line_r is updated to point to a nul-terminated
> +/* Copy part of the console ring into a buffer
> + *
> + * Return values:
> + *   1: Success, the buffer obtained from the console ring an
> + *   0: No more lines available right now
> + *   -ERROR_* on error
> + *
> + * On success, *line_r is updated to point to a nul-terminated
>   * string which is valid until the next call on the same console
> - * reader.  The libxl caller may overwrite parts of the string
> - * if it wishes. */
> + * reader. */
>  int libxl_xen_console_read_line(libxl_ctx *ctx,
>                                  libxl_xen_console_reader *cr,
> -                                char **line_r)
> +                                char **buff)

You may want to update "tools/include/libxl.h" as well. I don't know why
this comments is here instead of in the public header. At least there's
some documentation I guess.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Roger Pau Monné]

On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote:
> When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)"
> call in main_dmesg().  ASAN reports a heap buffer overflow: an
> off-by-one access to cr->buffer.
> 
> The readconsole sysctl copies up to count characters into the buffer,
> but it does not add a null character at the end.  Despite the
> documentation of libxl_xen_console_read_line(), line_r is not
> nul-terminated if 16384 characters were copied to the buffer.
> 
> Fix this by asking xc_readconsolering() to fill the buffer up to size
> - 1.  As the number of characters in the buffer is only needed in
> libxl_xen_console_read_line(), make it a local variable there instead
> of part of the libxl__xen_console_reader struct.

Instead of playing games with the buffer size in order to add an extra
NUL character, we could possibly use libxl_write_exactly(ctx,
STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer
length?

Regards, Roger.

Re: [XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Andrew Cooper]

On 29/08/2024 4:53 pm, Roger Pau Monné wrote:
> On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote:
>> When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)"
>> call in main_dmesg().  ASAN reports a heap buffer overflow: an
>> off-by-one access to cr->buffer.
>>
>> The readconsole sysctl copies up to count characters into the buffer,
>> but it does not add a null character at the end.  Despite the
>> documentation of libxl_xen_console_read_line(), line_r is not
>> nul-terminated if 16384 characters were copied to the buffer.
>>
>> Fix this by asking xc_readconsolering() to fill the buffer up to size
>> - 1.  As the number of characters in the buffer is only needed in
>> libxl_xen_console_read_line(), make it a local variable there instead
>> of part of the libxl__xen_console_reader struct.
> Instead of playing games with the buffer size in order to add an extra
> NUL character, we could possibly use libxl_write_exactly(ctx,
> STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer
> length?

Sadly no.

struct libxl__xen_console_reader (which has the count field) is a libxl
private (opaque) type which `xl` can't access.

Otherwise this would be a oneline fix already...

~Andrew

Re: [XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Roger Pau Monné]

On Fri, Aug 30, 2024 at 08:22:29PM +0100, Andrew Cooper wrote:
> On 29/08/2024 4:53 pm, Roger Pau Monné wrote:
> > On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote:
> >> When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)"
> >> call in main_dmesg().  ASAN reports a heap buffer overflow: an
> >> off-by-one access to cr->buffer.
> >>
> >> The readconsole sysctl copies up to count characters into the buffer,
> >> but it does not add a null character at the end.  Despite the
> >> documentation of libxl_xen_console_read_line(), line_r is not
> >> nul-terminated if 16384 characters were copied to the buffer.
> >>
> >> Fix this by asking xc_readconsolering() to fill the buffer up to size
> >> - 1.  As the number of characters in the buffer is only needed in
> >> libxl_xen_console_read_line(), make it a local variable there instead
> >> of part of the libxl__xen_console_reader struct.
> > Instead of playing games with the buffer size in order to add an extra
> > NUL character, we could possibly use libxl_write_exactly(ctx,
> > STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer
> > length?
> 
> Sadly no.
> 
> struct libxl__xen_console_reader (which has the count field) is a libxl
> private (opaque) type which `xl` can't access.

I was fooled by the libxl_xen_console_reader typedef.

> Otherwise this would be a oneline fix already...

Hm, maybe the easiest way is to introduce a new function that returns
the buffer and the number of characters?
(libxl_xen_console_dump_buffer() or similar?)

Thanks, Roger.

Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Alejandro Vallejo]

On Fri Aug 23, 2024 at 6:05 PM BST, Javi Merino wrote:
> Despite its name, libxl_xen_console_read_line() does not read a line,
> it fills the buffer with as many characters as fit.  Update the
> documentation to reflect the real behaviour of the function.  Rename
> line_r to avoid confusion since it is a pointer to an array of
> characters.
>
> Signed-off-by: Javi Merino <javi.merino@cloud.com>
> ---
>  tools/libs/light/libxl_console.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> index f42f6a51ee6f..652897e4ef6d 100644
> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -789,17 +789,19 @@ libxl_xen_console_reader *
>      return cr;
>  }
>  
> -/* return values:                                          *line_r
> - *   1          success, whole line obtained from buffer    non-0
> - *   0          no more lines available right now           0
> - *   negative   error code ERROR_*                          0
> - * On success *line_r is updated to point to a nul-terminated
> +/* Copy part of the console ring into a buffer
> + *
> + * Return values:
> + *   1: Success, the buffer obtained from the console ring an

Seems like this line in the comment is incomplete?

> + *   0: No more lines available right now
> + *   -ERROR_* on error
> + *
> + * On success, *line_r is updated to point to a nul-terminated
>   * string which is valid until the next call on the same console
> - * reader.  The libxl caller may overwrite parts of the string
> - * if it wishes. */
> + * reader. */

Cheers,
Alejandro

Re: [XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Javi Merino]

On Tue, Aug 27, 2024 at 03:20:10PM GMT, Anthony PERARD wrote:
> On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote:
> > diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> > index a563c9d3c7f9..012fd996fba9 100644
> > --- a/tools/libs/light/libxl_console.c
> > +++ b/tools/libs/light/libxl_console.c
> > @@ -774,12 +774,14 @@ libxl_xen_console_reader *
> >  {
> >      GC_INIT(ctx);
> >      libxl_xen_console_reader *cr;
> > -    unsigned int size = 16384;
> > +    /* We want xen to fill the buffer in as few hypercalls as
> > +     * possible, but xen will not nul-terminate it.  Leave one byte at
> > +     * the end for the null */
> > +    unsigned int size = 16384 + 1;
> 
> This comment doesn't really explain why the size choosen is 16k+1, it
> kind of explain the +1 but that's about it.
> 
> 16k seems to be the initial size
>     https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L110
> But then, it is changed to depend on $(nproc) and loglevel
>     https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1095
> with 16k been the minimum it seems:
>     https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1061
> 
> So, I think a useful comment here would reflect that 16k is default size
> of Xen's console buffer.

Ok, I'll add a line that explains this.

> Also, multi-line comments are normally expected to be with begin and end
> markers on separated lines:
>     /*
>      * Comments.
>      */

In this file, there were more comments that didn't have the end marker
in a separate line, so I was just trying to be consistent.  For
example:

- https://elixir.bootlin.com/xen/v4.19.0/source/tools/libs/light/libxl_console.c#L454
- https://elixir.bootlin.com/xen/v4.19.0/source/tools/libs/light/libxl_console.c#L752
  (this one is mixed actually)
- https://elixir.bootlin.com/xen/v4.19.0/source/tools/libs/light/libxl_console.c#L790

Sure, I will make all comments I introduce have an end marker on a separate line

> It be nice to fix both comments, but otherwise the patch looks good:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks for the review!
Javi

Re: [XEN PATCH v2 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Javi Merino]

On Mon, Sep 02, 2024 at 09:50:28AM GMT, Roger Pau Monné wrote:
> On Fri, Aug 30, 2024 at 08:22:29PM +0100, Andrew Cooper wrote:
> > On 29/08/2024 4:53 pm, Roger Pau Monné wrote:
> > > On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote:
> > >> When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)"
> > >> call in main_dmesg().  ASAN reports a heap buffer overflow: an
> > >> off-by-one access to cr->buffer.
> > >>
> > >> The readconsole sysctl copies up to count characters into the buffer,
> > >> but it does not add a null character at the end.  Despite the
> > >> documentation of libxl_xen_console_read_line(), line_r is not
> > >> nul-terminated if 16384 characters were copied to the buffer.
> > >>
> > >> Fix this by asking xc_readconsolering() to fill the buffer up to size
> > >> - 1.  As the number of characters in the buffer is only needed in
> > >> libxl_xen_console_read_line(), make it a local variable there instead
> > >> of part of the libxl__xen_console_reader struct.
> > > Instead of playing games with the buffer size in order to add an extra
> > > NUL character, we could possibly use libxl_write_exactly(ctx,
> > > STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer
> > > length?
> > 
> > Sadly no.
> > 
> > struct libxl__xen_console_reader (which has the count field) is a libxl
> > private (opaque) type which `xl` can't access.
> 
> I was fooled by the libxl_xen_console_reader typedef.
> 
> > Otherwise this would be a oneline fix already...
> 
> Hm, maybe the easiest way is to introduce a new function that returns
> the buffer and the number of characters?
> (libxl_xen_console_dump_buffer() or similar?)

Even if we did that, this function needs to be fixed as it are part of
the library and doesn't do what its documentation say: return a
nul-terminated string.

Personally, I would introduce a function as you suggest and call this
interface deprecated.  But I think it is overkill in this case, as
this is just `xl dmesg`.

Cheers,
Javi

Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Javi Merino]

On Mon, Sep 02, 2024 at 11:14:11AM GMT, Alejandro Vallejo wrote:
> On Fri Aug 23, 2024 at 6:05 PM BST, Javi Merino wrote:
> > Despite its name, libxl_xen_console_read_line() does not read a line,
> > it fills the buffer with as many characters as fit.  Update the
> > documentation to reflect the real behaviour of the function.  Rename
> > line_r to avoid confusion since it is a pointer to an array of
> > characters.
> >
> > Signed-off-by: Javi Merino <javi.merino@cloud.com>
> > ---
> >  tools/libs/light/libxl_console.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> > index f42f6a51ee6f..652897e4ef6d 100644
> > --- a/tools/libs/light/libxl_console.c
> > +++ b/tools/libs/light/libxl_console.c
> > @@ -789,17 +789,19 @@ libxl_xen_console_reader *
> >      return cr;
> >  }
> >  
> > -/* return values:                                          *line_r
> > - *   1          success, whole line obtained from buffer    non-0
> > - *   0          no more lines available right now           0
> > - *   negative   error code ERROR_*                          0
> > - * On success *line_r is updated to point to a nul-terminated
> > +/* Copy part of the console ring into a buffer
> > + *
> > + * Return values:
> > + *   1: Success, the buffer obtained from the console ring an
> 
> Seems like this line in the comment is incomplete?

Indeed.  Fixed to say:

    1: Success, *buff points to the string

Thanks,
Javi

Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Javi Merino]

On Fri, Aug 23, 2024 at 06:31:50PM GMT, Andrew Cooper wrote:
> On 23/08/2024 6:05 pm, Javi Merino wrote:
> > diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> > index f42f6a51ee6f..652897e4ef6d 100644
> > --- a/tools/libs/light/libxl_console.c
> > +++ b/tools/libs/light/libxl_console.c
> > @@ -789,17 +789,19 @@ libxl_xen_console_reader *
> >      return cr;
> >  }
> >  
> > -/* return values:                                          *line_r
> > - *   1          success, whole line obtained from buffer    non-0
> > - *   0          no more lines available right now           0
> > - *   negative   error code ERROR_*                          0
> > - * On success *line_r is updated to point to a nul-terminated
> > +/* Copy part of the console ring into a buffer
> > + *
> > + * Return values:
> > + *   1: Success, the buffer obtained from the console ring an
> > + *   0: No more lines available right now
> > + *   -ERROR_* on error
> > + *
> > + * On success, *line_r is updated to point to a nul-terminated
> 
> *buff.

Indeed.

> Also this really wants to say somewhere "despite the function name,
> there can be multiple lines in the returned buffer" or something to this
> effect.

Sure.  I had only written it in the commit message.  I have added it
to the documentation now.

> >   * string which is valid until the next call on the same console
> > - * reader.  The libxl caller may overwrite parts of the string
> > - * if it wishes. */
> > + * reader. */
> 
> While I don't want to derail this patch further, what happens if there
> happens to be an embedded \0 in Xen's console?  The hypercall is works
> by a count of chars, and AFACIT, in this function we're assuming that
> there's no \0 earlier in the string.

True.  This would have truncated the buffer before, and it still does
now.  libxl_xen_console_read_line() is the only one that knows the
size of the buffer and can't pass this information down to the caller,
so one way to fix this would be to scan the buffer for '\0' and
replace it with another character.

I'd rather do this in another patch by itself once this series is merged.


Cheers,
Javi

Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Javi Merino]

On Tue, Aug 27, 2024 at 03:26:09PM GMT, Anthony PERARD wrote:
> On Fri, Aug 23, 2024 at 06:05:05PM +0100, Javi Merino wrote:
> > Despite its name, libxl_xen_console_read_line() does not read a line,
> > it fills the buffer with as many characters as fit.  Update the
> > documentation to reflect the real behaviour of the function.  Rename
> > line_r to avoid confusion since it is a pointer to an array of
> > characters.
> > 
> > Signed-off-by: Javi Merino <javi.merino@cloud.com>
> > ---
> >  tools/libs/light/libxl_console.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
> > index f42f6a51ee6f..652897e4ef6d 100644
> > --- a/tools/libs/light/libxl_console.c
> > +++ b/tools/libs/light/libxl_console.c
> > @@ -789,17 +789,19 @@ libxl_xen_console_reader *
> >      return cr;
> >  }
> >  
> > -/* return values:                                          *line_r
> > - *   1          success, whole line obtained from buffer    non-0
> > - *   0          no more lines available right now           0
> > - *   negative   error code ERROR_*                          0
> > - * On success *line_r is updated to point to a nul-terminated
> > +/* Copy part of the console ring into a buffer
> > + *
> > + * Return values:
> > + *   1: Success, the buffer obtained from the console ring an
> > + *   0: No more lines available right now
> > + *   -ERROR_* on error
> > + *
> > + * On success, *line_r is updated to point to a nul-terminated
> >   * string which is valid until the next call on the same console
> > - * reader.  The libxl caller may overwrite parts of the string
> > - * if it wishes. */
> > + * reader. */
> >  int libxl_xen_console_read_line(libxl_ctx *ctx,
> >                                  libxl_xen_console_reader *cr,
> > -                                char **line_r)
> > +                                char **buff)
> 
> You may want to update "tools/include/libxl.h" as well.

Sure, I will change this in "tools/include/libxl.h".

> I don't know why
> this comments is here instead of in the public header. At least there's
> some documentation I guess.

Cheers,
Javi