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 <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
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
