frames carrying isochronous IN packets are truncated

[sindy39]

Hello,

on three sound card devices, each using a different chipset, I can observe the same issue - the data of last few IN isochronous packets are not stored to the output file (I've checked the file using hexdump of the file so I am sure it is an usbpcap issue and not a Wireshark dissector issue).

I believe I understand the reason but I may be wrong, here is how I see it:

In the URB request, the host indicates beginnings (ISO Data offset) of buffers reserved for individual incoming packets. The buffers are dimensioned for maximum possible size of the packet data as the device has declared it in enumeration phase. As the driver receives the packets, it writes each packet's contents to the appropriate buffer, and notes the actual size of the received data to the corresponding "ISO Data length" item in the URB response.

The mistake is that USBPcap then determines the total size of the frame payload (usb.data_len item in the pseudoheader) to be written into the output file by summing up all the ISO Data length values, while for correct operation, the complete sizes of the buffers should be summed instead of ISO Data length - the received data most often do not use all the space in the buffers but each packet's data always start at the ordered offset.

The fix may not be all that easy, though, because the reserved size of the buffer is not stated anywhere in the URB of the response (nor the request). Retrieving the information about buffer size from the captured enumeration phase communication would make the code much more complex and it would become mandatory to insert the device at the beginning of each capture which is not practical, and calculating the buffer size as a difference of ISO Data offsets of two subsequent buffers only works if at least two buffers are allocated and described in the request - I am not sure that this is always true.

So to me it seems that the best practical, even if not most accurate, way is to determine the payload size as (ISO Data offset + ISO Data length) of the last received packet. This works even for a single buffer in the URB, and the penalty we pay compared to using N times buffer size is that we do not save the useless contents of the last buffer between the end of received data and the end of the buffer.

Thank you Pavel

[desowin]

Hello,

I have just looked at the driver source code and I don't know where you got the "USBPcap then determines the total size of the frame payload (usb.data_len item in the pseudoheader) to be written into the output file by summing up all the ISO Data length values". Looking at the MSDN documentation however shows following: "TransferBufferLength Specifies the length, in bytes, of the buffer specified in TransferBuffer or described in TransferBufferMDL. The host controller driver returns the number of bytes that are sent to or read from the pipe in this member."

Hence I think the TransferBufferLength contains the actual buffer size on FDO -> PDO route and the number of bytes transferred on PDO -> FDO. However, I think the "(ISO Data offset + ISO Data length) of the last received packet." could work just fine.

"Retrieving the information about buffer size from the captured enumeration phase communication would make the code much more complex and it would become mandatory to insert the device at the beginning of each capture which is not practical" - this is not as bad as you described. Actually all the devices are monitored (even when the capture is not running) during enumeration phase. This is needed to be able to correlate PipeHandle with endpoint numbers.

My solution would be to store the whole buffer (truncated to snaplen of course). It could be retrieved on FDO->PDO route and stored in USBPCAP_DEVICE_DATA. Then on PDO->FDO it would be read from there. Would you mind testing such solution? If yes, please drop me an email stating which Windows version you are using and I'll send you a test driver release.

Best Regards, Tomasz

[sindy39]

Hello,

yes, I'll be glad to test your patch.

is "64 bit Windows 7 Professional, Service Pack 1" a sufficient system identification? If you need more details, please tell me where should I look for them.

As for "I don't know where you got..." - I was actually unable to find it in the code back then, it even seemed to me that you do not compute the number of bytes to store to the frame following the pointer & size block inside your own code but that you use a ready-made value computed "by someone else". The matter of fact is, however, that in the current version, the size of "the actual payload" part of the frame equals the sum of the used bytes from the individual packet buffers rather than the sum of their full sizes, no matter who and where performs the computation.

So any change which will lead to storing all of the interesting data is a good one from my point of view.

Thank you Pavel

[sindy39]

Hi Tomasz,

I have noticed that usbpcap has been integrated into Wireshark trunk. Is it your activity or someone else took over? Should I re-open the buffer truncation issue in Wireshark bugzilla (as in the usbpcap version which came along with the Wireshark 2.0.0, the last received isochronous packet is still missing in the URB)?

Thank you Pavel

[desowin]

"My solution would be to store the whole buffer (truncated to snaplen of course). It could be retrieved on FDO->PDO route and stored in USBPCAP_DEVICE_DATA. Then on PDO->FDO it would be read from there."

The problem with this idea is that the buffer is associated with IRP and it's possible to have multiple outgoing isochronous IRPs. For example with Creative SoundBlaster X-Fi HD, I observed 12 FDO->PDO IRPs before getting the first PDO->FDO response. So if it is to be stored in the USBPCAP_DEVICE_DATA then that would have to be in a map where IRP is the key. The values should be removed from the map once the PDO->FDO packet is seen.

[sindy39]

I'm not sure whether it would be a generic solution, but the space allocated for each of the USB packets in the URB seems to be invariant for each isochronous endpoint unless you change the parameters of the stream (i.e. the sampling rate and/or number of channels in case of audio streams) during operation, which happens quite rarely. So the exact mapping between requests and responses may not be necessary to determine the size of the response buffer if it would complicate/slow down the code too much. But apart from that performance impact - yes, to keep a table indexed by IRP ID would be a solution safe against surprises.

What I can see on all the USB audio devices so far is that the driver reuses the IRP ID - the first request sent after receiving a response has the same IRP ID like that response had, so the IRP ID -> response URB size mapping table is not likely to occupy too much space per endpoint. I don't have enough information to be sure whether the IRB ID is unique per each stream or whether the table index would have to include the complete endpoint address to be unambiguous.

But as thinking of it again three years later (which means almost as if it was a completely new thing to me), it still seems to me that to take the sum of the length of the last USB packet in the URB (which is indicated in the response's header) and the offset of the last USB packet (which is also indicated in the response's header) as the number of bytes to be captured past the last USB packet header structure (offset, length, status) should be enough to deliver the complete response; the performance impact of parsing the URB response's header to get these two numbers may be similar like the one of handling the request-response mapping table.

I'm still ready and keen to test, however in the meantime my Windows version has metamorphed into 10 Pro, currently in version 1803, build 17134.165 if these minor details are important for you.

And three years later, I still don't understand the code and where it actually gets the URB total size from :-)

Dziękuję Pavel

[vector360]

@desowin

I am surprised that this bug was reported 3 years ago and still has not been resolved. This issue was a show stopper for me, so I applied a fix to my own copy.

The defect is at line 710:

packetHeader->header.dataLength = (UINT32)transfer->TransferBufferLength;

The problem is that the TransferBufferLength contains the number of bytes received, and is NOT the actual buffer length.

The fix is to determine the actual buffer length. The fix shown below has been tested and works perfectly.

if (transfer->TransferBuffer)
{
    packetHeader->header.dataLength =
        transfer->IsoPacket[transfer->NumberOfPackets - 1].Offset +
        transfer->IsoPacket[transfer->NumberOfPackets - 1].Length;
}
else if (transfer->TransferBufferMDL)
{
    packetHeader->header.dataLength = MmGetMdlByteCount(
        transfer->TransferBufferMDL);
}
else
{
    packetHeader->header.dataLength = 0;
}

Please apply this fix and publish the updated / signed drivers.

Thanks in advance, Respectfully, Ralph vector360

[desowin]

@vector360 Would it be possible for you to submit the patch in the form of Pull Request?

Btw. Is the IsoPacket array guaranteed to be sorted by offsets? If there isn't explicit guarantee about that, I would prefer the code to compute the maximum (offset+length) and if there is such guarantee, it is good to mention it in a comment.

[vector360]

@desowin

I'd be happy to create a branch and submit a pull request. Unfortunately, I get a "forbidden 403" error when I try to create a branch.

$ git clone https://github.com/desowin/usbpcap.git $ cd usbpcap $ git checkout -b issue-21-isoch-truncation $ git push -u origin issue-21-isoch-truncation remote: Permission to desowin/usbpcap.git denied to vector360 fatal: unable to access 'https://github.com/desowin/usbpcap.git/': The requested URL returned error: 403

Are you able to provide permission to create a branch? Or point me to a procedure for this? I tried this procedure, but it didn't help. https://www.wikihow.com/Create-a-Pull-Request-on-Github

Best Regards

[desowin]

@vector360 The common github way of dealing with the permissions is to create your own fork of the repository, then create the branch in your fork and finally open the pull request from your repository (fork) towards the upstream repository.

You can read more about it in https://help.github.com/articles/fork-a-repo/

[vector360]

@desowin

Thank you for the info about forking a repo. I use git routinely, but haven't used GitHub until now. I forked the repo and committed the code changes, and will follow up with a pull request right away.

Best Regards vector360

[vector360]

@desowin I made some updates to my branch. I'm satisfied with the results.

Your suggestion to loop through the packet buffer array gave me the idea to return only as much as needed to capture occupied buffers, minimizing the capture size.

The final commit has been tested against Windows 7 x86 and x64. It has not been tested against Windows 10.

Much appreciation and respect, vector360

[vector360]

The latest version removes the gaps to minimize the capture size. It has been tested using Windows 7 x86. If you can provide a properly signed driver for Windows 10 x64, I'd be happy to test it prior to final release.

[desowin]

@vector360 For the Windows 10 attestation signed driver the inf (inx) file would need to be modified. See #57.

[desowin]

Should be fixed in USBPcap 1.3.0.0.