Re: [PATCH] tools/xl: don't crash on NULL command line

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/07/2025 1:29 am, Jason Andryuk wrote:
> On 2025-07-28 06:45, Andrew Cooper wrote:
>> On 28/07/2025 11:24 am, Marek Marczykowski-Górecki wrote:
>>> When running xl in a domU, it doesn't have access to the Xen command
>>> line. Before the non-truncating xc_xenver_cmdline(), it was always set
>>> with strdup, possibly of an empty string. Now it's NULL. Treat it the
>>> same as empty cmdline, as it was before. Autoballoon isn't relevant for
>>> xl devd in a domU anyway.
>>>
>>> Fixes: 75f91607621c ("tools: Introduce a non-truncating
>>> xc_xenver_cmdline()")
>>> Signed-off-by: Marek Marczykowski-Górecki
>>> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> So, apparently the "No API/ABI change" was a lie... it changed "empty
>>> string" to NULL in libxl_version_info->commandline. Quick search didn't
>>> spot any other (in-tree) place that could trip on NULL there. IMO NULL
>>> value in this case makes more sense. Buf maybe for the API stability
>>> reasons the old behavior should be restored?
>>
>> Hmm, I didn't intend to change things, but I also didn't anticipate
>> libxl__strdup()'s behaviour, or for something to depend on that.
>
> I think it isn't strdup()'s behaviour, but rather the old code:
>
> -    xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
> -    info->commandline = libxl__strdup(NOGC, u.xen_commandline);
> +    info->commandline = xc_xenver_commandline(ctx->xch);
>
> No error checking on xc_version(), so strdup() is duplicating whatever
> (stale?) data may be in the union.

Even better...

xc_version(XENVER_commandline) for better or worse couldn't fail in Xen
for anything other than -EFAULT (writing a 1k block of memory), but a
systematic failing with the old ABIs was that nothing caused Xen to
explicitly NUL-terminate the string.

Notice how XENVER_commandline operates on ARRAY_SIZE(saved_cmdline) and
not strlen().

I'll give you 0 guesses what happens when the bootloader passed a
cmdline of >1k, and also 0 guesses for how we stumbled onto this mess.

~Andrew