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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Aug 23, 2024 at 10:34:05AM +0100, Andrew Cooper wrote:
> On 22/08/2024 2:13 pm, Javi Merino wrote:
> This looks like it will fix the ASAN issue, but I think a better fix
> would be:
>
> -        printf("%s", line);
> +       printf("%.*s", cr->count, line);

nul-terminated string are safer to manipulate, and cr->count isn't
available outside of libxl, so that's a no go.

> because otherwise there's a world of sharp corners once Xen has wrapped
> the buffer for the first time.

If wrapped buffer are visible to libxl or `xl dmesg`, that a bug in xen,
in XEN_SYSCTL_readconsole. It doesn't looks like it's possible to know
when the console ring buffer wrapped.

>
> Which brings us a lot of other WTFs in this code...
>
> First, libxl_console.c describes it's functionality in terms of lines,
> and line_reader() in the API.  Yet it's not lines, it's a 16k buffer
> with generally multi-line content.  It's too late to fix the naming, but
> we could at least rewrite the comments not to be blatant lies.
>
>
> Just out of context above the hunk is:
>
>     unsigned int size = 16384;
>
> which isn't accurate.  The size of Xen's console ring can be changed at
> compile time (like XenServer does), and/or by command line variable.

This buffer size isn't link to Xen's console ring size, both value don't
need to match.

> Because the dmesg ring is never full (it just keeps producing and
> overwriting tail data), it's impossible to get a clean copy except in a
> single hypercall; the incremental/offset parameters are an illusion, and
> do not function correctly if a new printk() has occurred between
> adjacent hypercalls.

Well, let's hope there's no more that "ring_size - size" is been written
to the ring in between two hypercall. There's doesn't seems to be
anything that can be done to work around this with this hypercall
(beside using a huge buffer size).

I think xenconsoled can read the console ring buffer, but does so
continuously, so if `xl dmesg` have some shortcoming, it might be fine.

> And that is why setting count to size - 1 probably isn't wise.  It means
> that, even in the ideal case where Xen's ringbuffer is 16k, you've still
> got to make 2 hypercalls to get the content.

Well, I've run `xl dmesg` on one machine, it seems to have taken 9
hypercall to get everything, and there's still the first few line of the
boot available. So if the ringbuffer was indeed 16k, it would not be
ideal at all... as lots of stuff would have been overwritten.

Cheers,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech