[PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

From: Steven Dake <sdake-AqqL/iNfm4CbU/LNMs03Rw@public.gmane.org>

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.

Signed-off-by: Steven Dake <sdake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/init.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/src/init.c b/src/init.c
index 4f0130e..ac3efdc 100644
--- a/src/init.c
+++ b/src/init.c
@@ -46,10 +46,12 @@
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <dirent.h>
+#include <stddef.h>
 #include <errno.h>
 
 #include "ibverbs.h"
 
+
 HIDDEN int abi_ver;
 
 struct ibv_sysfs_dev {
@@ -85,6 +87,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 +96,12 @@ static int find_sysfs_devs(void)
 	if (!class_dir)
 		return ENOSYS;
 
-	while ((dent = readdir(class_dir))) {
+	buf = (struct dirent *)malloc(offsetof(struct dirent, d_name) + NAME_MAX + 1);
+	if (buf == NULL) {
+		goto out_closedir;
+		ret = ENOMEM;
+	}
+	while (readdir_r(class_dir, buf, &dent) == 0 && dent) {
 		struct stat buf;
 
 		if (dent->d_name[0] == '.')
@@ -146,9 +154,11 @@ static int find_sysfs_devs(void)
 	}
 
  out:
+	free (buf);
 	if (sysfs_dev)
 		free(sysfs_dev);
 
+out_closedir:
 	closedir(class_dir);
 	return ret;
 }
@@ -290,26 +300,31 @@ static void read_config_file(const char *path)
 	fclose(conf);
 }
 
-static void read_config(void)
+static int read_config(void)
 {
 	DIR *conf_dir;
 	struct dirent *dent;
+	struct dirent *buf;
 	char *path;
 
 	conf_dir = opendir(IBV_CONFIG_DIR);
 	if (!conf_dir) {
 		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
 			IBV_CONFIG_DIR);
-		return;
+		return 0;
 	}
 
-	while ((dent = readdir(conf_dir))) {
+	buf = (struct dirent *)malloc(offsetof(struct dirent, d_name) + NAME_MAX + 1);
+	if (buf == NULL)
+		return ENOMEM;
+
+	while (readdir_r(conf_dir, buf, &dent) == 0 && dent) {
 		struct stat buf;
 
 		if (asprintf(&path, "%s/%s", IBV_CONFIG_DIR, dent->d_name) < 0) {
 			fprintf(stderr, PFX "Warning: couldn't read config file %s/%s.\n",
 				IBV_CONFIG_DIR, dent->d_name);
-			return;
+			return 0;
 		}
 
 		if (stat(path, &buf)) {
@@ -326,7 +341,9 @@ next:
 		free(path);
 	}
 
+	free(buf);
 	closedir(conf_dir);
+	return 0;
 }
 
 static struct ibv_device *try_driver(struct ibv_driver *driver,
@@ -471,7 +488,9 @@ HIDDEN int ibverbs_init(struct ibv_device ***list)
 
 	check_memlock_limit();
 
-	read_config();
+	ret = read_config();
+	if (ret)
+		return -ret;
 
 	ret = find_sysfs_devs();
 	if (ret)
-- 
1.6.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Jason Gunthorpe]

On Wed, Jul 07, 2010 at 11:37:01AM -0700, Steven Dake wrote:
> From: Steven Dake <sdake-AqqL/iNfm4CbU/LNMs03Rw@public.gmane.org>
> 
> 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.

Well, that isn't true on glibc.

The buffer returned by readdir() is stored in the DIR * and is
allocated during opendir().
 
The only time you hit thread safety issues is if multiple threads
attempt to use the same DIR at once - glibc has internal locking to
protect the DIR, but the buffer would be re-used.

Since libibverbs doesn't pass the DIR * between threads this usage is
safe.

It isn't entirely clear to me if POSIX definition of readdir as
non-rentrant applies to the general case or to the specific case I
discuss above. Certainly, any implementation can do as glibc does and
allocate the buffer in the DIR *.

But, you surely could not have seen a bug on Linux that was caused by
this - so why was this patch prepared?

BTW, POSIX also says that NAME_MAX is optional, so you have traded one
unlikely portability problem for another unlikely portability problem :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Roland Dreier]

 > 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 is a bit vague.  Is the problem that other threads may do readdir()
while libibverbs initializes?

 >  #include "ibverbs.h"
 >  
 > +
 >  HIDDEN int abi_ver;

whitespace damage here.

 > +	buf = (struct dirent *)malloc(offsetof(struct dirent, d_name) + NAME_MAX + 1);

cast of void* is not needed.  Also it seems this could be alloca() and
simplify things (avoid error checking and freeing).

Thanks,
  Roland

-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

On 07/07/2010 11:52 AM, Jason Gunthorpe wrote:
> On Wed, Jul 07, 2010 at 11:37:01AM -0700, Steven Dake wrote:
>> From: Steven Dake<sdake-AqqL/iNfm4CbU/LNMs03Rw@public.gmane.org>
>>
>> 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.
>
> Well, that isn't true on glibc.
>
> The buffer returned by readdir() is stored in the DIR * and is
> allocated during opendir().
>
> The only time you hit thread safety issues is if multiple threads
> attempt to use the same DIR at once - glibc has internal locking to
> protect the DIR, but the buffer would be re-used.
>
> Since libibverbs doesn't pass the DIR * between threads this usage is
> safe.
>
> It isn't entirely clear to me if POSIX definition of readdir as
> non-rentrant applies to the general case or to the specific case I
> discuss above. Certainly, any implementation can do as glibc does and
> allocate the buffer in the DIR *.
>
> But, you surely could not have seen a bug on Linux that was caused by
> this - so why was this patch prepared?
>

We redefine all the non-thread-safe posix calls in our package (such as 
readdir) to assert(0).  This prevents their usage in our app.  The 
reason we do this is to prevent plugin developers from writing plugins 
that use non-thread-safe posix APIs.  Our app is portable, where the 
glibc semantics you describe above are not present.  POSIX is blank on 
this issue, and was addressed in readdir_r.

Regards
-steve
> BTW, POSIX also says that NAME_MAX is optional, so you have traded one
> unlikely portability problem for another unlikely portability problem :)
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

On 07/07/2010 11:54 AM, Roland Dreier wrote:
>   >  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 is a bit vague.  Is the problem that other threads may do readdir()
> while libibverbs initializes?
>
>   >   #include "ibverbs.h"
>   >
>   >  +
>   >   HIDDEN int abi_ver;
>
> whitespace damage here.
>

thanks I'll fix

>   >  +	buf = (struct dirent *)malloc(offsetof(struct dirent, d_name) + NAME_MAX + 1);
>
> cast of void* is not needed.  Also it seems this could be alloca() and
> simplify things (avoid error checking and freeing).
>

yes..  I can change if you like.  Never been fond of alloca because of 
its limitation on error checking with stack overflow which is unlikely 
in this call path.

Regards
-steve

> Thanks,
>    Roland
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Jason Gunthorpe]

On Wed, Jul 07, 2010 at 12:14:16PM -0700, Steven Dake wrote:

>> But, you surely could not have seen a bug on Linux that was caused by
>> this - so why was this patch prepared?

> We redefine all the non-thread-safe posix calls in our package (such as  
> readdir) to assert(0).  This prevents their usage in our app.  The  
> reason we do this is to prevent plugin developers from writing plugins  
> that use non-thread-safe posix APIs.  Our app is portable, where the  
> glibc semantics you describe above are not present.  POSIX is blank on  
> this issue, and was addressed in readdir_r.

Well, if you are already creating wrappers, wouldn't it be better to
provide wrappers that provide the glibc functionality on platforms you
port to that don't already do that, rather than forbid a perfectly
functional, and thread safe, call?

FWIW, I've always considered readdir_r to be broken, you pass in a
buffer without passing in a size and hope everything works out. Your
proposed patch to libibverbs is also not-portable because it uses
NAME_MAX, not pathconf.. Sigh POSIX.

I guess, as a general question, do you know of any libc
implementations that actually use a static buffer for readdir and
would require readdir_r to function properly?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

On 07/07/2010 01:47 PM, Jason Gunthorpe wrote:
> On Wed, Jul 07, 2010 at 12:14:16PM -0700, Steven Dake wrote:
>
>>> But, you surely could not have seen a bug on Linux that was caused by
>>> this - so why was this patch prepared?
>
>> We redefine all the non-thread-safe posix calls in our package (such as
>> readdir) to assert(0).  This prevents their usage in our app.  The
>> reason we do this is to prevent plugin developers from writing plugins
>> that use non-thread-safe posix APIs.  Our app is portable, where the
>> glibc semantics you describe above are not present.  POSIX is blank on
>> this issue, and was addressed in readdir_r.
>
> Well, if you are already creating wrappers, wouldn't it be better to
> provide wrappers that provide the glibc functionality on platforms you
> port to that don't already do that, rather than forbid a perfectly
> functional, and thread safe, call?
>

Not sure how to map a readdir to readdir_r on a thread unsafe system... 
  perhaps with thread keys.  In any regard, seems pointless, readdir_r 
is there and what POSIX specifies for this purpose.

> FWIW, I've always considered readdir_r to be broken, you pass in a
> buffer without passing in a size and hope everything works out. Your

I also have objections to some POSIX standard APIs - however, using 
non-reentrant POSIX apis when reentrant POSIX APIs are available seems 
counterproductive.

> proposed patch to libibverbs is also not-portable because it uses
> NAME_MAX, not pathconf.. Sigh POSIX.
>

On bsd/solaris/darwin/linux, NAME_MAX is defined.  Not sure which other 
POSIX systems one would care about..

> I guess, as a general question, do you know of any libc
> implementations that actually use a static buffer for readdir and
> would require readdir_r to function properly?

I have not looked at each libc's implementation to see if it matches the 
glibc behaviour.  The resolution provided by glibc seems like an 
enhancement over POSIX, where the behaviour is unspecified.

Regards
-steve
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Jason Gunthorpe]

On Wed, Jul 07, 2010 at 02:24:45PM -0700, Steven Dake wrote:

> Not sure how to map a readdir to readdir_r on a thread unsafe system...  
> perhaps with thread keys.  In any regard, seems pointless, readdir_r is 
> there and what POSIX specifies for this purpose.

Override opendir and allocate the buffer then and return a pointer to it
through a custom 'DIR *'.

>> FWIW, I've always considered readdir_r to be broken, you pass in a
>> buffer without passing in a size and hope everything works out. Your
>
> I also have objections to some POSIX standard APIs - however, using  
> non-reentrant POSIX apis when reentrant POSIX APIs are available seems  
> counterproductive.

Well, if the non-reentrant ones are badly designed I'm not sure it is
a good trade.. Ie Solaris's man pages say:

 It is safe to use readdir() in a threaded application, so long as only
 one thread reads from the directory stream at any given time. The
 readdir() function is generally preferred over the readdir_r()
 function.

Also see

http://lists.grok.org.uk/pipermail/full-disclosure/2005-November/038295.html

The horribleness of readdir_r is well documented, and is partly why
libc's advocate thread safe readdir() desipte the existence of
readdir_r.

>> proposed patch to libibverbs is also not-portable because it uses
>> NAME_MAX, not pathconf.. Sigh POSIX.

> On bsd/solaris/darwin/linux, NAME_MAX is defined.  Not sure which other  
> POSIX systems one would care about..

If all you care able is bsd/solaris/darwin/linux then this is a
non-problem, AFAIK they have sane libc's :) Ie I just checked and
openbsd libc has been using a dynamic buffer allocated at opendir
since 1996.

If you care about theortical portability then you have to worry about
NAME_MAX too..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

From: Steven Dake <sdake-AqqL/iNfm4CbU/LNMs03Rw@public.gmane.org>

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.

Signed-off-by: Steven Dake <sdake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/init.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/init.c b/src/init.c
index 4f0130e..11b8415 100644
--- a/src/init.c
+++ b/src/init.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

On 07/07/2010 02:49 PM, Jason Gunthorpe wrote:
> On Wed, Jul 07, 2010 at 02:24:45PM -0700, Steven Dake wrote:
>
>> Not sure how to map a readdir to readdir_r on a thread unsafe system...
>> perhaps with thread keys.  In any regard, seems pointless, readdir_r is
>> there and what POSIX specifies for this purpose.
>
> Override opendir and allocate the buffer then and return a pointer to it
> through a custom 'DIR *'.
>
>>> FWIW, I've always considered readdir_r to be broken, you pass in a
>>> buffer without passing in a size and hope everything works out. Your
>>
>> I also have objections to some POSIX standard APIs - however, using
>> non-reentrant POSIX apis when reentrant POSIX APIs are available seems
>> counterproductive.
>
> Well, if the non-reentrant ones are badly designed I'm not sure it is
> a good trade.. Ie Solaris's man pages say:
>
>   It is safe to use readdir() in a threaded application, so long as only
>   one thread reads from the directory stream at any given time. The
>   readdir() function is generally preferred over the readdir_r()
>   function.
>
> Also see
>
> http://lists.grok.org.uk/pipermail/full-disclosure/2005-November/038295.html
>
> The horribleness of readdir_r is well documented, and is partly why
> libc's advocate thread safe readdir() desipte the existence of
> readdir_r.
>
>>> proposed patch to libibverbs is also not-portable because it uses
>>> NAME_MAX, not pathconf.. Sigh POSIX.
>
>> On bsd/solaris/darwin/linux, NAME_MAX is defined.  Not sure which other
>> POSIX systems one would care about..
>
> If all you care able is bsd/solaris/darwin/linux then this is a
> non-problem, AFAIK they have sane libc's :) Ie I just checked and
> openbsd libc has been using a dynamic buffer allocated at opendir
> since 1996.
>
> If you care about theortical portability then you have to worry about
> NAME_MAX too..
>

Thanks I'll look into NAME_MAX for my server.

Regards
-steve
> Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Roland Dreier]

 > The readdir POSIX api is not thread safe.  This presents problems in
 > multithreaded programs that use the libibverbs APIs.

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?

 > +#if HAVE_ALLOCA_H
 > +#include <alloca.h>
 > +#endif

Elsewhere we just include alloca.h unconditionally, and in fact I don't
see anything that would define HAVE_ALLOCA_H in the libibverbs tree.
Does this actually work?

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Jason Gunthorpe]

On Thu, Jul 08, 2010 at 11:27:48AM -0700, Roland Dreier wrote:

> 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?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Roland Dreier]

 > +	buf = alloca(offsetof(struct dirent, d_name) + NAME_MAX + 1);
 > +	while (readdir_r(class_dir, buf, &dent) == 0 && dent) {

So after thinking this over, I don't think I'm going to apply this
patch.  I think the right fix is for corosync to allow readdir() -- in
general pushing people to safer APIs is probably a good thing, but I
think in this particular case it doesn't make sense.  In fact it seems
that readdir_r() is actually worse since the "buf" parameter does not
have a well-defined required size, while readdir() is actually safe in
most uses.

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] change thread-unsafe readdir to thread-safe readdir_r calls

[Steven Dake]

On 07/21/2010 11:06 AM, Roland Dreier wrote:
>   >  +	buf = alloca(offsetof(struct dirent, d_name) + NAME_MAX + 1);
>   >  +	while (readdir_r(class_dir, buf,&dent) == 0&&  dent) {
>
> So after thinking this over, I don't think I'm going to apply this
> patch.  I think the right fix is for corosync to allow readdir() -- in
> general pushing people to safer APIs is probably a good thing, but I
> think in this particular case it doesn't make sense.  In fact it seems
> that readdir_r() is actually worse since the "buf" parameter does not
> have a well-defined required size, while readdir() is actually safe in
> most uses.
>
>   - R.

thanks for considering

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