[PATCH v2 libevdev] Enable event logging on request
David Herrmann
dh.herrmann at gmail.com
Fri Apr 4 13:04:01 PDT 2014
Hi
On Fri, Apr 4, 2014 at 4:54 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> Check an environment variable in the form for
> LIBEVDEV_LOG_EVENTS="error"
> or
> LIBEVDEV_LOG_EVENTS="info:event0,event3"
>
> If set, log all or the selected devices to the normal event logger, with the
> given priority.
>
> Key codes in the normal alphanumeric range are obfuscated to avoid accidental
> leakage of information, higher-order keys are sent as normal.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> Changes to v1:
> - use the event node name instead of the minor number, avoids confusion
> - add tests
>
> libevdev/libevdev-int.h | 5 +
> libevdev/libevdev.c | 164 ++++++++++++++++++++++++++
> libevdev/libevdev.h | 23 ++++
> test/test-libevdev-events.c | 275 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 467 insertions(+)
>
> diff --git a/libevdev/libevdev-int.h b/libevdev/libevdev-int.h
> index 98c75ce..5a164bd 100644
> --- a/libevdev/libevdev-int.h
> +++ b/libevdev/libevdev-int.h
> @@ -107,6 +107,11 @@ struct libevdev {
> unsigned long *tracking_id_changes;
> size_t tracking_id_changes_sz; /* in bytes */
> } mt_sync;
> +
> + struct {
> + char *device_node;
> + enum libevdev_log_priority priority;
> + } debug;
> };
>
> struct logdata {
> diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> index 96bd41b..74fd935 100644
> --- a/libevdev/libevdev.c
> +++ b/libevdev/libevdev.c
> @@ -21,13 +21,16 @@
> */
>
> #include <config.h>
> +#include <dirent.h>
> #include <errno.h>
> +#include <limits.h>
> #include <poll.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <stdarg.h>
> #include <stdbool.h>
> +#include <sys/stat.h>
>
> #include "libevdev.h"
> #include "libevdev-int.h"
> @@ -44,6 +47,32 @@ enum event_filter_status {
>
> static int sync_mt_state(struct libevdev *dev, int create_events);
>
> +static inline void
> +log_event(struct libevdev *dev, int flags, const struct input_event *ev)
> +{
> + const char *mode;
> + const char *type, *code;
> +
> + if (dev->debug.priority == 0)
> + return;
> +
> + if (flags & LIBEVDEV_READ_FLAG_SYNC)
> + mode = "S";
> + else if (flags & LIBEVDEV_READ_FLAG_NORMAL)
> + mode = "N";
> + else
> + mode = "I";
> +
> + type = libevdev_event_type_get_name(ev->type);
> + if (ev->type == EV_KEY && ev->code < KEY_STOP)
> + code = "OBFUSCATED";
> + else
> + code = libevdev_event_code_get_name(ev->type, ev->code);
> +
> + log_msg_cond(dev->debug.priority, "%s: EVENT(%s): %s %s %d\n",
> + dev->debug.device_node, mode, type, code, ev->value);
This chains the default log-level test here. So LIBEVDEV_LOG_EVENTS
still requires a suitable LIBEVDEV_LOG_LEVEL. Is that intentional?
Wasn't clear to me from the documentation, you might wanna add that
(or maybe I missed it, then I'm sorry..).
Or how about forcing the log-level to DEBUG in case LIBEVDEV_LOG_EVENTS is set?
> +}
> +
> static inline int*
> slot_value(const struct libevdev *dev, int slot, int axis)
> {
> @@ -161,6 +190,7 @@ libevdev_reset(struct libevdev *dev)
> free(dev->mt_sync.mt_state);
> free(dev->mt_sync.tracking_id_changes);
> free(dev->mt_sync.slot_update);
> + free(dev->debug.device_node);
> memset(dev, 0, sizeof(*dev));
> dev->fd = -1;
> dev->initialized = false;
> @@ -235,6 +265,129 @@ libevdev_get_log_priority(void)
> return log_data.priority;
> }
>
> +static int
> +is_event_device(const struct dirent *dir) {
> + return strncmp("event", dir->d_name, 5) == 0;
> +}
> +
> +static char*
> +get_event_node(struct libevdev *dev)
> +{
> + struct dirent **entries;
> + int nentries, i;
> + struct stat st;
> + dev_t rdev;
> + char *node = NULL;
> +
> + if (fstat(dev->fd, &st) == -1)
> + return NULL;
> +
> + rdev = st.st_rdev;
> +
> + nentries = scandir("/dev/input/",
> + &entries,
> + is_event_device,
> + versionsort);
> + if (nentries == -1)
> + return NULL;
> +
> + for (i = 0; i < nentries; i++) {
> + char path[PATH_MAX];
> +
> + snprintf(path, sizeof(path), "/dev/input/%s", entries[i]->d_name);
> + if (stat(path, &st) == -1)
> + continue;
> +
> + if (st.st_rdev == rdev) {
> + node = strdup(entries[i]->d_name);
> + break;
> + }
> + }
> +
> + for (i = 0; i < nentries; i++)
> + free(entries[i]);
> + free(entries);
What are you doing? You iterate "/dev/input/" until you find an
event-node with the same major-minor than the fd? Why not use:
sprintf(buf, "event%d", minor(st.st_rdev));
and be done with it. You might wanna use "access(.., F_OK)" to see
whether the node really exists, but even that is not needed, imho.
- all nodes have _hardcoded_ names in /sys (/sys/class/input/eventXY)
and these IDs are guaranteed to be equal to the minor-ID!
- your code _filters_ for nodes called "eventX"
- the kernel _hardcodes_ the input/event<minor> path
=> The only reason the node doesn't exist but we need that scanning,
is if someone _intentionally_ destroys device-nodes and recreates with
different IDs. In that case, the 'eventXY'-nodes in /dev/input/ would
differ from the names in /sys/class/input/. That is so _wrong_ that I
don't think we should ever encourage people to do that.
> +
> + return node;
> +}
> +
> +static void
> +enable_event_logging(struct libevdev *dev)
I think "enable_event_logging()" is a misleading name here as it gives
the impression it enables it _unconditionally_. I don't have any
better idea, though. So up to the native speak to decide..
> +{
> + enum libevdev_log_priority pri = 0;
> + const char *env;
> + char *tmp = NULL,
> + *tok = NULL, *saveptr;
> + char *node;
> +
> + env = getenv("LIBEVDEV_LOG_EVENTS");
Are you sure you don't want to use secure_getenv()? Yes, we obfuscate
most input, but I still think it's wrong to allow users to trigger
that without privileges.
> + if (!env || strlen(env) == 0)
> + return;
> +
> + if (dev->debug.device_node)
> + free(dev->debug.device_node);
Please add "dev->debug.device_node = NULL;", otherwise your
error-paths are wrong and you trigger double-free assertions.
> +
> + node = get_event_node(dev);
> + if (!node) {
> + log_error("Failed to get device node for debugging\n");
> + return;
> + }
> +
> + if (strncmp(env, "error", 5) == 0) {
> + pri = LIBEVDEV_LOG_ERROR;
> + tok = (char*)&env[5];
> + } else if (strncmp(env, "info", 4) == 0) {
> + pri = LIBEVDEV_LOG_INFO;
> + tok = (char*)&env[4];
> + } else if (strncmp(env, "debug", 5) == 0) {
> + pri = LIBEVDEV_LOG_DEBUG;
> + tok = (char*)&env[5];
> + }
> +
> + if (tok && *tok == '\0') {
> + dev->debug.device_node = node;
> + dev->debug.priority = pri;
> + return;
> + }
> +
> + if (!tok || *tok != ':') {
> + log_error("Invalid value '%s' for $LIBEVDEV_LOG_EVENTS\n", env);
> + free(node);
> + return;
> + }
> +
> + tmp = strdup(tok + 1);
> + if (!tmp) {
> + log_error("Allocation failure, debugging disabled\n");
> + free(node);
> + return;
> + }
> +
> + tok = strtok_r(tmp, ",", &saveptr);
> +
> + while (tok) {
> + if (strncmp(tok, "event", 5) != 0) {
> + log_error("Invalid value '%s' for $LIBEVDEV_LOG_EVENTS\n", env);
> + break;
> + }
> +
> + if (strncmp(tok, node, strlen(node)) == 0) {
tok = "event15"
node = "event1"
=> false positive
You need to test "tok[strlen(node)] == 0".
Btw., strtok_r() sets a terminating zero, right? (otherwise, the
strdup() would be redundant..) So why not use strcmp()?
> + dev->debug.device_node = node;
> + dev->debug.priority = pri;
> + break;
> + }
> +
> + tok = strtok_r(NULL, ",", &saveptr);
> + }
> +
> + free(tmp);
> +
> + if (!dev->debug.device_node)
> + free(node);
> +
> + return;
> +}
> +
> LIBEVDEV_EXPORT int
> libevdev_change_fd(struct libevdev *dev, int fd)
> {
> @@ -242,7 +395,11 @@ libevdev_change_fd(struct libevdev *dev, int fd)
> log_bug("device not initialized. call libevdev_set_fd() first\n");
> return -1;
> }
> +
> dev->fd = fd;
> +
> + enable_event_logging(dev);
> +
> return 0;
> }
>
> @@ -390,6 +547,8 @@ libevdev_set_fd(struct libevdev* dev, int fd)
>
> dev->fd = fd;
>
> + enable_event_logging(dev);
> +
> /* devices with ABS_MT_SLOT - 1 aren't MT devices,
> see the documentation for multitouch-related
> functions for more details */
> @@ -957,6 +1116,9 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> of the device too */
> while (queue_shift(dev, &e) == 0) {
> dev->queue_nsync--;
> +
> + log_event(dev, 0, ev);
> +
> if (sanitize_event(dev, &e, dev->sync_state) != EVENT_FILTER_DISCARD)
> update_state(dev, &e);
> }
> @@ -988,6 +1150,8 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> if (queue_shift(dev, ev) != 0)
> return -EAGAIN;
>
> + log_event(dev, flags, ev);
> +
> filter_status = sanitize_event(dev, ev, dev->sync_state);
> if (filter_status != EVENT_FILTER_DISCARD)
> update_state(dev, ev);
> diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> index aa7b90d..c20ed84 100644
> --- a/libevdev/libevdev.h
> +++ b/libevdev/libevdev.h
> @@ -151,6 +151,29 @@ extern "C" {
> * libevdev is licensed under the
> * [X11 license](http://cgit.freedesktop.org/libevdev/tree/COPYING).
> *
> + * Debugging events
> + * ================
> + * libevdev checks the LIBEVDEV_LOG_EVENTS environment variable during
> + * libevdev_set_fd() and libevdev_change_fd(). If set to "error", "info", or
> + * "debug", all events are logged with the respective priority before they
> + * are passed to the client. This is useful to check event sequences that
> + * happen before a specific bug in the higher layers of the stack
> + *
> + * To debug specific devices only, the event node may be provided as a
> + * comma-separated list after the priority:
> + * LIBEVDEV_LOG_EVENTS="error:event0,event5,event3" logs all events from the
> + * devices with the given /dev/input/event nodes. This requires that the
> + * libevdev process is able to iterate and stat each file in the /dev/input
> + * directory. An invalid value of the variable or unsuccessful attempt to
> + * initialize logging for a device will be logged as error. Debugging is
> + * disabled (for this device, if appropriate), otherwise libevdev continues
> + * normally.
> + *
> + * Events logged by libevdev are likely to end up in public bug reports. For
> + * this reason key event codes obfuscated to protect accidental leakage of
> + * private information. Key codes between KEY_ESC up to including
> + * KEY_COMPOSE are always logged as "OBFUSCATED".
> + *
I'm still not big fan of it, but that's personal taste. Acked-by-me
and if you fix the issues, I can do another review. Code looks fine
besides the minor issues I pointed out.
Thanks!
David
More information about the Input-tools
mailing list