UInput: Constructor can throw exception, leaving open a uinput fd

[antheas]

At the end of the uinput constructor, it calls self._find_device().

This function does the following:

    def _find_device(self):
        #:bug: the device node might not be immediately available
        time.sleep(0.1)

        for path in util.list_devices('/dev/input/'):
            d = device.InputDevice(path)
            if d.name == self.name:
                return d

If between util.list_devices('/dev/input/'): being called and device.InputDevice(path) being called, the device of path closes, this leads to a thrown exception.

This exception propagates through the constructor, which has opened a filedescriptor from uinput. That fd never closes.

This leads to a duplicate device.

[antheas]

Simplest solution is to wrap the inner loop to avoid errors.

    def _find_device(self):
        #:bug: the device node might not be immediately available
        time.sleep(0.1)

        for path in util.list_devices('/dev/input/'):
            try:
                d = device.InputDevice(path)
                if d.name == self.name:
                    return d
            except Exception:
                pass

[sezanzeb]

except Exception:

Which Exception exactly? It would be better practice to specify the exact exception that we want to catch here

[antheas]

You can not re-raise any exceptions there unless you rewrite the constructor to close the file descriptor before raising.

The error is a FileNotFoundError.

[sezanzeb]

The error is a FileNotFoundError.

Thanks. I guess it would make sense to commit this change to python-evdev, however, there might be more changes to this soon because of #205, so lets wait

[antheas]

You have to catch all exceptions, not just file not found error, and not re-emit them since the inner loop crashing does not affect the class functionality.

Exception does not include Keyboard Interrupts.

There is also a race condition with Keyboard Interrupts but python evdev does not work correctly with those because it cleans up on free.

[sezanzeb]

You have to catch all exceptions, not just file not found error, and not re-emit them since the inner loop crashing does not affect the class functionality.

Any other bug unrelated to the race condition of a missing devnode would be lost. Catching all exceptions is bad practice.

If d = device.InputDevice(path) fails due to some other problem, developers want to see that stack trace.

[antheas]

I will leave it up to you on how to handle the _find_device() behavior.

In order for this bug to be resolved, you either have to make sure you never throw on the Uinput constructor, or if you do, to close the file descriptor below before returning.
https://github.com/gvalkov/python-evdev/blob/main/evdev/uinput.py#L134

According to the caller hint for _find_device(), it should return None when a device is not found, so catching all exceptions is the correct backwards compatible behavior here.

        #: An :class:`InputDevice <evdev.device.InputDevice>` instance
        #: for the fake input device. ``None`` if the device cannot be
        #: opened for reading and writing.
        self.device = self._find_device()

Even if that hides other bugs related to the InputDevice constructor, when called from _find_device(). I would also think that obscure permissions errors on certain devices (unrelated to the uinput one) can also cause _find_device() to raise, which would break downstream libraries.

[antheas]

Referencing the other thread, it may be preferable to completely rewrite _find_device() and always make it throw if the correct device is not found.

Since the downstream device is not required for correct behavior, you should remove the self.device instantiation from the constructor, and lazily initialize it in the functions that use it, so that libraries that rely on the device existing can correctly handle it missing and get a proper stack trace.

I, for one, do not use those functions, so I monkey patch _find_device() to be blank so that I avoid all errors related to it with the current buggy version.

[sezanzeb]

or if you do, to close the file descriptor below before returning.

Or use a destructor

class A:
    def __del__(self):
        print('__del__')

    def foo(self):
        raise Exception('foo')

A().foo()

will print

__del__
Traceback (most recent call last):
  File "/home/mango/.config/JetBrains/PyCharmCE2023.2/scratches/scratch_6.py", line 8, in <module>
    A().foo()
  File "/home/mango/.config/JetBrains/PyCharmCE2023.2/scratches/scratch_6.py", line 6, in foo
    raise Exception('foo')
Exception: foo

In this destructor, the file descriptor could be closed.

I wonder if this could break existing code. Would any project out there expect the uinput to remain open if the UInput object is garbage collected?

[sezanzeb]

Since the downstream device is not required for correct behavior, you should remove the self.device instantiation from the constructor, and lazily initialize it in the functions that use it, so that libraries that rely on the device existing can correctly handle it missing and get a proper stack trace.

I'm afraid this might break existing projects that handle errors that originate in _find_device when the UInput object is constructed.

[sezanzeb]

But why again do we want to close the fd to /dev/uinput/ if the matching InputDevice (for example /dev/input/event5 cannot be opened?

[antheas]

Because that descriptor is stored on self.fd of an object that is never created.

If the constructor throws, it can never be closed.

[antheas]

Do destructors run if the init method does not finish? If it does it may be an option.

[sezanzeb]

ah, right, thanks.

Do destructors run if the init method does not finish? If it does it may be an option.

yes

[antheas]

Otherwise, try... except on the constructor.

then raise e again

[sezanzeb]

destructor sounds cleaner to me.

By the way, finally can be used to avoid re-throwing the exception.

try:
    raise Exception('foo')
finally:
    print("finally")

will print

finally
Traceback (most recent call last):
  File "/home/mango/.config/JetBrains/PyCharmCE2023.2/scratches/scratch_6.py", line 2, in <module>
    raise Exception('foo')
Exception: foo

[antheas]

You can not assume fd will exist in the destructor. But you can use something like the following.

if hasattr(self, 'fd'):
    fd = getattr(self, 'fd')
    if fd:
        os.close(fd)

In general, destructors break KeyboardInterrupts so I had to remove Interrupts from my app (hhd). But they are convenient I will agree.

[antheas]

Finally will run regardless, we do not want that.

[sezanzeb]

Finally will run regardless, we do not want that.

whoops