Repository: pyusb/pyusb

Issue:
Regression in v1.3.0: `ctrl_transfer` function in `utils/core.py` does not re-use the array passed in the `data_or_wLength` argument
Hi,

While coming back to my PyUSB project after a few months (https://github.com/HydraDancer/facedancer), nothing worked anymore. I realized PyUSB had introduced a new version since then (`v1.3.0`) and going back to `v1.2.1` fixed the issue.

The problem comes from from the `ctrl_transfer` function I'm using (here https://github.com/HydraDancer/facedancer/blob/7753b7117ebd5af4f8b9d18a0bf542c5a89e3d05/facedancer/backends/hydradancer.py#L741) for a CTRL IN transfer, where I'm passing an already created array as `data_or_wLength` to save some time.

In `v1.2.1`, when data_or_wLength was an array, a `TypeError` exception was triggered in `create_buffer` which means `_interop.as_array` was called and the same array object was used (modified in-place as expected).

Annotated output in `ctrl_transfer`
```
data_or_wLength
array('B', [1, 0, 2, 0, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
id(data_or_wLength)
139751414283008
can't multiply sequence by non-int of type 'array.array'
buff
array('B', [1, 0, 2, 0, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
id(buf)
139751414283008
```

In  `v1.3.0`, commit 3ea79b01af08c1e8b2e0096a266c2cbd347738db introduced a change in `create_buffer` that does not trigger the `TypeError` exception and instead creates a new array from the one passed in `data_or_wLength`.

```
data_or_wLength                                                                                                                                                           
array('B', [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
id(data_or_wLength)                                                              
140588380757648                                                                                                                                                           
buff                                                                                                                                                                      
array('B', [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) 
id(buf)                                                                
140588380765408                                                                                                                                                           
```

This means a new array is created and receives data for IN transfers, not the array passed as argument.

I propose the following PR https://github.com/pyusb/pyusb/pull/527 to fix this issue, `_interop.as_array` will re-use the existing array or create a new one if data is iterable, and otherwise fail with a `TypeError` and `util.create_buffer` will be called.

Best,

Discussion:
@kauwua: Alternative fix PR https://github.com/pyusb/pyusb/pull/529

Solution patch:
diff --git a/usb/_interop.py b/usb/_interop.py
index d52780f..6181a85 100644
--- a/usb/_interop.py
+++ b/usb/_interop.py
@@ -35,30 +35,35 @@
 
 import sys
 import array
+import usb.util as util
 
 __all__ = []
 
 # we support Python >= 3.9
 assert sys.hexversion >= 0x030900f0
 
-def as_array(data=None):
-    """Convert loosely specified `data` to a byte array.
+def as_array(data_or_length=None):
+    """Convert loosely specified `data_or_length` to a byte array.
 
-    The following types of `data` are supported:
+    The following types of `data_or_length` are supported:
 
-    - the `None` value (returns an new empty array);
     - an `array('B')` value (no-op, returns it back);
+    - the `None` value (returns an new empty array);
+    - an integer length (returns a new array with the specified size);
     - lists or iterables of small enough integers;
     - strings.
     """
 
-    if data is None:
-        return array.array('B')
+    if isinstance(data_or_length, array.array) and data_or_length.typecode == 'B':
+        return data_or_length
 
-    if isinstance(data, array.array) and data.typecode == 'B':
-        return data
+    if isinstance(data_or_length, str):
+        return array.array('B', data_or_length.encode('utf-8'))
 
-    if isinstance(data, str):
-        return array.array('B', data.encode('utf-8'))
+    if data_or_length is None:
+        return array.array('B')
 
-    return array.array('B', data)
+    try:
+        return util.create_buffer(data_or_length)
+    except TypeError:
+        return array.array('B', data_or_length)
diff --git a/usb/core.py b/usb/core.py
index c407eed..85ca038 100644
--- a/usb/core.py
+++ b/usb/core.py
@@ -1077,10 +1077,7 @@ class Device(_objfinalizer.AutoFinalizedObject):
         object which the data will be read to, and the return value is the
         number of bytes read.
         """
-        try:
-            buff = util.create_buffer(data_or_wLength)
-        except TypeError:
-            buff = _interop.as_array(data_or_wLength)
+        buff = _interop.as_array(data_or_wLength)
 
         self._ctx.managed_open()
 
diff --git a/usb/util.py b/usb/util.py
index 2794b84..4184c2b 100644
--- a/usb/util.py
+++ b/usb/util.py
@@ -156,7 +156,8 @@ def create_buffer(length):
     call. This function creates a compatible sequence buffer
     of the given length.
     """
-    return array.array('B', bytes(length))
+    # Return an array with `length` zeros or raise a `TypeError`.
+    return array.array('B', length * b"\x00")
 
 def find_descriptor(desc, find_all=False, custom_match=None, **args):
     r"""Find an inner descriptor.


Test patch:
diff --git a/tests/test_interop.py b/tests/test_interop.py
index 02ec808..16cf9f1 100644
--- a/tests/test_interop.py
+++ b/tests/test_interop.py
@@ -40,6 +40,10 @@ class InteropTest(unittest.TestCase):
     def test_none_as_array(self):
         self.assertEqual(as_array(None), array('B'))
 
+    @methodtrace(utils.logger)
+    def test_length_as_array(self):
+        self.assertEqual(as_array(3 ), array('B', [0, 0, 0]))
+
     @methodtrace(utils.logger)
     def test_byte_array_as_array(self):
         data = array('B', [10, 20, 30])
diff --git a/tests/test_util.py b/tests/test_util.py
index 5484221..b87e4d9 100644
--- a/tests/test_util.py
+++ b/tests/test_util.py
@@ -28,12 +28,15 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-import utils
+import array
 import unittest
+import utils
+
+import usb.backend
+from usb._debug import methodtrace
 from usb.util import *
+
 from devinfo import *
-from usb._debug import methodtrace
-import usb.backend
 
 class _ConfigurationDescriptor(object):
     def __init__(self, bConfigurationValue):
@@ -146,6 +149,16 @@ class UtilTest(unittest.TestCase):
         self.assertEqual(build_request_type(CTRL_IN, CTRL_TYPE_RESERVED, CTRL_RECIPIENT_ENDPOINT), 0xe2)
         self.assertEqual(build_request_type(CTRL_IN, CTRL_TYPE_RESERVED, CTRL_RECIPIENT_OTHER), 0xe3)
 
+    @methodtrace(utils.logger)
+    def test_create_buffer_with_int(self):
+        self.assertEqual(create_buffer(3), array.array('B', [0, 0, 0]))
+
+    @methodtrace(utils.logger)
+    def test_create_buffer_with_other_types(self):
+        self.assertRaises(TypeError, lambda: create_buffer(None))
+        self.assertRaises(TypeError, lambda: create_buffer([10, 20, 30]))
+        self.assertRaises(TypeError, lambda: create_buffer(array.array('B', [10, 20, 30])))
+
 def get_suite():
     suite = unittest.TestSuite()
     suite.addTest(FindDescriptorTest())