Skip to content

Commit 9c2f3d0

Browse files
committed
usb1: Keep __ctypesCallbackWrapper alive longer.
When calling "close" inside the callback (which at least happens if current transfer has been "doom"ed before user callback returns), this instance attribute was cleared, causing it to be possibly gabrage-collected before code could return to libusb, causing occasional segfaults.
1 parent 740c977 commit 9c2f3d0

1 file changed

Lines changed: 30 additions & 21 deletions

File tree

usb1/__init__.py

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -314,8 +314,10 @@ class USBTransfer(object):
314314
# pylint: enable=undefined-variable
315315
__transfer = None
316316
__initialized = False
317-
__submitted = False
317+
__submitted_dict = {}
318318
__callback = None
319+
# Just to silence pylint watnings, this attribute gets overridden after
320+
# class definition.
319321
__ctypesCallbackWrapper = None
320322
__doomed = False
321323
__user_data = None
@@ -342,23 +344,19 @@ def __init__(self, handle, iso_packets, before_submit, after_completion):
342344
raise USBErrorNoMem
343345
# pylint: enable=undefined-variable
344346
self.__transfer = result
345-
self.__ctypesCallbackWrapper = libusb1.libusb_transfer_cb_fn_p(
346-
self.__callbackWrapper)
347347

348348
def close(self):
349349
"""
350350
Break reference cycles to allow instance to be garbage-collected.
351351
Raises if called on a submitted transfer.
352352
"""
353-
if self.__submitted:
353+
if self.isSubmitted():
354354
raise ValueError('Cannot close a submitted transfer')
355355
self.doom()
356356
self.__initialized = False
357357
# Break possible external reference cycles
358358
self.__callback = None
359359
self.__user_data = None
360-
# Break libusb_transfer reference cycles
361-
self.__ctypesCallbackWrapper = None
362360
# For some reason, overwriting callback is not enough to remove this
363361
# reference cycle - though sometimes it works:
364362
# self -> self.__dict__ -> libusb_transfer -> dict[x] -> dict[x] ->
@@ -395,20 +393,19 @@ def __del__(self):
395393
# Transfer was not submitted, we can free it.
396394
self.__libusb_free_transfer(self.__transfer)
397395

398-
# pylint: disable=unused-argument
399-
def __callbackWrapper(self, transfer_p):
396+
@classmethod
397+
def __callbackWrapper(cls, transfer_p):
400398
"""
401399
Makes it possible for user-provided callback to alter transfer when
402400
fired (ie, mark transfer as not submitted upon call).
403401
"""
404-
self.__submitted = False
402+
self = cls.__submitted_dict.pop(addressof(transfer_p.contents))
405403
self.__after_completion(self)
406404
callback = self.__callback
407405
if callback is not None:
408406
callback(self)
409407
if self.__doomed:
410408
self.close()
411-
# pylint: enable=unused-argument
412409

413410
def setCallback(self, callback):
414411
"""
@@ -443,7 +440,7 @@ def setControl(
443440
timeout
444441
Transfer timeout in milliseconds. 0 to disable.
445442
"""
446-
if self.__submitted:
443+
if self.isSubmitted():
447444
raise ValueError('Cannot alter a submitted transfer')
448445
if self.__doomed:
449446
raise DoomedTransferError('Cannot reuse a doomed transfer')
@@ -497,7 +494,7 @@ def setBulk(
497494
timeout
498495
Transfer timeout in milliseconds. 0 to disable.
499496
"""
500-
if self.__submitted:
497+
if self.isSubmitted():
501498
raise ValueError('Cannot alter a submitted transfer')
502499
if self.__doomed:
503500
raise DoomedTransferError('Cannot reuse a doomed transfer')
@@ -535,7 +532,7 @@ def setInterrupt(
535532
timeout
536533
Transfer timeout in milliseconds. 0 to disable.
537534
"""
538-
if self.__submitted:
535+
if self.isSubmitted():
539536
raise ValueError('Cannot alter a submitted transfer')
540537
if self.__doomed:
541538
raise DoomedTransferError('Cannot reuse a doomed transfer')
@@ -577,7 +574,7 @@ def setIsochronous(
577574
will be divided evenly among available transfers if possible, and
578575
raise ValueError otherwise.
579576
"""
580-
if self.__submitted:
577+
if self.isSubmitted():
581578
raise ValueError('Cannot alter a submitted transfer')
582579
num_iso_packets = self.__num_iso_packets
583580
if num_iso_packets == 0:
@@ -771,7 +768,7 @@ def setBuffer(self, buffer_or_len):
771768
setIsochronous).
772769
Note: disallowed on control transfers (use setControl).
773770
"""
774-
if self.__submitted:
771+
if self.isSubmitted():
775772
raise ValueError('Cannot alter a submitted transfer')
776773
transfer = self.__transfer.contents
777774
# pylint: disable=undefined-variable
@@ -798,13 +795,15 @@ def isSubmitted(self):
798795
"""
799796
Tells if this transfer is submitted and still pending.
800797
"""
801-
return self.__submitted
798+
return self.__transfer is not None and addressof(
799+
self.__transfer.contents,
800+
) in self.__submitted_dict
802801

803802
def submit(self):
804803
"""
805804
Submit transfer for asynchronous handling.
806805
"""
807-
if self.__submitted:
806+
if self.isSubmitted():
808807
raise ValueError('Cannot submit a submitted transfer')
809808
if not self.__initialized:
810809
raise ValueError(
@@ -813,11 +812,13 @@ def submit(self):
813812
if self.__doomed:
814813
raise DoomedTransferError('Cannot submit doomed transfer')
815814
self.__before_submit(self)
816-
self.__submitted = True
817-
result = libusb1.libusb_submit_transfer(self.__transfer)
815+
transfer = self.__transfer
816+
assert transfer is not None
817+
self.__submitted_dict[addressof(transfer.contents)] = self
818+
result = libusb1.libusb_submit_transfer(transfer)
818819
if result:
819820
self.__after_completion(self)
820-
self.__submitted = False
821+
self.__submitted_dict.pop(addressof(transfer.contents))
821822
raiseUSBError(result)
822823

823824
def cancel(self):
@@ -826,13 +827,21 @@ def cancel(self):
826827
Note: cancellation happens asynchronously, so you must wait for
827828
TRANSFER_CANCELLED.
828829
"""
829-
if not self.__submitted:
830+
if not self.isSubmitted():
830831
# XXX: Workaround for a bug reported on libusb 1.0.8: calling
831832
# libusb_cancel_transfer on a non-submitted transfer might
832833
# trigger a segfault.
833834
raise self.__USBErrorNotFound
834835
self.__mayRaiseUSBError(self.__libusb_cancel_transfer(self.__transfer))
835836

837+
# XXX: This is very unsightly, but I do not see another way of declaring within
838+
# class body both the class method and its ctypes function pointer.
839+
# pylint: disable=protected-access,no-member
840+
USBTransfer._USBTransfer__ctypesCallbackWrapper = libusb1.libusb_transfer_cb_fn_p(
841+
USBTransfer._USBTransfer__callbackWrapper,
842+
)
843+
# pylint: enable=protected-access,no-member
844+
836845
class USBTransferHelper(object):
837846
"""
838847
Simplifies subscribing to the same transfer over and over, and callback

0 commit comments

Comments
 (0)