[PATCH] darwin_async_io_callback data truncation

[rogueresearch]

The private function darwin_async_io_callback() has a bug.

It is correctly declared with the IOAsyncCallback1 signature:

static void darwin_async_io_callback (void *refcon, IOReturn result, void *arg0)

Note the last param is a pointer and thus either 32 or 64 bit in size, depending on architecture.

The last lines of code in the function are:

write (priv->fds[1], &message, sizeof (message));
write (priv->fds[1], &itransfer, sizeof (itransfer));
write (priv->fds[1], &result, sizeof (IOReturn));
write (priv->fds[1], &arg0, sizeof (UInt32));

The last line is wrong on 64 bit architectures. And a wonderful example of why it is preferable to use sizeof() on a variable, and not on a type. That's done for the first 2, but not the last two.

Attached is an *untested* patch that fixes the above. I also reviewed use of sizeof() in the same file and fixed a few others.

I'm hoping this is why libusb is broken for me on ppc64 but working on x86_64. On big endian, only the most significant (likely 0) bytes are written.

[stuge]

The patch is not quite complete, because it will write 64 bits but only read 32 on the other end.

The question is if it is better to write 64 bits and truncate into UInt32 io_size on the receiving end, or if it is better to truncated void *arg0 into an UInt32 before it gets written over the pipe. What's your take, Nathan?

[stuge]

Replying to stuge:

The question is if it is better to write 64 bits and truncate into UInt32 io_size on the receiving end, or if it is better to truncated void *arg0 into an UInt32 before it gets written

..or the third option of course: to always write and read UInt64, which would require darwin_handle_callback() to change the io_size parameter from UInt32. I'm not sure if UInt64 would be relevant?

[rogueresearch]

stuge, thanks for noticing the omission in my patch. As I said, it's untested.

Option 3 would quickly morph into ticket #85 I suspect.

Perhaps it would be best to truncate 64 -> 32 (the value, not the pointer, of course) until #85 can be thoroughly dealt with.

But I don't know libusb well, and defer to Nathan and you.

[vlovich]

I think it's best to assign arg0 to a 32-bit value that is written but also put in an assert in darwin_async_io_callback that arg0 <= UINT32_MAX (or instead of assert make it an explicit check that prints an error). Also, it should send the full 64-bit value across the pipe & do the truncation in op_handle_events.

[stuge]

Replying to vlovich:

I think it's best to assign arg0 to a 32-bit value that is written

..

Also, it should send the full 64-bit value across the pipe

I think these are directly opposing statements? :)

[hjelmn]

I doubt a usb transaction will ever be more than 2^{32 bytes so it is safe to just write a 32 bit value (for now).}

Add the following declaration:
UInt32 size = (UInt32) arg0;

and change
write (priv->fds[1], &arg0, sizeof (UInt32));
to
write (priv->fds[1], &size, sizeof (size));

[rogueresearch]

newer patch

[rogueresearch]

The just attached patch does as Nathan suggests, which, given the imminent release, is probably best. It introduces a compiler warning about truncating a pointer to a smaller integer (which indeed should not be done), but at run time this is better than what was there previously.

I've only tested that it builds.

[rogueresearch]

OK, I've tested this on ppc64, i386, and x86_64. Works great for me. Can we please get it in 1.0.9, thanks!

[hjelmn]

Should have noted I fixed this in libusb-darwin. Should make it into 1.0.9.

[Nathan Hjelm <hjelmn@…>]

In [b1ee2ef8d0b67dcc51c15742cb2decb386cffd9b/libusb]:

Darwin: Fix #117 transfer size 64/32 bit issue for transfer callbacks

The transfer size is now always truncated to 32 bits.