Re: [XEN PATCH v1] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.08.2024 15:13, 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 making count one less that the size of the allocated
> buffer so that the last byte is always null.
> 
> Reported-by: Edwin Török <edwin.torok@xxxxxxxxx>
> Signed-off-by: Javi Merino <javi.merino@xxxxxxxxx>

Perhaps wants a Fixes: tag as well?

> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -779,7 +779,7 @@ libxl_xen_console_reader *
>      cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader));
>      cr->buffer = libxl__zalloc(NOGC, size);
>      cr->size = size;
> -    cr->count = size;
> +    cr->count = size - 1;
>      cr->clear = clear;
>      cr->incremental = 1;

While this looks to be addressing the issue at hand, I still wonder: Why
does the "count" field exist at all? It's certainly odd to be set right
here (when the buffer actually is empty). It's used solely in
libxl_xen_console_read_line(), so could be a local variable there.

Then, further, I wonder why struct libxl__xen_console_reader lives in
libxl_internal.h - it's used solely in libxl_console.c.

Finally - why libxl__zalloc() when libxl_xen_console_read_line() clears
the buffer anyway?

Jan