Re: [Libguestfs] [libnbd PATCH] info: Try harder for graceful disconnect from server
On Fri, Sep 22, 2023 at 09:47:55PM +0100, Richard W.M. Jones wrote:
> On Fri, Sep 22, 2023 at 12:33:36PM -0500, Eric Blake wrote:
> > There are a number of ways in which gathering information can fail.
> > But when possible, it makes sense to let the server know that we are
> > done talking, as it minimizes the likelihood that nbdinfo's error
> > message will be obscured by an accompanying error message by the
> > server complaining about an unclean disconnect.
> >
> > For example, with a one-off qemu temporarily patched as mentioned in
> > commit 0f8ee8c6 to advertise sizes larger than 2^63, kicking off
> > 'qemu-nbd -f raw -r file &' produces:
> >
> > pre-patch:
> >
> > $ ./run nbdinfo --size nbd://localhost
> > /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> > 9223372036854781440 which does not fit in signed result: Value too large
> > for defined data type
> > qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
> > end-of-file before all data were read
>
> This doesn't necessarily seem like a bug?
It's a quality of service thing - qemu is just being noisy that the
client closed the socket abruptly without giving any reason why. It
doesn't change libnbd's behavior (nbdinfo has already reported its
error message), but may confuse people reading qemu-nbd logs.
It may also be a case where we could patch qemu-nbd to not output a
complaint if the client exited at a clean point in the back-and-forth
transactions. We still want to be noisy if the socket closes after
the first byte has been read, but if there are zero bytes available,
announcing that the client no longer cares does not add much value.
>
> This feels like quite a significant change in behaviour to me. In
> particular I'm worried if it interacts in some subtle way with the
> forking done by the "[ ... ]" syntax for running servers on the
> command line (or any of the other ways that libnbd might fork/exec).
Interesting observation. atexit() handlers are not preserved across
exec, and I think all our fork() paths end in either exec or _exit
(also where atexit handlers are ignored), so I don't think we are
risking calling the handler twice.
>
> Can we hold this patch until after 1.18 is released and then put it
> in? Should only be a week or two.
Sure, being conservative on this one is fine by me. I'll delay it
until after 1.18.
>
> Provisionally ACKed for post-1.18
>
> Rich.
>
> > post-patch:
> > $ ./run nbdinfo --size nbd://localhost
> > /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> > 9223372036854781440 which does not fit in signed result: Value too large
> > for defined data type
> >
> > if nbdinfo is run in the same terminal as qemu-nbd.
> >
> > Signed-off-by: Eric Blake
> > ---
> > info/main.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/info/main.c b/info/main.c
> > index c8248c61..05f19f43 100644
> > --- a/info/main.c
> > +++ b/info/main.c
> > @@ -88,6 +88,17 @@ usage (FILE *fp, int exitcode)
> >exit (exitcode);
> > }
> >
> > +void
> > +clean_shutdown (void)
> > +{
> > + /* If we are connected but detect an error, try to give the server
> > + * notice that we are done talking. Ignore failures, as this is
> > + * only a courtesy measure.
> > + */
> > + if (nbd)
> > +nbd_shutdown (nbd, 0);
> > +}
> > +
> > int
> > main (int argc, char *argv[])
> > {
> > @@ -277,6 +288,7 @@ main (int argc, char *argv[])
> > fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> > exit (EXIT_FAILURE);
> >}
> > + atexit (clean_shutdown);
> >nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
> >
> >/* Set optional modes in the handle. */
> > @@ -353,6 +365,7 @@ main (int argc, char *argv[])
> >free_exports ();
> >nbd_shutdown (nbd, 0);
> >nbd_close (nbd);
> > + nbd = NULL;
> >
> >/* Close the output stream and copy it to the real stdout. */
> >if (fclose (fp) == EOF) {
> > --
> > 2.41.0
> >
>
>
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-top is 'top' for virtual machines. Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://people.redhat.com/~rjones/virt-top
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] info: Try harder for graceful disconnect from server
On Fri, Sep 22, 2023 at 12:33:36PM -0500, Eric Blake wrote:
> There are a number of ways in which gathering information can fail.
> But when possible, it makes sense to let the server know that we are
> done talking, as it minimizes the likelihood that nbdinfo's error
> message will be obscured by an accompanying error message by the
> server complaining about an unclean disconnect.
>
> For example, with a one-off qemu temporarily patched as mentioned in
> commit 0f8ee8c6 to advertise sizes larger than 2^63, kicking off
> 'qemu-nbd -f raw -r file &' produces:
>
> pre-patch:
>
> $ ./run nbdinfo --size nbd://localhost
> /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> 9223372036854781440 which does not fit in signed result: Value too large for
> defined data type
> qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
> end-of-file before all data were read
This doesn't necessarily seem like a bug?
This feels like quite a significant change in behaviour to me. In
particular I'm worried if it interacts in some subtle way with the
forking done by the "[ ... ]" syntax for running servers on the
command line (or any of the other ways that libnbd might fork/exec).
Can we hold this patch until after 1.18 is released and then put it
in? Should only be a week or two.
Provisionally ACKed for post-1.18
Rich.
> post-patch:
> $ ./run nbdinfo --size nbd://localhost
> /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> 9223372036854781440 which does not fit in signed result: Value too large for
> defined data type
>
> if nbdinfo is run in the same terminal as qemu-nbd.
>
> Signed-off-by: Eric Blake
> ---
> info/main.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/info/main.c b/info/main.c
> index c8248c61..05f19f43 100644
> --- a/info/main.c
> +++ b/info/main.c
> @@ -88,6 +88,17 @@ usage (FILE *fp, int exitcode)
>exit (exitcode);
> }
>
> +void
> +clean_shutdown (void)
> +{
> + /* If we are connected but detect an error, try to give the server
> + * notice that we are done talking. Ignore failures, as this is
> + * only a courtesy measure.
> + */
> + if (nbd)
> +nbd_shutdown (nbd, 0);
> +}
> +
> int
> main (int argc, char *argv[])
> {
> @@ -277,6 +288,7 @@ main (int argc, char *argv[])
> fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> exit (EXIT_FAILURE);
>}
> + atexit (clean_shutdown);
>nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
>
>/* Set optional modes in the handle. */
> @@ -353,6 +365,7 @@ main (int argc, char *argv[])
>free_exports ();
>nbd_shutdown (nbd, 0);
>nbd_close (nbd);
> + nbd = NULL;
>
>/* Close the output stream and copy it to the real stdout. */
>if (fclose (fp) == EOF) {
> --
> 2.41.0
>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH] info: Try harder for graceful disconnect from server
There are a number of ways in which gathering information can fail.
But when possible, it makes sense to let the server know that we are
done talking, as it minimizes the likelihood that nbdinfo's error
message will be obscured by an accompanying error message by the
server complaining about an unclean disconnect.
For example, with a one-off qemu temporarily patched as mentioned in
commit 0f8ee8c6 to advertise sizes larger than 2^63, kicking off
'qemu-nbd -f raw -r file &' produces:
pre-patch:
$ ./run nbdinfo --size nbd://localhost
/home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
9223372036854781440 which does not fit in signed result: Value too large for
defined data type
qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
end-of-file before all data were read
post-patch:
$ ./run nbdinfo --size nbd://localhost
/home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
9223372036854781440 which does not fit in signed result: Value too large for
defined data type
if nbdinfo is run in the same terminal as qemu-nbd.
Signed-off-by: Eric Blake
---
info/main.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/info/main.c b/info/main.c
index c8248c61..05f19f43 100644
--- a/info/main.c
+++ b/info/main.c
@@ -88,6 +88,17 @@ usage (FILE *fp, int exitcode)
exit (exitcode);
}
+void
+clean_shutdown (void)
+{
+ /* If we are connected but detect an error, try to give the server
+ * notice that we are done talking. Ignore failures, as this is
+ * only a courtesy measure.
+ */
+ if (nbd)
+nbd_shutdown (nbd, 0);
+}
+
int
main (int argc, char *argv[])
{
@@ -277,6 +288,7 @@ main (int argc, char *argv[])
fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
exit (EXIT_FAILURE);
}
+ atexit (clean_shutdown);
nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
/* Set optional modes in the handle. */
@@ -353,6 +365,7 @@ main (int argc, char *argv[])
free_exports ();
nbd_shutdown (nbd, 0);
nbd_close (nbd);
+ nbd = NULL;
/* Close the output stream and copy it to the real stdout. */
if (fclose (fp) == EOF) {
--
2.41.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 0/6] Fix fuzzer fallout
On Thu, Sep 21, 2023 at 03:57:59PM -0500, Eric Blake wrote: > Cat's out of the bag: Rich's fuzzer run found not one, but two > independent assertion failures that a malicious server could trigger > in my recent 64-bit extension code additions. What's more, in the > process of fixing them, we've discovered another long-standing issue > where nbd_get_size() returns confusing results compared to its > documentation, when talking to an odd server that reports a really > large export size. > > After off-list discussion between Rich, Laszlo, and myself, we didn't > think an embargoed CVE against libnbd is necessary (the assertion > failures only happen to unstable releases, and the nbd_get_size() > misbehavior does not happen with normal servers and has been in place > since v1.0, so it is nothing new), so I am posting the series now for > public review. But we will still be reaching out to secalert for > their opinion (it may be that they still see a low-priority exploit in > an app that gets confused when trying to use a negative size as a loop > bound, for example). Once they answer, and regardless of whether we > end up doing a libnbd CVE after all, I will follow up to the mailing > list with a security incident (client apps that demand a positive > export size should probably favor nbd_get_size()<0 over > nbd_get_size()==-1). > > Eric Blake (6): > states: Tweak comment in OPT_GO state handler > fuzzing: Disable client-side strictness checks > api: Sanitize sizes larger than INT64_MAX > block_status: Fix assertion with large server size > block_status: Fix assertion on bad 64-bit block status reply > info: Tolerate missing size After making a few edits based on the reviews, the series now in as adf32845..f8375d3c. I'll wait for an answer from secalert before posting a followup security advisory email. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On Fri, Sep 22, 2023 at 05:27:25PM +0200, Lee Garrett wrote: > On 22.09.23 16:47, Lee Garrett wrote: > >On 22.09.23 14:54, Richard W.M. Jones wrote: > >>On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote: > >>>On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote: > On 21.09.23 19:43, Richard W.M. Jones wrote: > >So this is probably another instance or variation of the timezone > >formatting problem (of schtasks). Which version of virt-v2v is this? > >I want to check that you have a version with all the latest patches in > >this area. > > It's 2.2.0-1 from Debian (12) bookworm. I've verified that it > doesn't have any distro-specific patches. > > (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian > would have a patches/series file in this case) > >>> > >>>The timezone fixes are: > >>> > >>>commit 597d177567234c3a539098c423649781424eeb6f > >>>Author: Laszlo Ersek > >>>Date: Tue Mar 8 15:30:51 2022 +0100 > >>> > >>> convert_windows: rewrite "configure_qemu_ga" script purely in > >>>PowerShell > >>> > >>>commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f > >>>Author: Richard W.M. Jones > >>>Date: Fri Nov 12 08:47:55 2021 + > >>> > >>> convert/convert_windows.ml: Handle date formats with dots instead of / > >>> > >>>They are all included in >= 2.0 > >>> > >>>I wonder if 597d177567 has a subtle flaw, or if we introduced a bug > >>>somewhere when refactoring this code later. > >>> > >>>Lee: Do you have a theory about exactly what is wrong with the > >>>schtasks date? Like what was it supposed to be, assuming it was 120 > >>>seconds in the future from boot time, versus what it was set to: > >>> > Firstboot-qemu-ga 9/21/2023 4:04:00 PM Ready > >>> > >>>Could a date or time field have not been swapped or been corrupted > >>>in some predictable way? > >> > >>Or in even simpler terms, what is the time (and timezone) that > >>this ^^^ machine was booted? > > > >I believe I have it figured out. > >The guest local time is currently 7:08 AM (a few minutes after > >firstboot/provisioning), pacific daylight time (UTC-7, though > >Windows displays it as "UTC-08:00"). This is the timezone that the > >guest comes configured with at first boot. The task is scheduled > >for 2:01 PM, meaning it's scheduled to run ~7 hours in the future. > > > >So it seems like the task was meant to be scheduled for 2:01 PM > >UTC (= 7:01 AM PDT), but for some reason was scheduled for 2:01 PM > >*local time*. > > > > From what I can see, the host machine time zone is irrelevant (UTC+2). > > > >I don't know where the timezone mixup comes from, though. Running > >`(get-date)` in the powershell at this point correctly returns the > >local time (7:08 AM). I guess during injection the time is in UTC, > >and schtasks.exe has no awareness of timezones? > > I digged a bit into the XML description and noticed this: > > > > To my knowledge Windows runs the BIOS clock in local time. So This > should probably be > > > > > > > > > as this is what libvirt uses in the preset for Windows 11 hosts. I > manually changed this before first boot, however it didn't solve the > issue. It only increased the offset to +9 hours in the future (7 > hours difference to UTC, +2 hours of local timezone). How does the > firstboot injection exactly happen? Is the guest actually booted for > it? No, the guest is not running. Rich. > > > >> > >>Rich. > >> > >>>The code we run is here: > >>> > >>>https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L571 > >>> > >>>Ming: this could be a bug affecting PST (UTC-8) guests, perhaps > >>>somehow related to having a single digit month field? > >>> > >>>Rich. > >>> > >>>-- > >>>Richard Jones, Virtualization Group, Red Hat > >>>http://people.redhat.com/~rjones > >>>Read my programming and virtualization blog: http://rwmj.wordpress.com > >>>libguestfs lets you edit virtual machines. Supports shell scripting, > >>>bindings from many languages. http://libguestfs.org > >> > > -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On 22.09.23 16:47, Lee Garrett wrote: On 22.09.23 14:54, Richard W.M. Jones wrote: On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote: On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote: On 21.09.23 19:43, Richard W.M. Jones wrote: So this is probably another instance or variation of the timezone formatting problem (of schtasks). Which version of virt-v2v is this? I want to check that you have a version with all the latest patches in this area. It's 2.2.0-1 from Debian (12) bookworm. I've verified that it doesn't have any distro-specific patches. (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian would have a patches/series file in this case) The timezone fixes are: commit 597d177567234c3a539098c423649781424eeb6f Author: Laszlo Ersek Date: Tue Mar 8 15:30:51 2022 +0100 convert_windows: rewrite "configure_qemu_ga" script purely in PowerShell commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f Author: Richard W.M. Jones Date: Fri Nov 12 08:47:55 2021 + convert/convert_windows.ml: Handle date formats with dots instead of / They are all included in >= 2.0 I wonder if 597d177567 has a subtle flaw, or if we introduced a bug somewhere when refactoring this code later. Lee: Do you have a theory about exactly what is wrong with the schtasks date? Like what was it supposed to be, assuming it was 120 seconds in the future from boot time, versus what it was set to: Firstboot-qemu-ga 9/21/2023 4:04:00 PM Ready Could a date or time field have not been swapped or been corrupted in some predictable way? Or in even simpler terms, what is the time (and timezone) that this ^^^ machine was booted? I believe I have it figured out. The guest local time is currently 7:08 AM (a few minutes after firstboot/provisioning), pacific daylight time (UTC-7, though Windows displays it as "UTC-08:00"). This is the timezone that the guest comes configured with at first boot. The task is scheduled for 2:01 PM, meaning it's scheduled to run ~7 hours in the future. So it seems like the task was meant to be scheduled for 2:01 PM UTC (= 7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local time*. From what I can see, the host machine time zone is irrelevant (UTC+2). I don't know where the timezone mixup comes from, though. Running `(get-date)` in the powershell at this point correctly returns the local time (7:08 AM). I guess during injection the time is in UTC, and schtasks.exe has no awareness of timezones? I digged a bit into the XML description and noticed this: To my knowledge Windows runs the BIOS clock in local time. So This should probably be as this is what libvirt uses in the preset for Windows 11 hosts. I manually changed this before first boot, however it didn't solve the issue. It only increased the offset to +9 hours in the future (7 hours difference to UTC, +2 hours of local timezone). How does the firstboot injection exactly happen? Is the guest actually booted for it? Rich. The code we run is here: https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L571 Ming: this could be a bug affecting PST (UTC-8) guests, perhaps somehow related to having a single digit month field? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On 22.09.23 14:54, Richard W.M. Jones wrote: On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote: On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote: On 21.09.23 19:43, Richard W.M. Jones wrote: So this is probably another instance or variation of the timezone formatting problem (of schtasks). Which version of virt-v2v is this? I want to check that you have a version with all the latest patches in this area. It's 2.2.0-1 from Debian (12) bookworm. I've verified that it doesn't have any distro-specific patches. (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian would have a patches/series file in this case) The timezone fixes are: commit 597d177567234c3a539098c423649781424eeb6f Author: Laszlo Ersek Date: Tue Mar 8 15:30:51 2022 +0100 convert_windows: rewrite "configure_qemu_ga" script purely in PowerShell commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f Author: Richard W.M. Jones Date: Fri Nov 12 08:47:55 2021 + convert/convert_windows.ml: Handle date formats with dots instead of / They are all included in >= 2.0 I wonder if 597d177567 has a subtle flaw, or if we introduced a bug somewhere when refactoring this code later. Lee: Do you have a theory about exactly what is wrong with the schtasks date? Like what was it supposed to be, assuming it was 120 seconds in the future from boot time, versus what it was set to: Firstboot-qemu-ga9/21/2023 4:04:00 PM Ready Could a date or time field have not been swapped or been corrupted in some predictable way? Or in even simpler terms, what is the time (and timezone) that this ^^^ machine was booted? I believe I have it figured out. The guest local time is currently 7:08 AM (a few minutes after firstboot/provisioning), pacific daylight time (UTC-7, though Windows displays it as "UTC-08:00"). This is the timezone that the guest comes configured with at first boot. The task is scheduled for 2:01 PM, meaning it's scheduled to run ~7 hours in the future. So it seems like the task was meant to be scheduled for 2:01 PM UTC (= 7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local time*. From what I can see, the host machine time zone is irrelevant (UTC+2). I don't know where the timezone mixup comes from, though. Running `(get-date)` in the powershell at this point correctly returns the local time (7:08 AM). I guess during injection the time is in UTC, and schtasks.exe has no awareness of timezones? Rich. The code we run is here: https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L571 Ming: this could be a bug affecting PST (UTC-8) guests, perhaps somehow related to having a single digit month field? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] nbdinfo default output [was: [libnbd PATCH v2 6/6] info: Tolerate missing size]
On Fri, Sep 22, 2023 at 09:03:53AM -0500, Eric Blake wrote: > > > $ ./run nbdinfo nbd://localhost > > > protocol: newstyle-fixed without TLS, using extended packets > > > ... > > > block_size_maximum: 33554432 For the human-readable output, should we also be using human_size() on the block_size_* values? I'd find: block_size_minimum: 1 block_size_preferred: 4096 (4K) block_size_maximum: 33554432 (32M) somewhat easier to read. I don't know if the JSON version would need to output that extra information, though. But that's a question independent of this patch series. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
On Fri, Sep 22, 2023 at 02:19:32PM +0200, Laszlo Ersek wrote:
> On 9/21/23 22:58, Eric Blake wrote:
> > As previous patches showed, the NBD spec does not yet forbid a server
> > sending us a size that does not fit in int64_t. We should gracefully
> > handle this during nbdinfo, rather than giving up early.
> >
> > With the same one-line hack to qemu to set the most significant bit of
> > the export size, output changes from pre-patch:
> >
> > $ ./run nbdinfo nbd://localhost
> > /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> > 9223372036854781440 which does not fit in signed result: Value too large
> > for defined data type
> > qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
> > end-of-file before all data were read
> >
> > to post-patch:
> >
> > $ ./run nbdinfo nbd://localhost
> > protocol: newstyle-fixed without TLS, using extended packets
> > ...
> > block_size_maximum: 33554432
> >
[1]
> >
> > Sadly, since writing a server with such large export sizes requires a
> > one-off hack, I don't see the point in adding a unit test.
> >
> > Signed-off-by: Eric Blake
> > ---
> > info/show.c | 25 +
> > 1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/info/show.c b/info/show.c
> > index a71d837e..3d80545e 100644
> > --- a/info/show.c
> > +++ b/info/show.c
> > @@ -46,7 +46,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> > bool first, bool last)
> > {
> >int64_t i, size;
> > - char size_str[HUMAN_SIZE_LONGEST];
> > + char size_str[HUMAN_SIZE_LONGEST] = "unavailable";
> >bool human_size_flag;
Uninitialized...
> >char *export_name = NULL;
> >char *export_desc = NULL;
> > @@ -89,13 +89,10 @@ show_one_export (struct nbd_handle *nbd, const char
> > *desc,
> > return false;
> >}
> >size = nbd_get_size (nbd);
> > - if (size == -1) {
> > -fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> > -exit (EXIT_FAILURE);
> > + if (size >= 0) {
> > +human_size (size_str, size, _size_flag);
> >}
> >
> > - human_size (size_str, size, _size_flag);
...and relies on human_size() to initialize it, but that is now
conditionally skipped. Would matter if...
> > -
> >if (uri_is_meaningful ())
> > uri = nbd_get_uri (nbd);
> >
> > @@ -130,7 +127,8 @@ show_one_export (struct nbd_handle *nbd, const char
> > *desc,
> > show_context = true;
> >
> >/* Get content last, as it moves the connection out of negotiating */
> > - content = get_content (nbd, size);
> > + if (size >= 0)
> > +content = get_content (nbd, size);
> >
> >if (!json_output) {
> > ansi_colour (ANSI_FG_BOLD_BLACK, fp);
> > or
> >
> > $ ./run nbdinfo nbd://localhost --json
> > {
> > "protocol": "newstyle-fixed",
> > ...
> > "block_size_maximum": 33554432,
> > "export-size-str": "unavailable"
> > } ]
> > }
> > @@ -140,10 +138,12 @@ show_one_export (struct nbd_handle *nbd, const char
> > *desc,
> > fprintf (fp, ":\n");
> > if (desc && *desc)
> >fprintf (fp, "\tdescription: %s\n", desc);
> > -if (human_size_flag)
> > - fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> > -else
> > - fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> > +if (size >= 0) {
> > + if (human_size_flag)
> > +fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
...branching on the variable is guarded by anything different than the
condition that controlled whether the variable was set in the first
place. Although it is not broken here, it is a trap for the unwary,
so I'll be initializing human_size_flag to false at its declaration.
> > + else
> > +fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> > +}
[2]
> > if (content)
> >fprintf (fp, "\tcontent: %s\n", content);
> > if (uri)
> > @@ -273,7 +273,8 @@ show_one_export (struct nbd_handle *nbd, const char
> > *desc,
> > block_maximum);
> >
> > /* Put this one at the end because of the stupid comma thing in JSON.
> > */
> > -fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> > +if (size >= 0)
> > + fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> > fprintf (fp, "\t\"export-size-str\": \"%s\"\n", size_str);
> >
> > if (last)
>
> Assuming the size is unavailable, the non-JSON output includes neither
> the numeric nor the human-readable size, whereas the JSON output still
> includes the human-readable size (as "unavailable").
Indeed, [1] shows the inconsistency. The argument for JSON is that
"export-size-str" is not designed for machine parsing (it already
makes approximations, so outputting "unavailable" is no worse for a
human to parse than "1G", while machines should be parsing
"export-size" if present).
Additionally, having the last row in JSON be unconditionally printed
makes handling trailing
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote: > On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote: > > On 21.09.23 19:43, Richard W.M. Jones wrote: > > >So this is probably another instance or variation of the timezone > > >formatting problem (of schtasks). Which version of virt-v2v is this? > > >I want to check that you have a version with all the latest patches in > > >this area. > > > > It's 2.2.0-1 from Debian (12) bookworm. I've verified that it > > doesn't have any distro-specific patches. > > > > (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian > > would have a patches/series file in this case) > > The timezone fixes are: > > commit 597d177567234c3a539098c423649781424eeb6f > Author: Laszlo Ersek > Date: Tue Mar 8 15:30:51 2022 +0100 > > convert_windows: rewrite "configure_qemu_ga" script purely in PowerShell > > commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f > Author: Richard W.M. Jones > Date: Fri Nov 12 08:47:55 2021 + > > convert/convert_windows.ml: Handle date formats with dots instead of / > > They are all included in >= 2.0 > > I wonder if 597d177567 has a subtle flaw, or if we introduced a bug > somewhere when refactoring this code later. > > Lee: Do you have a theory about exactly what is wrong with the > schtasks date? Like what was it supposed to be, assuming it was 120 > seconds in the future from boot time, versus what it was set to: > > > Firstboot-qemu-ga9/21/2023 4:04:00 PM Ready > > Could a date or time field have not been swapped or been corrupted > in some predictable way? Or in even simpler terms, what is the time (and timezone) that this ^^^ machine was booted? Rich. > The code we run is here: > > https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L571 > > Ming: this could be a bug affecting PST (UTC-8) guests, perhaps > somehow related to having a single digit month field? > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > libguestfs lets you edit virtual machines. Supports shell scripting, > bindings from many languages. http://libguestfs.org -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
On 9/21/23 22:58, Eric Blake wrote:
> As previous patches showed, the NBD spec does not yet forbid a server
> sending us a size that does not fit in int64_t. We should gracefully
> handle this during nbdinfo, rather than giving up early.
>
> With the same one-line hack to qemu to set the most significant bit of
> the export size, output changes from pre-patch:
>
> $ ./run nbdinfo nbd://localhost
> /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> 9223372036854781440 which does not fit in signed result: Value too large for
> defined data type
> qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
> end-of-file before all data were read
>
> to post-patch:
>
> $ ./run nbdinfo nbd://localhost
> protocol: newstyle-fixed without TLS, using extended packets
> ...
> block_size_maximum: 33554432
>
> or
>
> $ ./run nbdinfo nbd://localhost --json
> {
> "protocol": "newstyle-fixed",
> ...
> "block_size_maximum": 33554432,
> "export-size-str": "unavailable"
> } ]
> }
>
> Sadly, since writing a server with such large export sizes requires a
> one-off hack, I don't see the point in adding a unit test.
>
> Signed-off-by: Eric Blake
> ---
> info/show.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/info/show.c b/info/show.c
> index a71d837e..3d80545e 100644
> --- a/info/show.c
> +++ b/info/show.c
> @@ -46,7 +46,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> bool first, bool last)
> {
>int64_t i, size;
> - char size_str[HUMAN_SIZE_LONGEST];
> + char size_str[HUMAN_SIZE_LONGEST] = "unavailable";
>bool human_size_flag;
>char *export_name = NULL;
>char *export_desc = NULL;
> @@ -89,13 +89,10 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> return false;
>}
>size = nbd_get_size (nbd);
> - if (size == -1) {
> -fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> -exit (EXIT_FAILURE);
> + if (size >= 0) {
> +human_size (size_str, size, _size_flag);
>}
>
> - human_size (size_str, size, _size_flag);
> -
>if (uri_is_meaningful ())
> uri = nbd_get_uri (nbd);
>
> @@ -130,7 +127,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> show_context = true;
>
>/* Get content last, as it moves the connection out of negotiating */
> - content = get_content (nbd, size);
> + if (size >= 0)
> +content = get_content (nbd, size);
>
>if (!json_output) {
> ansi_colour (ANSI_FG_BOLD_BLACK, fp);
> @@ -140,10 +138,12 @@ show_one_export (struct nbd_handle *nbd, const char
> *desc,
> fprintf (fp, ":\n");
> if (desc && *desc)
>fprintf (fp, "\tdescription: %s\n", desc);
> -if (human_size_flag)
> - fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> -else
> - fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> +if (size >= 0) {
> + if (human_size_flag)
> +fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> + else
> +fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> +}
> if (content)
>fprintf (fp, "\tcontent: %s\n", content);
> if (uri)
> @@ -273,7 +273,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> block_maximum);
>
> /* Put this one at the end because of the stupid comma thing in JSON. */
> -fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> +if (size >= 0)
> + fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> fprintf (fp, "\t\"export-size-str\": \"%s\"\n", size_str);
>
> if (last)
Assuming the size is unavailable, the non-JSON output includes neither
the numeric nor the human-readable size, whereas the JSON output still
includes the human-readable size (as "unavailable").
Is this difference intentional? I'd have thought that the JSON output
should similarly exclude the human-readable size string in case the size
were not available -- consequently, there'd be no need for initializing
"size_str" to "unavailable", because "size_str" would never be printed
if the size were unavailable.
If OTOH the behavior is intentional, then
Reviewed-by: Laszlo Ersek
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply
On 9/21/23 22:58, Eric Blake wrote:
> If a server replies to a block status command with an invalid count in
> NBD_REPLY_TYPE_BLOCK_STATUS_EXT, we correctly detect the server's
> error, but fail to mark that we've consumed enough data off the wire
> to resync back to the server's next reply. Rich's fuzzing run
> initially found this, but I was able to quickly write a one-byte patch
> on top of my pending qemu patches [1] to reproduce it:
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html
>
> | diff --git i/nbd/server.c w/nbd/server.c
> | index 898580a9b0b..bd8d46ba3c4 100644
> | --- i/nbd/server.c
> | +++ w/nbd/server.c
> | @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest
> *request, NBDExtentArray *ea,
> | iov[1].iov_base = _ext;
> | iov[1].iov_len = sizeof(meta_ext);
> | stl_be_p(_ext.context_id, context_id);
> | -stl_be_p(_ext.count, ea->count);
> | +stl_be_p(_ext.count, !ea->count);
> |
> | nbd_extent_array_convert_to_be(ea);
> | iov[2].iov_base = ea->extents;
>
> then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have
> pre-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
>> def f(*k):
>> pass
>> try:
>> h.block_status(1,0,f)
>> except nbd.Error as ex:
>> print(ex.string)
>> h.shutdown()
>> EOF
> nbdsh: generator/states-reply-chunk.c:701:
> enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed.
> Aborted (core dumped)
>
> vs. post-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
>> def f(*k):
>> pass
>> try:
>> h.block_status(1,0,f)
>> except nbd.Error as ex:
>> print(ex.string)
>> h.shutdown()
>> EOF
> nbd_block_status: block-status: command failed: Protocol error
>
> Appears to be a casualty of rebasing: I added h->payload_left
> verification fairly late in the game, then floated it earlier in the
> series, and missed a spot where I added a state machine jump to RESYNC
> without having updated h->payload_left. An audit of h->hlen
> modification sites show that all other chunked reads updated
> h->payload_left appropriately (often in the next statement, but
> sometimes in a later state when that made logic easier).
>
> Requires a non-compliant server, and only possible when extended
> headers are negotiated, which does not affect any stable released
> libnbd. Thus, there is no reason to create a CVE, although since I
> will already be doing a security info email about previous patches
> also addressing fuzzer findings, I can mention this at the same time.
>
> Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status")
> Thanks: Richard W.M. Jones
> Signed-off-by: Eric Blake
> ---
> generator/states-reply-chunk.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 20407d91..5a31c192 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -476,6 +476,7 @@ REPLY.CHUNK_REPLY.RECV_BS_HEADER:
>if (h->bs_count != be32toh (h->sbuf.reply.payload.bs_hdr_64.count)) {
> h->rbuf = NULL;
> h->rlen = h->payload_left;
> +h->payload_left = 0;
> SET_NEXT_STATE (%RESYNC);
> return 0;
>}
Right, this seems consistent with other transitions to RESYNC (we zero
out payload_left in all those locations), and after we go from RESYNC to
FINISH, FINISH asserts the zero value.
Reviewed-by: Laszlo Ersek
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 4/6] block_status: Fix assertion with large server size
On 9/21/23 22:58, Eric Blake wrote:
> As mentioned in the previous commit ("api: Sanitize sizes larger than
> INT64_MAX"), the NBD spec does not (yet) prohibit a server from
> advertising a size larger than INT64_MAX. While we can't report such
> size to the user, v1.16 was at least internally consistent with the
> server's size everywhere else.
>
> But when adding code to implement 64-bit block status, I intentionally
> wanted to guarantee that the callback sees a positive int64_t length
> even when the server's wire value can be 64 bits, for ease in writing
> language bindings (OCaml in particular benefitted from that
> guarantee), even though I didn't document that in the API until now.
> That was because I had blindly assumed that the server's exportsize
> fit in 63 bits, and therefore I didn't have to worry about arithmetic
> overflow once I capped the extent length to exportsize. But the
> fuzzer quickly proved me wrong. What's more, with the same one-line
> hack to qemu as shown in the previous commit to advertise a size with
> the high-order bit set,
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
>> def f(*k):
>> pass
>> h.block_status(1,0,f)
>> EOF
> nbdsh: generator/states-reply-chunk.c:554:
> enter_STATE_REPLY_CHUNK_REPLY_RECV_BS_ENTRIES: Assertion `h->exportsize <=
> INT64_MAX' failed.
> Aborted (core dumped)
>
> even though it did not dump core in v1.16.
>
> Since my assumption was bad, rewrite the logic to increment total
> after bounds-checking rather than before, and to bounds-check based on
> INT64_MAX+1-64M rather than on the export size. As before, we never
> report a zero-length extent to the callback. Whether or not secalert
> advises us to create a CVE for the previous patch, this bug does not
> deserve its own CVE as it was only introduced in recent unstable
> releases.
>
> Fixes: e8d837d306 ("block_status: Add some sanity checking of server
> lengths", v1.17.4)
> Thanks: Richard W.M. Jones
> Signed-off-by: Eric Blake
> ---
> generator/states-reply-chunk.c | 49 ++
> generator/C.ml | 2 +-
> 2 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 2cebe456..20407d91 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -547,11 +547,16 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
>break;
> }
>
> +/* Be careful to avoid arithmetic overflow, even when the user
> + * disabled LIBNBD_STRICT_BOUNDS to pass a suspect offset, or the
> + * server returns suspect lengths or advertised exportsize larger
> + * than 63 bits. We guarantee that callbacks will not see a
> + * length exceeding INT64_MAX or the advertised h->exportsize.
> + */
> name = h->meta_contexts.ptr[i].name;
> -total = 0;
> -cap = h->exportsize - cmd->offset;
> -assert (cap <= h->exportsize);
> -assert (h->exportsize <= INT64_MAX);
> +total = cap = 0;
> +if (cmd->offset <= h->exportsize)
> + cap = h->exportsize - cmd->offset;
>
> /* Need to byte-swap the entries returned into the callback size
> * requested by the caller. The NBD protocol allows truncation as
> @@ -560,10 +565,11 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> * don't like. We stop iterating on a zero-length extent (error
> * only if it is the first extent), on an extent beyond the
> * exportsize (unconditional error after truncating to
> - * exportsize), and on an extent exceeding a 32-bit callback (no
> - * error, and to simplify alignment, we truncate to 4G-64M); but
> - * do not diagnose issues with the server's length alignments,
> - * flag values, nor compliance with the REQ_ONE command flag.
> + * exportsize), and on an extent exceeding a callback length limit
> + * (no error, and to simplify alignment, we truncate to 64M before
> + * the limit); but we do not diagnose issues with the server's
> + * length alignments, flag values, nor compliance with the REQ_ONE
> + * command flag.
> */
> for (i = 0, stop = false; i < h->bs_count && !stop; ++i) {
>if (type == NBD_REPLY_TYPE_BLOCK_STATUS) {
> @@ -572,16 +578,12 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
>}
>else {
> orig_len = len = be64toh (h->bs_raw.wide[i].length);
> -if (len > h->exportsize) {
> - /* Since we already asserted exportsize is at most 63 bits,
> - * this ensures the extent length will appear positive even
> - * if treated as signed; treat this as an error now, rather
> - * than waiting for the comparison to cap later, to avoid
> - * arithmetic overflow.
> +if (len > INT64_MAX) {
> + /* Pick an aligned value rather than overflowing 64-bit
> + * callback; this does not require an error.
> */
>stop = true;
> -
Re: [Libguestfs] [libnbd PATCH v2 3/6] api: Sanitize sizes larger than INT64_MAX
On 9/21/23 22:58, Eric Blake wrote:
> Our stable API has always claimed that nbd_get_size() reports a
> non-negative value on success, and -1 on failure. While we know of no
> existing production server (such as nbdkit, qemu-nbd, nbd-server) that
> would advertise a size larger than off_t, the NBD spec has not yet
> committed to enforcing a hard maximum of an export size as a signed
> integer; at present, the protocol mandates merely:
> S: 64 bits, size of the export in bytes (unsigned)
>
> I've considered proposing a spec patch to upstream NBD to require a
> positive signed value, and can point to this patch to help justify
> such a patch - but that hasn't happened yet. Thus, it should be no
> surprise that our fuzzer was quickly able to emulate a theoretical
> server that claims a size larger than INT64_MAX - at which point,
> nbd_get_size() is returning a negative value that is not -1, and the
> API documentation was unclear whether the application should treat
> this as success or failure. However, as we did not crash, the fuzzer
> did not flag it as interesting in v1.16.
>
> I considered changing nbd_internal_set_size_and_flags() to move the
> state machine to DEAD for any server advertising something so large
> (that is, nbd_connect_*() would be unable to connect to such a
> server); but without the NBD spec mandating such a limit, that is an
> artificial constraint. Instead, this patch chooses to normalize all
> wire values that can't fit in the int64_t return type to an EOVERFLOW
> error, and clarifies the API documentation accordingly. Existing
> clients that depend on a known positive size and check for
> nbd_get_size()<0 are not impacted, while those that check for ==-1
> will now reject such uncommon servers instead of potentially using a
> negative value in subsequent computations (the latter includes
> nbdinfo, which changes from reporting a negative size to instead
> printing an error message). Meanwhile, clients that never called
> nbd_get_size() (presumably because they infer the size from other
> means, or because they intend to access offsets above 2^63 despite not
> being able to reliably see that size from nbd_get_size) can still do
> so.
>
> Next, I audited all instances of 'exportsize', to see if libnbd itself
> has any arithmetic issues when a large size is stored. My results for
> v1.16 are as follows:
>
> lib/nbd-protocol.h and lib/internal.h both have uint64_t exportsize
> (so we are already treating size as unsigned both for wire and
> internal representation - no nasty surprises with sign extension).
>
> generator/states-{oldstyle,opt-{go,export-name}}.c grab exportsize off
> the wire, byteswap if needed, and pass it to
> nbd_internal_set_size_and_flags() without further inspection.
>
> lib/flags.c has nbd_internal_set_size_and_flags() which stores the
> wire value into the internal value, then nbd_unlocked_get_size() which
> reports the wire value as-is (prior to this patch adding EOVERFLOW
> normalization). nbd_internal_set_block_size() mentions exportsize in
> comments, but does not actually compare against it.
>
> lib/rw.c added LIBNBD_STRICT_BOUNDS checking in v1.6 via:
> if ((h->strict & LIBNBD_STRICT_BOUNDS) &&
> (offset > h->exportsize || count > h->exportsize - offset)) {
> where offset and count are also unsigned values (count is 32-bit in
> 1.16, 64-bit in master); but the order of comparisons is robust
> against wraparound when using unsigned math. Earlier releases, or
> clients which use nbd_set_strict_mode(,0) to skip bounds checking,
> have been assuming the server will reject requests with a weird
> offset (most servers do, although the NBD spec only recommends
> that sever behavior, by stating that the burden is on the client
> to pass in-bounds requests in the first place).
>
> generator/states-reply-chunk.c added comparisons against exportsize
> for NBD_OPT_BLOCK_STATUS only after 1.16, and that's the subject of
> the next patch.
>
> No other direct uses of exportsize exist, so libnbd itself can
> internally handle sizes larger than 2^63 without misbehaving, outside
> of nbd_get_size(), even if such an offset was computed from taking the
> negative int64_t value of nbd_get_size() and turning it back into
> uint64_t offset parameter.
>
> I also audited our other APIs - everything else uses a parameter of
> type UInt64 in the generator for offsets, which is translated in C.ml
> to uint64_t; and we already know we are safe when C converts negative
> int64_t values into uint64_t.
>
> For other languages, Python.ml generates code for UInt64 by using
> 'PyArg_ParseValue("K")' with a detour through 'unsigned long long' in
> case 'uint64_t' is a different type rank despite being the same number
> of bits. The Python documentation states that "K" converts an
> arbitrary python integer value to a C uint64_t without overflow
> checking - so it is already possible to pass offset values larger than
> 2^63 in nbdsh; while values larger
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote: > On 21.09.23 19:43, Richard W.M. Jones wrote: > >So this is probably another instance or variation of the timezone > >formatting problem (of schtasks). Which version of virt-v2v is this? > >I want to check that you have a version with all the latest patches in > >this area. > > It's 2.2.0-1 from Debian (12) bookworm. I've verified that it > doesn't have any distro-specific patches. > > (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian > would have a patches/series file in this case) The timezone fixes are: commit 597d177567234c3a539098c423649781424eeb6f Author: Laszlo Ersek Date: Tue Mar 8 15:30:51 2022 +0100 convert_windows: rewrite "configure_qemu_ga" script purely in PowerShell commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f Author: Richard W.M. Jones Date: Fri Nov 12 08:47:55 2021 + convert/convert_windows.ml: Handle date formats with dots instead of / They are all included in >= 2.0 I wonder if 597d177567 has a subtle flaw, or if we introduced a bug somewhere when refactoring this code later. Lee: Do you have a theory about exactly what is wrong with the schtasks date? Like what was it supposed to be, assuming it was 120 seconds in the future from boot time, versus what it was set to: > Firstboot-qemu-ga9/21/2023 4:04:00 PM Ready Could a date or time field have not been swapped or been corrupted in some predictable way? The code we run is here: https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L571 Ming: this could be a bug affecting PST (UTC-8) guests, perhaps somehow related to having a single digit month field? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 2/6] fuzzing: Disable client-side strictness checks
On Thu, Sep 21, 2023 at 10:22:08PM +0100, Richard W.M. Jones wrote: > On Thu, Sep 21, 2023 at 03:58:01PM -0500, Eric Blake wrote: > > When fuzzing, it is more desirable to always provoke the server into > > sending a response, rather than sometimes accidentally skipping a wire > > call because a client-side strictness test failed. > > > > [Our fuzzer could probably be made even more powerful by changing the > > fuzzer input file to be a series of records, where each record is the > > API to call and then the server's response; right now, the sequence of > > APIs called is hard-coded, which is not as powerful at testing > > potential cross-command coupling. But that's a project for another > > day.] > > Intrigued by how this would work exactly. The current file is just a > stream of bytes from the "server" (the output from libnbd is > discarded). Would the input file be of the form: > > ( [ request + args ] [ stream of bytes from server ] )* > > where [ request + args ] would be a simple serialized representation > of a synchronous nbd_* API call? I think that would work ... Actually I don't think this would work. The stream of bytes is variable length so it would need a sentinel to indicate the end of one stream (hard to do in-stream) or a length field up front. However fuzzing doesn't work well with length + data since the fuzzer doesn't understand the relationship. This could work (I've abbreviated "request + args" to just "request" and "stream ..." to "stream"): ( [ request ] )* [ sentinel ] [ stream ](* case B *) where [ sentinel ] is some sentinel that can be distinguished from a request. The idea here is the fuzzing wrapper would read the set of requests into an internal structure, then process the stream as it does now while issuing the requests. However I still don't think the fuzzer would work well with this. The problem is that the stream of bytes is still related to the set of requests. For example if a new request was inserted then some later point in the stream would need to change, but the fuzzer wouldn't understand this relationship. [Now I'm thinking if only we could use an AI transformer it would solve this problem too! Transformers can find these kind of long-range moving dependencies.] This seems better still: [ request ] [ stream ] (* case C *) There's a single request (which can represent exactly one nbd_* request), and a single associated stream. The fuzzer would generate alternate nbd_* APIs as it mutated the request field. However the problem here is we no longer have any way to describe multiple dependent requests. So ... I think my answer is to go back to case B: ( [ request ] )* [ sentinel ] [ stream ] but make the starting test cases all look (nearly) like case C: [ request ] [ sentinel ] [ stream ] The fuzzer would start by finding the simple test cases looking like C, and then might create longer test cases looking like B later in the run (or might not, we'd need to examine the output after a long run). Anyway, something for a rainy day. Rich. > Rich. > > > Signed-off-by: Eric Blake > > Reviewed-by: Richard W.M. Jones > > Reviewed-by: Laszlo Ersek > > --- > > fuzzing/libnbd-fuzz-wrapper.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c > > index cbd55380..fcd1d04c 100644 > > --- a/fuzzing/libnbd-fuzz-wrapper.c > > +++ b/fuzzing/libnbd-fuzz-wrapper.c > > @@ -193,8 +193,11 @@ client (int sock) > >} > > > >/* Note we ignore errors in these calls because we are only > > - * interested in whether the process crashes. > > + * interested in whether the process crashes. Likewise, we don't > > + * want to accidentally avoid sending traffic to the server merely > > + * because client side strictness sees a problem. > > */ > > + nbd_set_strict_mode (nbd, 0); > > > >/* Enable a metadata context, for block status below. */ > >nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION); > > -- > > 2.41.0 > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-top is 'top' for virtual machines. Tiny program with many > powerful monitoring features, net stats, disk stats, logging, etc. > http://people.redhat.com/~rjones/virt-top -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
