vpelletier / python-libusb1

Python ctype-based wrapper around libusb1
GNU Lesser General Public License v2.1
174 stars 67 forks source link

Improve a sketchy implementation of free() #60

Closed whitequark closed 3 years ago

whitequark commented 3 years ago

It's not necessary to import anything to get free since it is (dynamically) linked into the Python executable and can be found in it. We could open a ctypes.CDLL(None) (equivalent to dlopen(NULL), but a more idiomatic ctypes.pythonapi import works the same:

>>> import ctypes
>>> ctypes.pythonapi
<PyDLL 'None', handle 7fd37eb34190 at 0x7fd37dd35e10>
>>> ctypes.pythonapi.free
<_FuncPtr object at 0x7fd37e021750>

Of course, well-designed C libraries provide functions to free any memory they allocate. libusb does, too, since 1.0.20, so use that if possible (but keep the workaround so we don't start leaking memory where we didn't before).

vpelletier commented 3 years ago

Hello,

On the addition of libusb_free_pollfds: fantastic, thanks ! Sorry that I missed this function's introduction in libusb, which indeed allows avoiding this historically shady assumption about how the memory was allocated inside libusb.

OTOH, I'm less enthusiastic about the change of how to locate libc free: IIRC, the import of free from the libc was because of a duality on windows (I forgot the name of the non-libc function), in prevision of pollfd support there. Overall I am very tempted to not change that part (beyond a comment that this is compatibility for libusb<1.0.20), as becomes less likely to matter now, and I would rather get free from the horse's mouth than from a library which does not export it itself:

$ objdump -tT /usr/lib/x86_64-linux-gnu/libpython2.7_d.so.1.0 | grep '\<free\>'
0000000000000000       F *UND*  0000000000000000              free@@GLIBC_2.2.5
0000000000000000      DF *UND*  0000000000000000  GLIBC_2.2.5 free

Just as a side note from experimenting with this, I realised this symbol can also be pulled from libusb (but I'm not a fan of this either):

$ objdump -tT /usr/lib/x86_64-linux-gnu/libusb-1.0.so | grep '\<free\>'
0000000000000000      DF *UND*  0000000000000000  GLIBC_2.2.5 free
>>> import usb1
>>> usb1.libusb1.libusb.free
<_FuncPtr object at 0x7efe67e08dc0>
whitequark commented 3 years ago

IIRC, the import of free from the libc was because of a duality on windows

Yes, I guessed that this was your concern. However, pollfd is not a thing on Windows and will never be a thing there, so in my view this is irrelevant.

whitequark commented 3 years ago

To elaborate, the Windows IO model is such that you cannot (with the exception of BSD-derived sockets) wait or poll for IO readiness. The only thing you can wait on, by design, is an operation you have submitted. That tends to cause a lot of issues porting Unix software, but in our case, it means we don't ever have to worry about pollfd becoming available.

whitequark commented 3 years ago

Rebased this too

vpelletier commented 3 years ago

Merged, thanks.

vpelletier commented 3 years ago

...and I notice now that travis-ci is not happy: pypi does not have pythonapi.

There are also segfaults on all tests which build libusb.git's master, but this is likely unrelated. And the very latest builds are broken because pip falls into my setup.py traps, I'm working on this.

whitequark commented 3 years ago

...and I notice now that travis-ci is not happy: pypi does not have pythonapi.

Ah yeah, I haven't even been thinking of 2.7 when writing that. CDLL(None) should still work though...

vpelletier commented 3 years ago

I confirm this fixes the issue. Now all is left are the libusb1 master failures, which do not block a release, but will require investigation.