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

[Javi Merino <javi.merino@xxxxxxxxx>]

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