Re: [Libusb-win32-devel] Crash in reads with short timeout - race.

[Peter D. T. <don...@td...>]

Hi Travis,

Thanks for your review/input.

Sorry for late reply. Was traveling.

> Let's check the return code.  What about this:
> static int _usb_cancel_io(usb_context_t *context)
> {
>      int ret;
>      ret = _usb_abort_ep(context->dev, context-
> >req.endpoint.endpoint);
>      if (ret == 0)
>      {
>          WaitForSingleObject(context->ol.hEvent, 1000);
>      }
>      else
>      {
>          WaitForSingleObject(context->ol.hEvent, 0);
>      }
>      return ret;
> }

1) Yes, it should definately check the return code, and only change the
code path that succeeds. Good catch.

2) Normally i would agree that forever timeouts are dangerous, and can
lead to stalls. However, in this particular case the opposite is the
case. You can _not_ return without having completed the pending
read/write that has been cancelled (which is what WaitForSingleObject()
is waiting for).

If the timeout actually fires, the read/write completion would likely
trigger a crash as described in my original post. So it is not an
option that the programming calling this function can use for anything
useful. On the contrary, it can lead to strange memory corruption
issues. When _usb_cancel_io() returns you _must_ be able to rely on the
fact that you can free any resources which were used by the pending
transaction. And in the case where the resource is the local stack (as
my original problem was) the crash/corruption in unavoidable.

Further, the only way that the timeout can happen (if big enough), is
if the entire driver stack is deadlocked, which would probably lead to
something more colorful, like a BSOD.

So i would still argue that for this case the timeout needs to be
INFINITE (with a nice comment) even though it might feel wrong to do so
in other similar cases.

In any case, 1 second is a bit short, as that can be triggered on a
fully loaded system with many USB devices attached via the libusb0
driver. It should at least be 5-10 seconds to be on the safe side.

Thanks,

/pedro