hw/us/bus.c PCAP adding 0xA in Windows version

Since Windows text files use CRLFs for all \n, the Windows version of QEMU
inserts a CR in the PCAP stream when a LF is encountered when using USB PCAP
files. This is due to the fact that the PCAP file is opened as TEXT instead
of BINARY.

To show an example, when using a very common protocol to USB disks, the BBB
protocol uses a 10-byte command packet. For example, the READ_CAPACITY(10)
command will have a command block length of 10 (0xA). When this 10-byte
command (part of the 31-byte CBW) is placed into the PCAP file, the Windows
file manager inserts a 0xD before the 0xA, turning the 31-byte CBW into a
32-byte CBW.

Actual CBW:
  0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0a 25 USBC...........%
  0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00       ...............

PCAP CBW
  0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0d 0a USBC............
  0050 25 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 %..............

I believe simply opening the PCAP file as BINARY instead of TEXT will fix
this issue.

Resolves: https://bugs.launchpad.net/qemu/+bug/2054889
Signed-off-by: Benjamin David Lunt <benlunt@fysnet.net>
---
 hw/usb/bus.c | 2 +-
1 file changed, 2 insertions(+), 2 deletions(-)

     }
 }

On Sun, 25 Feb 2024, benlunt@fysnet.net wrote:
> Since Windows text files use CRLFs for all \n, the Windows version of QEMU
> inserts a CR in the PCAP stream when a LF is encountered when using USB PCAP
> files. This is due to the fact that the PCAP file is opened as TEXT instead
> of BINARY.
>
> To show an example, when using a very common protocol to USB disks, the BBB
> protocol uses a 10-byte command packet. For example, the READ_CAPACITY(10)
> command will have a command block length of 10 (0xA). When this 10-byte
> command (part of the 31-byte CBW) is placed into the PCAP file, the Windows
> file manager inserts a 0xD before the 0xA, turning the 31-byte CBW into a
> 32-byte CBW.
>
> Actual CBW:
>  0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0a 25 USBC...........%
>  0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00       ...............
>
> PCAP CBW
>  0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0d 0a USBC............
>  0050 25 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 %..............
>
> I believe simply opening the PCAP file as BINARY instead of TEXT will fix
> this issue.

Do you believe or also tested it? :-) There's a typo in the subject, 
should be hw/usb/. No need to resend, just noting it for whoever will 
manage this patch.

Regards,
BALATON Zoltan

> Resolves: https://bugs.launchpad.net/qemu/+bug/2054889
> Signed-off-by: Benjamin David Lunt <benlunt@fysnet.net>
> ---
> hw/usb/bus.c | 2 +-
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff -u a/hw/usb/bus.c b/hw/usb/bus.c
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -273,13 +273,13 @@
>     }
>
>     if (dev->pcap_filename) {
> -        int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY |
> O_TRUNC, 0666);
> +        int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY |
> O_TRUNC | O_BINARY, 0666);
>         if (fd < 0) {
>             error_setg(errp, "open %s failed", dev->pcap_filename);
>             usb_qdev_unrealize(qdev);
>             return;
>         }
> -        dev->pcap = fdopen(fd, "w");
> +        dev->pcap = fdopen(fd, "wb");
>         usb_pcap_init(dev->pcap);
>     }
> }
>
>
>
>

On 25/02/2024 20.49, benlunt@fysnet.net wrote:
> Since Windows text files use CRLFs for all \n, the Windows version of QEMU
> inserts a CR in the PCAP stream when a LF is encountered when using USB PCAP
> files. This is due to the fact that the PCAP file is opened as TEXT instead
> of BINARY.
> 
> To show an example, when using a very common protocol to USB disks, the BBB
> protocol uses a 10-byte command packet. For example, the READ_CAPACITY(10)
> command will have a command block length of 10 (0xA). When this 10-byte
> command (part of the 31-byte CBW) is placed into the PCAP file, the Windows
> file manager inserts a 0xD before the 0xA, turning the 31-byte CBW into a
> 32-byte CBW.
> 
> Actual CBW:
>    0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0a 25 USBC...........%
>    0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00       ...............
> 
> PCAP CBW
>    0040 55 53 42 43 01 00 00 00 08 00 00 00 80 00 0d 0a USBC............
>    0050 25 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 %..............
> 
> I believe simply opening the PCAP file as BINARY instead of TEXT will fix
> this issue.
> 
> Resolves: https://bugs.launchpad.net/qemu/+bug/2054889
> Signed-off-by: Benjamin David Lunt <benlunt@fysnet.net>
> ---
>   hw/usb/bus.c | 2 +-
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -u a/hw/usb/bus.c b/hw/usb/bus.c
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -273,13 +273,13 @@
>       }
>   
>       if (dev->pcap_filename) {
> -        int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY |
> O_TRUNC, 0666);
> +        int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY |
> O_TRUNC | O_BINARY, 0666);

  Hi Benjamin!

Thanks for the patch! Since it's rather a trivial patch and our USB 
maintainer Gerd is pretty much busy with other stuff right now, I went ahead 
and put it in my current pull request.

Two things to notice: First, something along the way (likely your mail 
program) added a line break after the "O_WRONLY |" in the above two lines, 
so I had to undo that change manually before I was able to apply the patch 
... please try to use "git send-email" for sending patches, then such things 
don't happen.
And second, please use the scripts/checkpatch.pl script to check your 
patches - it was complaining about a line getting too long here, so I went 
ahead and fixed that, too (i.e. no need to resend, just a FYI for future 
patches).

  Thanks,
   Thomas

>           if (fd < 0) {
>               error_setg(errp, "open %s failed", dev->pcap_filename);
>               usb_qdev_unrealize(qdev);
>               return;
>           }
> -        dev->pcap = fdopen(fd, "w");
> +        dev->pcap = fdopen(fd, "wb");
>           usb_pcap_init(dev->pcap);
>       }
>   }
> 
> 
>