diff mbox

lib_wpa_client: Add a reference counter on wpa_ctrl.

Message ID 4ED7A9645AB51849A442949141BBE1F5C71B34@IRSMSX102.ger.corp.intel.com
State Rejected
Headers show

Commit Message

Poulain, LoicX July 19, 2013, 3:26 p.m. UTC
Hello Jouni, list

In response to thread "lib_wpa_client: making it thread safe?"

We would like to propose the following patch which adds a reference counter on wpa_ctrl. We are going to integrate this patch in the Intel Android tree.
The goal of this patch is to prevent a whole class crashes caused by freeing the wpa_ctrl while it's still in use by another thread.
e.g in Android, this is typically the case, many crashes are reported due to libhardware_legacy (wifi) usage of this library.

We had the following constraints in mind when writing the patch:
- This patch should not block any thread.
- This patch should not modify the lib API.
- This patch should not modify the wpa_ctrl structure.
- Modification are internal to the lib.
- This feature can be enabled/disabled with a C flag: CONFIG_CTRL_REF_COUNT
- This patch should fix the "issue" at lib level, avoiding implement workarounds at upper level

General principle:
- A wpa_ctrl resource cannot be freed if it still used by another thread.
- A wpa_ctrl resource will be freed when closed and no longer used.
- Prevent crashes If a lib function is used with an invalid/no longer valid wpa_ctrl identifier.
- Main original code modifications consist in calling 'get' and 'put' on wpa_ctrl.

We welcome any improvements you may have on this patch. 

BR,
Loïc

From: Loic Poulain <loicx.poulain@intel.com>

This library provides a wpa_ctrl pointer to third parties. This pointer
is used as a context identifier. But third parties can use this library
asynchronously. So, if a third party closes a ctrl which is already used
by an other request (ex: wpa_ctrl_request), then this context will no longer
be valid, leading to a crash (NULL pointer dereference).

In order to avoid such issue, this patch implements an internal reference
counter so that a ctrl interface can be freed only if its reference counter
reaches 0. Moreover, if a third party tries to call a library function with
an invalid context/pointer, this function will handle this issue by returning
an error.

This patch is a first step to make this library thread safe.

Signed-hostap: Loic Poulain <loicx.poulain@intel.com>
Signed-hostap: Jean-Michel Bachot <jean-michel.bachot@intel.com>
Signed-hostap: Quentin Casasnovas <quentinx.casasnovas@intel.com>
---
 src/common/wpa_ctrl.c |  215 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 206 insertions(+), 9 deletions(-)
diff mbox

Patch

diff --git a/src/common/wpa_ctrl.c b/src/common/wpa_ctrl.c
index d9a7509..fa7e823 100644
--- a/src/common/wpa_ctrl.c
+++ b/src/common/wpa_ctrl.c
@@ -1,6 +1,9 @@ 
 /*
  * wpa_supplicant/hostapd control interface library
  * Copyright (c) 2004-2007, Jouni Malinen <j@w1.fi>
+ * Copyright (c) 2013, Quentin Casasnovas <quentinx.casasnovas@intel.com>
+ *                     Jean-Michel Bachot <jean-michel.bachot@intel.com>
+ *                     Loic Poulain <loicx.poulain@intel.com>
  *
  * This software may be distributed under the terms of the BSD license.
  * See README for more details.
@@ -25,6 +28,11 @@ 
 #include "private/android_filesystem_config.h"
 #endif /* ANDROID */
 
+#ifdef CONFIG_CTRL_REF_COUNT
+#include <pthread.h>
+#include "utils/list.h"
+#endif /* CONFIG_CTRL_REF_COUNT */
+
 #include "wpa_ctrl.h"
 #include "common.h"
 
@@ -62,6 +70,103 @@  struct wpa_ctrl {
 #endif /* CONFIG_CTRL_IFACE_NAMED_PIPE */
 };
 
+#ifdef CONFIG_CTRL_REF_COUNT
+/**
+ * struct wpa_ctrl_meta - Internal meta data associated with a wpa_ctrl
+ */
+struct wpa_ctrl_meta {
+	struct wpa_ctrl *ctrl;
+	struct dl_list list;
+	int ref; /* reference counter on wpa_ctrl */
+};
+
+/* Internal protected linked list of wpa_ctrl_meta elements */
+static struct dl_list wpa_ctrl_list;
+static pthread_mutex_t list_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+/* Return the meta data associated with a wpa_ctrl, or NULL if unavailable */
+static struct wpa_ctrl_meta * ctrl_to_meta(struct wpa_ctrl *ctrl)
+{
+	struct wpa_ctrl_meta *meta;
+
+	dl_list_for_each(meta, &wpa_ctrl_list, struct wpa_ctrl_meta, list) {
+		if (meta->ctrl == ctrl)
+			return meta;
+	}
+
+	return NULL;
+}
+
+/* Initialize the meta data of the wpa_ctrl and take reference */
+static int wpa_ctrl_init_meta(struct wpa_ctrl *ctrl)
+{
+	struct wpa_ctrl_meta *meta = os_malloc(sizeof(struct wpa_ctrl_meta));
+
+	if (!meta)
+		return -ENOMEM;
+
+	pthread_mutex_lock(&list_mutex);
+
+	if (!wpa_ctrl_list.next)
+		dl_list_init(&wpa_ctrl_list);
+
+	meta->ctrl = ctrl;
+	meta->ref = 1;
+
+	dl_list_add(&wpa_ctrl_list, &meta->list);
+
+	pthread_mutex_unlock(&list_mutex);
+
+	return 0;
+}
+
+/* Increment the wpa_ctrl reference counter, return 0 on success, -1 on error */
+static int wpa_ctrl_get(struct wpa_ctrl *ctrl)
+{
+	struct wpa_ctrl_meta *meta;
+	int res = 0;
+
+	pthread_mutex_lock(&list_mutex);
+	meta = ctrl_to_meta(ctrl);
+	if (meta)
+		meta->ref++;
+	else
+		res = -1;
+	pthread_mutex_unlock(&list_mutex);
+
+	return res;
+}
+
+/* Release the wpa_ctrl resources */
+static void wpa_ctrl_release(struct wpa_ctrl *ctrl);
+
+/* Decrement the wpa_ctrl reference counter, return 0 on success, -1 on error */
+static int wpa_ctrl_put(struct wpa_ctrl *ctrl)
+{
+	struct wpa_ctrl_meta *meta;
+	int res = 0;
+
+	pthread_mutex_lock(&list_mutex);
+	meta = ctrl_to_meta(ctrl);
+	if (meta) {
+		meta->ref--;
+		if (!meta->ref) {
+			dl_list_del(&meta->list);
+			os_free(meta);
+			wpa_ctrl_release(ctrl);
+		}
+	} else {
+		res = -1;
+	}
+	pthread_mutex_unlock(&list_mutex);
+
+	return res;
+}
+#else /* CONFIG_CTRL_REF_COUNT */
+static int wpa_ctrl_init_meta(struct wpa_ctrl *ctrl) {return 0;}
+static int wpa_ctrl_get(struct wpa_ctrl *ctrl) {return 0;}
+static int wpa_ctrl_put(struct wpa_ctrl *ctrl) {return 0;}
+#endif /* CONFIG_CTRL_REF_COUNT */
 
 #ifdef CONFIG_CTRL_IFACE_UNIX
 
@@ -126,6 +231,13 @@  try_again:
 		return NULL;
 	}
 
+	if (wpa_ctrl_init_meta(ctrl)) {
+		close(ctrl->s);
+		unlink(ctrl->local.sun_path);
+		os_free(ctrl);
+		return NULL;
+	}
+
 #ifdef ANDROID
 	chmod(ctrl->local.sun_path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
 	chown(ctrl->local.sun_path, AID_SYSTEM, AID_WIFI);
@@ -206,6 +318,10 @@  try_again:
 
 void wpa_ctrl_close(struct wpa_ctrl *ctrl)
 {
+	wpa_ctrl_put(ctrl);
+#ifdef CONFIG_CTRL_REF_COUNT
+} void wpa_ctrl_release(struct wpa_ctrl *ctrl) {
+#endif /* CONFIG_CTRL_REF_COUNT */
 	if (ctrl == NULL)
 		return;
 	unlink(ctrl->local.sun_path);
@@ -343,6 +459,13 @@  struct wpa_ctrl * wpa_ctrl_open(const char *ctrl_path)
 		ctrl->remote_ip = os_strdup("localhost");
 #endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
 
+	if (wpa_ctrl_init_meta(ctrl)) {
+		close(ctrl->s);
+		os_free(ctrl->remote_ip);
+		os_free(ctrl);
+		return NULL;
+	}
+
 	if (connect(ctrl->s, (struct sockaddr *) &ctrl->dest,
 		    sizeof(ctrl->dest)) < 0) {
 		perror("connect");
@@ -371,14 +494,25 @@  char * wpa_ctrl_get_remote_ifname(struct wpa_ctrl *ctrl)
 {
 #define WPA_CTRL_MAX_PS_NAME 100
 	static char ps[WPA_CTRL_MAX_PS_NAME] = {};
+
+	if (wpa_ctrl_get(ctrl))
+		return NULL;
+
 	os_snprintf(ps, WPA_CTRL_MAX_PS_NAME, "%s/%s",
 		    ctrl->remote_ip, ctrl->remote_ifname);
+
+	wpa_ctrl_put(ctrl);
+
 	return ps;
 }
 
 
 void wpa_ctrl_close(struct wpa_ctrl *ctrl)
 {
+	wpa_ctrl_put(ctrl);
+#ifdef CONFIG_CTRL_REF_COUNT
+} void wpa_ctrl_release(struct wpa_ctrl *ctrl) {
+#endif /* CONFIG_CTRL_REF_COUNT */
 	close(ctrl->s);
 	os_free(ctrl->cookie);
 	os_free(ctrl->remote_ifname);
@@ -402,13 +536,16 @@  int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
 	char *cmd_buf = NULL;
 	size_t _cmd_len;
 
+	if (wpa_ctrl_get(ctrl))
+		return -EEXIST;
+
 #ifdef CONFIG_CTRL_IFACE_UDP
 	if (ctrl->cookie) {
 		char *pos;
 		_cmd_len = os_strlen(ctrl->cookie) + 1 + cmd_len;
 		cmd_buf = os_malloc(_cmd_len);
 		if (cmd_buf == NULL)
-			return -1;
+			goto error;
 		_cmd = cmd_buf;
 		pos = cmd_buf;
 		os_strlcpy(pos, ctrl->cookie, _cmd_len);
@@ -447,7 +584,7 @@  retry_send:
 		}
 	send_err:
 		os_free(cmd_buf);
-		return -1;
+		goto error;
 	}
 	os_free(cmd_buf);
 
@@ -458,11 +595,11 @@  retry_send:
 		FD_SET(ctrl->s, &rfds);
 		res = select(ctrl->s + 1, &rfds, NULL, NULL, &tv);
 		if (res < 0)
-			return res;
+			goto exit;
 		if (FD_ISSET(ctrl->s, &rfds)) {
 			res = recv(ctrl->s, reply, *reply_len, 0);
 			if (res < 0)
-				return res;
+				goto exit;
 			if (res > 0 && reply[0] == '<') {
 				/* This is an unsolicited message from
 				 * wpa_supplicant, not the reply to the
@@ -481,10 +618,24 @@  retry_send:
 			*reply_len = res;
 			break;
 		} else {
-			return -2;
+			goto timeout;
 		}
 	}
-	return 0;
+
+	res = 0;
+	goto exit;
+
+error:
+	res = -1;
+	goto exit;
+
+timeout:
+	res = -2;
+
+exit:
+	wpa_ctrl_put(ctrl);
+
+	return res;
 }
 #endif /* CTRL_IFACE_SOCKET */
 
@@ -522,11 +673,38 @@  int wpa_ctrl_detach(struct wpa_ctrl *ctrl)
 int wpa_ctrl_recv(struct wpa_ctrl *ctrl, char *reply, size_t *reply_len)
 {
 	int res;
+	fd_set rfds;
+	struct timeval tv;
+
+
+	for (;;) {
+		if (wpa_ctrl_get(ctrl))
+			return -EEXIST;
+
+		tv.tv_sec = 10;
+		tv.tv_usec = 0;
+
+		FD_ZERO(&rfds);
+		FD_SET(ctrl->s, &rfds);
+
+		if ((res = select(ctrl->s + 1, &rfds, NULL, NULL, &tv)) == -1) {
+			wpa_ctrl_put(ctrl);
+			break;
+		}
+
+		if (FD_ISSET(ctrl->s, &rfds)) {
+			res = recv(ctrl->s, reply, *reply_len, 0);
+			wpa_ctrl_put(ctrl);
+			break;
+		}
+
+		wpa_ctrl_put(ctrl);
+	}
 
-	res = recv(ctrl->s, reply, *reply_len, 0);
 	if (res < 0)
 		return res;
 	*reply_len = res;
+
 	return 0;
 }
 
@@ -534,19 +712,38 @@  int wpa_ctrl_recv(struct wpa_ctrl *ctrl, char *reply, size_t *reply_len)
 int wpa_ctrl_pending(struct wpa_ctrl *ctrl)
 {
 	struct timeval tv;
+	int res;
+
 	fd_set rfds;
 	tv.tv_sec = 0;
 	tv.tv_usec = 0;
 	FD_ZERO(&rfds);
+
+	if (wpa_ctrl_get(ctrl))
+		return -EEXIST;
+
 	FD_SET(ctrl->s, &rfds);
 	select(ctrl->s + 1, &rfds, NULL, NULL, &tv);
-	return FD_ISSET(ctrl->s, &rfds);
+	res = FD_ISSET(ctrl->s, &rfds);
+
+	wpa_ctrl_put(ctrl);
+
+	return res;
 }
 
 
 int wpa_ctrl_get_fd(struct wpa_ctrl *ctrl)
 {
-	return ctrl->s;
+	int fd;
+
+	if (wpa_ctrl_get(ctrl))
+		return -EEXIST;
+
+	fd = ctrl->s;
+
+	wpa_ctrl_put(ctrl);
+
+	return fd;
 }
 
 #endif /* CTRL_IFACE_SOCKET */