libwayland: Add WAYLAND_DEBUG_INTERFACES

Submitted by wl@ongy.net on June 29, 2018, 8:43 a.m.

Details

Message ID 20180629084328.31681-1-wl@ongy.net
State New
Series "libwayland: Add WAYLAND_DEBUG_INTERFACES"
Headers show

Commit Message

wl@ongy.net June 29, 2018, 8:43 a.m.
From: Markus Ongyerth <wl@ongy.net>

Add environment variable WAYLAND_DEBUG_INTERFACES for filtering the
output of WAYLAND_DEBUG logs.
While WAYLAND_DEBUG is a pretty powerful and useful debug tool, printing
everything has a few downsides.

1) It's a full keylogger (getting debug-logs from users)
2) It can be overly spammy with wl_buffer/wl_surface actions (e.g. when
   playing a video))

With this addition it's possible to supply another environment
variable, to filter on the interfaces one is interested in.
E.g. when interested in the behaviour of xdg-shell popups the filter could be
WAYLAND_DEBUG_INTERFACES=xdg_positioner,xdg_surface,xdg_popup
greatly improving SNR on the output and hiding potentially sensitive
information such as keystrokes.
---
 src/wayland-client.c | 85 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 81 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/wayland-client.c b/src/wayland-client.c
index efeb745..9de8e4e 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -118,6 +118,31 @@  struct wl_display {
 /** \endcond */
 
 static int debug_client = 0;
+static size_t debug_client_count = 0;
+static char **debug_client_interfaces = NULL;
+
+static void
+wl_client_debug_print(struct wl_closure *closure,
+                      struct wl_object *target, int send)
+{
+	if (debug_client_interfaces) {
+		bool found = false;
+		size_t i;
+		for (i = 0; i < debug_client_count; ++i) {
+			if (!strcmp(target->interface->name,
+			            debug_client_interfaces[i])) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			return;
+		}
+	}
+
+	wl_closure_print(closure, target, send);
+}
 
 /**
  * This helper function wakes up all threads that are
@@ -748,7 +773,7 @@  wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,
 		wl_abort("Error marshalling request: %s\n", strerror(errno));
 
 	if (debug_client)
-		wl_closure_print(closure, &proxy->object, true);
+		wl_client_debug_print(closure, &proxy->object, true);
 
 	if (wl_closure_send(closure, proxy->display->connection))
 		wl_abort("Error sending request: %s\n", strerror(errno));
@@ -1016,6 +1041,56 @@  connect_to_socket(const char *name)
 	return fd;
 }
 
+/* Set up the filter list for WAYLAND_DEBUG output.
+ * This reads WAYLAND_DEBUG_INTERFACEs and splits the provided string on every
+ * ','.
+ * The resulting list of strings is used as whitelist for interfaces printed by
+ * the WAYLAND_DEBUG mechanism.
+ * Sadly this leaks the memory required to strdup() the environment variable,
+ * but since this is a debug feature, and it should be constant over the
+ * lifetime of a single process, I consider it a minor problem
+ */
+static void
+setup_interface_filter(void)
+{
+	char *saveptr;
+	char *token;
+	size_t count = 0;
+	size_t i;
+	char *env;
+
+	/* We set this up before (on another wl_display_connect probably) so we
+	 * don't have to do anything this time
+	 */
+	if (debug_client_interfaces)
+		return;
+
+	if (!(env = getenv("WAYLAND_DEBUG_INTERFACES")))
+		return;
+
+	if (!(env = strdup(env))) {
+		wl_log("error: Could not allocate memory for WAYLAND_DEBUG_INTERFACES\n");
+		return;
+	}
+
+	for (i = 0; env[i]; ++i) {
+		if (env[i] == ',')
+			++count;
+	}
+
+	/* The maximum possible interfaces is the number of ',' found + 1 */
+	debug_client_interfaces = calloc(count + 1, sizeof(char *));
+	if (!debug_client_interfaces) {
+		wl_log("error: Could not allocate memory for WAYLAND_DEBUG_INTERFACES\n");
+		free(env);
+		return;
+	}
+
+	for (token = strtok_r(env, ",", &saveptr); token; token = strtok_r(NULL, ",", &saveptr)) {
+		debug_client_interfaces[debug_client_count++] = token;
+	}
+}
+
 /** Connect to Wayland display on an already open fd
  *
  * \param fd The fd to use for the connection
@@ -1034,8 +1109,10 @@  wl_display_connect_to_fd(int fd)
 	const char *debug;
 
 	debug = getenv("WAYLAND_DEBUG");
-	if (debug && (strstr(debug, "client") || strstr(debug, "1")))
+	if (debug && (strstr(debug, "client") || strstr(debug, "1"))) {
 		debug_client = 1;
+		setup_interface_filter();
+	}
 
 	display = zalloc(sizeof *display);
 	if (display == NULL) {
@@ -1423,13 +1500,13 @@  dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
 
 	if (proxy->dispatcher) {
 		if (debug_client)
-			wl_closure_print(closure, &proxy->object, false);
+			wl_client_debug_print(closure, &proxy->object, false);
 
 		wl_closure_dispatch(closure, proxy->dispatcher,
 				    &proxy->object, opcode);
 	} else if (proxy->object.implementation) {
 		if (debug_client)
-			wl_closure_print(closure, &proxy->object, false);
+			wl_client_debug_print(closure, &proxy->object, false);
 
 		wl_closure_invoke(closure, WL_CLOSURE_INVOKE_CLIENT,
 				  &proxy->object, opcode, proxy->user_data);

Comments

wl@ongy.net June 29, 2018, 8:46 a.m.
Urgh, of course I forget to mention something when I write mails.

This is current RFC quality, not actual patch. Should this be something people 
are interested in, I think it should live mostly in connection.c and be 
supported by both -server and -client libraries.

I'm also not sure whether the supplied whitelist approach is the best way to 
go, or this should be a blacklist in practice.

On 2018/6月/29 10:43, wl@ongy.net wrote:
> From: Markus Ongyerth <wl@ongy.net>
> 
> Add environment variable WAYLAND_DEBUG_INTERFACES for filtering the
> output of WAYLAND_DEBUG logs.
> While WAYLAND_DEBUG is a pretty powerful and useful debug tool, printing
> everything has a few downsides.
> 
> 1) It's a full keylogger (getting debug-logs from users)
> 2) It can be overly spammy with wl_buffer/wl_surface actions (e.g. when
>    playing a video))
> 
> With this addition it's possible to supply another environment
> variable, to filter on the interfaces one is interested in.
> E.g. when interested in the behaviour of xdg-shell popups the filter could be
> WAYLAND_DEBUG_INTERFACES=xdg_positioner,xdg_surface,xdg_popup
> greatly improving SNR on the output and hiding potentially sensitive
> information such as keystrokes.
> ---
>  src/wayland-client.c | 85 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index efeb745..9de8e4e 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -118,6 +118,31 @@ struct wl_display {
>  /** \endcond */
>  
>  static int debug_client = 0;
> +static size_t debug_client_count = 0;
> +static char **debug_client_interfaces = NULL;
> +
> +static void
> +wl_client_debug_print(struct wl_closure *closure,
> +                      struct wl_object *target, int send)
> +{
> +	if (debug_client_interfaces) {
> +		bool found = false;
> +		size_t i;
> +		for (i = 0; i < debug_client_count; ++i) {
> +			if (!strcmp(target->interface->name,
> +			            debug_client_interfaces[i])) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found) {
> +			return;
> +		}
> +	}
> +
> +	wl_closure_print(closure, target, send);
> +}
>  
>  /**
>   * This helper function wakes up all threads that are
> @@ -748,7 +773,7 @@ wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,
>  		wl_abort("Error marshalling request: %s\n", strerror(errno));
>  
>  	if (debug_client)
> -		wl_closure_print(closure, &proxy->object, true);
> +		wl_client_debug_print(closure, &proxy->object, true);
>  
>  	if (wl_closure_send(closure, proxy->display->connection))
>  		wl_abort("Error sending request: %s\n", strerror(errno));
> @@ -1016,6 +1041,56 @@ connect_to_socket(const char *name)
>  	return fd;
>  }
>  
> +/* Set up the filter list for WAYLAND_DEBUG output.
> + * This reads WAYLAND_DEBUG_INTERFACEs and splits the provided string on every
> + * ','.
> + * The resulting list of strings is used as whitelist for interfaces printed by
> + * the WAYLAND_DEBUG mechanism.
> + * Sadly this leaks the memory required to strdup() the environment variable,
> + * but since this is a debug feature, and it should be constant over the
> + * lifetime of a single process, I consider it a minor problem
> + */
> +static void
> +setup_interface_filter(void)
> +{
> +	char *saveptr;
> +	char *token;
> +	size_t count = 0;
> +	size_t i;
> +	char *env;
> +
> +	/* We set this up before (on another wl_display_connect probably) so we
> +	 * don't have to do anything this time
> +	 */
> +	if (debug_client_interfaces)
> +		return;
> +
> +	if (!(env = getenv("WAYLAND_DEBUG_INTERFACES")))
> +		return;
> +
> +	if (!(env = strdup(env))) {
> +		wl_log("error: Could not allocate memory for WAYLAND_DEBUG_INTERFACES\n");
> +		return;
> +	}
> +
> +	for (i = 0; env[i]; ++i) {
> +		if (env[i] == ',')
> +			++count;
> +	}
> +
> +	/* The maximum possible interfaces is the number of ',' found + 1 */
> +	debug_client_interfaces = calloc(count + 1, sizeof(char *));
> +	if (!debug_client_interfaces) {
> +		wl_log("error: Could not allocate memory for WAYLAND_DEBUG_INTERFACES\n");
> +		free(env);
> +		return;
> +	}
> +
> +	for (token = strtok_r(env, ",", &saveptr); token; token = strtok_r(NULL, ",", &saveptr)) {
> +		debug_client_interfaces[debug_client_count++] = token;
> +	}
> +}
> +
>  /** Connect to Wayland display on an already open fd
>   *
>   * \param fd The fd to use for the connection
> @@ -1034,8 +1109,10 @@ wl_display_connect_to_fd(int fd)
>  	const char *debug;
>  
>  	debug = getenv("WAYLAND_DEBUG");
> -	if (debug && (strstr(debug, "client") || strstr(debug, "1")))
> +	if (debug && (strstr(debug, "client") || strstr(debug, "1"))) {
>  		debug_client = 1;
> +		setup_interface_filter();
> +	}
>  
>  	display = zalloc(sizeof *display);
>  	if (display == NULL) {
> @@ -1423,13 +1500,13 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>  
>  	if (proxy->dispatcher) {
>  		if (debug_client)
> -			wl_closure_print(closure, &proxy->object, false);
> +			wl_client_debug_print(closure, &proxy->object, false);
>  
>  		wl_closure_dispatch(closure, proxy->dispatcher,
>  				    &proxy->object, opcode);
>  	} else if (proxy->object.implementation) {
>  		if (debug_client)
> -			wl_closure_print(closure, &proxy->object, false);
> +			wl_client_debug_print(closure, &proxy->object, false);
>  
>  		wl_closure_invoke(closure, WL_CLOSURE_INVOKE_CLIENT,
>  				  &proxy->object, opcode, proxy->user_data);
> -- 
> 2.18.0
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Pekka Paalanen June 29, 2018, 10:21 a.m.
On Fri, 29 Jun 2018 10:46:16 +0200
Markus Ongyerth <wl@ongy.net> wrote:

> Urgh, of course I forget to mention something when I write mails.
> 
> This is current RFC quality, not actual patch. Should this be something people 
> are interested in, I think it should live mostly in connection.c and be 
> supported by both -server and -client libraries.
> 
> I'm also not sure whether the supplied whitelist approach is the best way to 
> go, or this should be a blacklist in practice.
> 
> On 2018/6月/29 10:43, wl@ongy.net wrote:
> > From: Markus Ongyerth <wl@ongy.net>
> > 
> > Add environment variable WAYLAND_DEBUG_INTERFACES for filtering the
> > output of WAYLAND_DEBUG logs.
> > While WAYLAND_DEBUG is a pretty powerful and useful debug tool, printing
> > everything has a few downsides.
> > 
> > 1) It's a full keylogger (getting debug-logs from users)
> > 2) It can be overly spammy with wl_buffer/wl_surface actions (e.g. when
> >    playing a video))
> > 
> > With this addition it's possible to supply another environment
> > variable, to filter on the interfaces one is interested in.
> > E.g. when interested in the behaviour of xdg-shell popups the filter could be
> > WAYLAND_DEBUG_INTERFACES=xdg_positioner,xdg_surface,xdg_popup
> > greatly improving SNR on the output and hiding potentially sensitive
> > information such as keystrokes.
> > ---
> >  src/wayland-client.c | 85 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 81 insertions(+), 4 deletions(-)

Hi,

I'm not fond of this.

Whether it is a white-list or a black-list, it will be hard to use. You
cannot be sure which interfaces will actually be interesting, which is
likely to result in asking the user to trace multiple times trying to
get the filter right. There might be interfaces you didn't know of
existing. It is much better to capture everything and filter while you
analyze the dump.

For example, you almost always need wl_surface messages, because so
many things get latched in on wl_surface.commit.

I'm not concerned about the keylogger aspect much. Presumably most use
cases are relatively fast to reproduce so the user can simply avoid
inputting anything sensitive, knowing that e.g. key presses will be
logged (the developer asking for a log should note this if there is any
danger of leaking sensitive data).

libwayland also does not understand the interfaces, so any kind of more
intelligent analysis must be done with other tools. We do have
middle-man protocol logger tools. If key logging is a concern, I would
suggest to have a middle-man logger enhanced to censor the key codes in
key events.


Thanks,
pq