Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 340bd9b6 authored by Harry Cutts's avatar Harry Cutts
Browse files

EventHub: abort if device disappears while opening 

Connecting an input device for a brief period of time — long enough
for EventHub::openDeviceLocked to open its FD and give it a class, but
not long enough for it to cache all of its absolute axis information —
can cause InputReader to crash. This happens when a mapper looks up
information for an absolute axis that it should be able to depend on,
but finds it missing. For example, TouchpadInputMapper depends on the
device having ABS_MT_POSITION_X and _Y axes, since these are required
for the device to receive InputDeviceClass::TOUCHPAD, but if this
race condition occurs those axes may not be present.

Since we're already caching all of the absolute axis information during
opening, we can just stop opening the device if we detect that it's
gone, allowing mappers to safely assume that all of the absolute axes
required for a device to be given its class will have information
available through EventHub::getAbsoluteAxisInfo.

It's still possible that the device could be removed after
openDeviceLocked finishes but before the mappers initialize themselves
using the axis information. However, in that case the information will
already have been cached, so the worst that will happen is that we'll
initialize some mappers unnecessarily and then immediately destroy them.

Test: create test-fast-disconnect.json with the following contents:
      {
          "id": 1, "command": "register",
          "name": "BADLY CONNECTED TOUCHPAD",
          "vid": 0x18d1, "pid": 0xbaad, "bus": "usb",
          "configuration": [
              {"type": "UI_SET_EVBIT", "data":["EV_KEY", "EV_ABS"]},
              {"type": "UI_SET_KEYBIT",
	       "data": ["BTN_LEFT", "BTN_TOUCH", "BTN_TOOL_FINGER"]},
              {"type": "UI_SET_ABSBIT",
	       "data": ["ABS_X", "ABS_Y", "ABS_MT_SLOT",
	                "ABS_MT_POSITION_X", "ABS_MT_POSITION_Y",
		        "ABS_MT_TRACKING_ID"]},
              {"type": "UI_SET_PROPBIT",
	       "data": ["INPUT_PROP_POINTER", "INPUT_PROP_BUTTONPAD"]}
          ],
          "abs_info": [
              {"code": "ABS_X",
	       "info": {"value":0, "minimum":-255, "maximum":255,
	                "fuzz":0, "flat":0, "resolution":1}},
              {"code": "ABS_Y",
	       "info": {"value":0, "minimum":-255, "maximum":255,
	                "fuzz":0, "flat":0, "resolution":1}},
              {"code": "ABS_MT_SLOT",
	       "info": {"value":0, "minimum":0, "maximum":4, "fuzz":0,
	                "flat":0, "resolution":0}},
              {"code": "ABS_MT_POSITION_X",
	       "info": {"value":0, "minimum":0, "maximum":1, "fuzz":0,
	                "flat":0, "resolution":1}},
              {"code": "ABS_MT_POSITION_Y",
	       "info": {"value":0, "minimum":0, "maximum":1, "fuzz":0,
	                "flat":0, "resolution":1}},
              {"code": "ABS_MT_TRACKING_ID",
	       "info": {"value":0, "minimum":0, "maximum":65535,
	                "fuzz":0, "flat":0, "resolution":0}}
          ]
      }
      Then pipe it to uinput:
      $ adb shell uinput - < test-fast-disconnect.json
      and check that the device does not crash. Check that the "removed
      while opening" line is logged, to confirm that the fix is taking
      effect rather than the timing just being nice for this case. (You
      may have to repeat the uinput command a few times to see this.)
Bug: 424789999
Flag: com.android.input.flags.abort_device_opening_on_enodev
Change-Id: I8d7468eddd64854ff5b15986815999969388b375
parent d12ae836

libs/input/input_flags.aconfig

+10 −0
Original line number Diff line number Diff line
@@ -255,3 +255,13 @@ flag { 
    purpose: PURPOSE_BUGFIX

  }

}



flag {

  name: "abort_device_opening_on_enodev"

  namespace: "input"

  description: "Stop opening a device if it is removed before we can cache all of its axis information"

  bug: "424789999"

  metadata {

    purpose: PURPOSE_BUGFIX

  }

}

services/inputflinger/reader/EventHub.cpp

+24 −8
Original line number Diff line number Diff line
@@ -34,14 +34,14 @@ 
#include <sys/sysmacros.h>

#include <unistd.h>



#include <android_companion_virtualdevice_flags.h>



#define LOG_TAG "EventHub"



// #define LOG_NDEBUG 0

#include <android-base/file.h>

#include <android-base/stringprintf.h>

#include <android-base/strings.h>

#include <android_companion_virtualdevice_flags.h>

#include <com_android_input_flags.h>

#include <cutils/properties.h>

#include <ftl/enum.h>

#include <input/InputEventLabels.h>
@@ -72,6 +72,7 @@ using android::base::StringPrintf; 


namespace android {



namespace input_flags = com::android::input::flags;

namespace vd_flags = android::companion::virtualdevice::flags;



using namespace ftl::flag_operators;
@@ -632,7 +633,7 @@ status_t EventHub::Device::readDeviceBitMask(unsigned long ioctlCode, BitArray<N 
    return ret;

}



void EventHub::Device::configureFd() {

bool EventHub::Device::configureFd() {

    // Set fd parameters with ioctl, such as key repeat, suspend block, and clock type

    if (classes.test(InputDeviceClass::KEYBOARD)) {

        // Disable kernel key repeat since we handle it ourselves
@@ -656,10 +657,10 @@ void EventHub::Device::configureFd() { 
    ALOGI("usingClockIoctl=%s", toString(usingClockIoctl));



    // Query the initial state of keys and switches, which is tracked by EventHub.

    readDeviceState();

    return readDeviceState();

}



void EventHub::Device::readDeviceState() {

bool EventHub::Device::readDeviceState() {

    if (readDeviceBitMask(EVIOCGKEY(0), keyState) < 0) {

        ALOGD("Unable to query the global key state for %s: %s", path.c_str(), strerror(errno));

    }
@@ -668,10 +669,10 @@ void EventHub::Device::readDeviceState() { 
    }



    // Read absolute axis info and values for all available axes for the device.

    populateAbsoluteAxisStates();

    return populateAbsoluteAxisStates();

}



void EventHub::Device::populateAbsoluteAxisStates() {

bool EventHub::Device::populateAbsoluteAxisStates() {

    absState.clear();



    for (int axis = 0; axis <= ABS_MAX; axis++) {
@@ -684,6 +685,10 @@ void EventHub::Device::populateAbsoluteAxisStates() { 
                  identifier.name.c_str(),

                  InputEventLookup::getLinuxEvdevLabel(EV_ABS, axis, 0).code.c_str(), fd,

                  strerror(errno));

            if (input_flags::abort_device_opening_on_enodev() && errno == ENODEV) {

                // The device no longer exists. There's no point trying to query any more axes.

                return false;

            }

            continue;

        }

        auto& [axisInfo, value] = absState[axis];
@@ -694,6 +699,7 @@ void EventHub::Device::populateAbsoluteAxisStates() { 
        axisInfo.resolution = info.resolution;

        value = info.value;

    }

    return true;

}



bool EventHub::Device::hasKeycodeLocked(int keycode) const {
@@ -2631,7 +2637,17 @@ void EventHub::openDeviceLocked(const std::string& devicePath) { 
        return;

    }



    device->configureFd();

    if (!device->configureFd()) {

        // The device may have been removed since we queried its bitmasks earlier in this method. In

        // that case, the absolute axis info may not have been cached correctly, leading to us

        // trying to create mappers that depend on axis information we don't have. For example, we

        // may have decided on InputDeviceClass::TOUCH because ABS_MT_POSITION_X and _Y are in

        // device->absBitmask, but when we then query them in an input mapper they'll be missing,

        // causing a crash. To prevent this, return early without adding the device.

        ALOGE("Device '%s' (%s) was removed while opening. Dropping it.",

              device->identifier.name.c_str(), devicePath.c_str());

        return;

    }



    ALOGI("New device: id=%d, fd=%d, path='%s', name='%s', classes=%s, "

          "configuration='%s', keyLayout='%s', keyCharacterMap='%s', builtinKeyboard=%s, ",

services/inputflinger/reader/include/EventHub.h

+10 −3
Original line number Diff line number Diff line
@@ -701,8 +701,16 @@ private: 
        template <std::size_t N>

        status_t readDeviceBitMask(unsigned long ioctlCode, BitArray<N>& bitArray);



        void configureFd();

        void populateAbsoluteAxisStates();

        /**

         * Configures the device's FD and caches its current state.

         *

         * Returns false if an error is encountered that means the caller should not continue to try

         * opening the device (e.g. if the device no longer exists).

         */

        bool configureFd();

        bool readDeviceState();

        bool populateAbsoluteAxisStates();



        bool hasKeycodeLocked(int keycode) const;

        bool hasKeycodeInternalLocked(int keycode) const;

        bool loadVirtualKeyMapLocked();
@@ -715,7 +723,6 @@ private: 


        bool currentFrameDropped;

        void trackInputEvent(const struct input_event& event);

        void readDeviceState();

    };



    /**