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@invisiblethingslab.com>
---
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?

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.
---
 tools/xl/xl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index ec72ca60c32a..e183d42b1d65 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -81,6 +81,8 @@ static int auto_autoballoon(void)
     info = libxl_get_version_info(ctx);
     if (!info)
         return 1; /* default to on */
+    if (!info->commandline)
+        return 1;
 
 #define SIZE_PATTERN "-?[0-9]+[bBkKmMgGtT]?"
 
-- 
2.49.0

On Mon, Jul 28, 2025 at 12:24:03PM +0200, 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@invisiblethingslab.com>
> ---
> 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?
> 
> 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.
> ---
>  tools/xl/xl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index ec72ca60c32a..e183d42b1d65 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -81,6 +81,8 @@ static int auto_autoballoon(void)
>      info = libxl_get_version_info(ctx);
>      if (!info)
>          return 1; /* default to on */
> +    if (!info->commandline)
> +        return 1;

It's a nit, but could you join with the previous if condition, so that
the comment also applies?

     if (!info || !info->commandline)
         return 1; /* default to on */

Thanks, Roger.

On Mon, Jul 28, 2025 at 02:11:16PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 28, 2025 at 12:24:03PM +0200, 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@invisiblethingslab.com>
> > ---
> >  tools/xl/xl.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> > index ec72ca60c32a..e183d42b1d65 100644
> > --- a/tools/xl/xl.c
> > +++ b/tools/xl/xl.c
> > @@ -81,6 +81,8 @@ static int auto_autoballoon(void)
> >      info = libxl_get_version_info(ctx);
> >      if (!info)
> >          return 1; /* default to on */
> > +    if (!info->commandline)
> > +        return 1;
> 
> It's a nit, but could you join with the previous if condition, so that
> the comment also applies?
> 
>      if (!info || !info->commandline)
>          return 1; /* default to on */

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

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@invisiblethingslab.com>
> ---
> 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.

> 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

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@invisiblethingslab.com>
>> ---
>> 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.

Regards,
Jason

> 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.
>

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@invisiblethingslab.com>
>>> ---
>>> 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

On Mon, Jul 28, 2025 at 11:46 AM Andrew Cooper
<andrew.cooper3@citrix.com> 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@invisiblethingslab.com>
> > ---
> > 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@cloud.com>

Frediano

On 28.07.2025 12:50, Frediano Ziglio wrote:
> On Mon, Jul 28, 2025 at 11:46 AM Andrew Cooper
> <andrew.cooper3@citrix.com> 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@invisiblethingslab.com>
>>> ---
>>> 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.

Except that such behavior isn't mandated by the C99 spec, afaics. In
fact, strict interpretation of the language there ("the argument shall
be a pointer to the initial element of an array of character type" and
"the argument shall be a pointer to the initial element of an array of
wchar_t type") precludes passing in NULL.

Jan