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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Jul 28, 2025 at 11:46 AM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> 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.
>
> While this does turn out to be a marginal API change, I think your
> solution is the right one.  I do not think it's reasonable for there to
> be one magic pointer that has differing NULL-ness to the others, and
> NULL for "no information" is the better interface.
>
> That said, is the other use fully safe?  I can't see anything that
> requires sprintf()'s %s to detect a NULL pointer and not crash.
>

That's the standard behavior, usually "(null)" is printed.

> > PS I'm working on a CI test for this case (and driver domains in
> > general). I have it working with Alpine already, but it wouldn't detect
> > this issue, as musl's regexec() doesn't crash on NULL... So, I'll add a
> > test on Debian too.
>
> Excellent.
>
> ~Andrew
>

For the commit

Reviewed-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

Frediano