change thread-unsafe readdir to thread-safe readdir_r calls

From: Steven Dake <sdake-AqqL/iNfm4CbU/[email protected]>

The readdir POSIX api is not thread safe. This presents problems in multithreaded programs that use the libibverbs APIs. This patch has been tested with a libibverbs application (http://www.corosync.org) on Mellanox MT26428 cards.

This updated version uses alloca instead of malloc.

---
 src/init.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

index 4f0130e..11b8415 100644
@@ -35,6 +35,9 @@
 #  include <config.h>
 #endif /* HAVE_CONFIG_H */
 
+#if HAVE_ALLOCA_H
+#include <alloca.h>
+#endif
 #include <stdlib.h>
 #include <string.h>
 #include <glob.h>

@@ -46,6 +49,7 @@
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <dirent.h>
+#include <stddef.h>
 #include <errno.h>
 
 #include "ibverbs.h"

@@ -85,6 +89,7 @@ static int find_sysfs_devs(void)
 	struct ibv_sysfs_dev *sysfs_dev = NULL;
 	char value[8];
 	int ret = 0;
+	struct dirent *buf;
 
 	snprintf(class_path, sizeof class_path, "%s/class/infiniband_verbs",
 		 ibv_get_sysfs_path());

@@ -93,7 +98,8 @@ static int find_sysfs_devs(void)
 	if (!class_dir)
 		return ENOSYS;
 
-	while ((dent = readdir(class_dir))) {
+	buf = alloca(offsetof(struct dirent, d_name) + NAME_MAX + 1);
+	while (readdir_r(class_dir, buf, &dent) == 0 && dent) {
 		struct stat buf;
 
 		if (dent->d_name[0] == '.')

@@ -294,6 +300,7 @@ static void read_config(void)
 {
 	DIR *conf_dir;
 	struct dirent *dent;
+	struct dirent *buf;
 	char *path;
 
 	conf_dir = opendir(IBV_CONFIG_DIR);

@@ -303,7 +310,8 @@ static void read_config(void)
 		return;
 	}
 
-	while ((dent = readdir(conf_dir))) {
+	buf = alloca(offsetof(struct dirent, d_name) + NAME_MAX + 1);
+	while (readdir_r(conf_dir, buf, &dent) == 0 && dent) {
 		struct stat buf;
 
 		if (asprintf(&path, "%s/%s", IBV_CONFIG_DIR, dent->d_name) < 0) {

1.6.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

So as discussed this is not true ... as far as I can see libibverbs only calls readdir inside a mutex and so on all platforms where libibverbs has even a chance of being applicable, the current code works fine. The real issue is that corosync defines a new environment where readdir is not allowed.

I'm really not sure about this patch one way or another. With alloca, it ends up looking pretty clean, but we do introduce the tiny possibility of buffer overrun (since readdir_r uses a user-supplied buffer of poorly-specified size); the benefit is that we work within corosync's constraints. I honestly don't have a good feeling whether the benefit exceeds the cost.

Jason, any opinion one way or another?

Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Well, my view is that this is needless churn/risk for a very bad choice made by another project. Aliasing a perfectly thread safe readdir() to assert is anti-social :)

I also do not think it enhances theoretical portability, as NAME_MAX is not guarenteed to exist by POSIX and alloca is not a C or POSIX standard function. (C99 stadardized alloca as variable sized arrays)

Frankly, as a Linux only low level library, relying on glibc behavior to make it simpler and more correct is perfectly acceptable to me..

It isn't even the case that libibverbs is an outlier here, common things like libcrypto even use readdir! Purging readdir from every library your app might want to load via a plugin is a really futile quest :(

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

thanks for considering

regards
-steve
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html