[Gluster-devel] readdir() harmful in threaded code

[Pranith Kumar Karampuri]

Emmanuel,
       I procrastinated too long on this :-/, It is July already :-(. I
just looked at the man page in Linux and it is a bit confusing, so I am not
sure how to go ahead.

For readdir_r(), I see:

DESCRIPTION
       This function is deprecated; use readdir(3) instead.

       The readdir_r() function was invented as a reentrant version of
readdir(3).  It reads
       the next directory entry from the directory stream dirp, and returns
it in the  call‐
       er-allocated  buffer  pointed  to by entry.  For details of the
dirent structure, see
       readir(3).

For readdir(3) I see:
ATTRIBUTES
       For an explanation of the terms used in this section, see
attributes(7).

       ┌──────────┬───────────────┬──────────────────────────┐
       │Interface │ Attribute     │ Value                    │
       ├──────────┼───────────────┼──────────────────────────┤
       │readdir() │ Thread safety │ MT-Unsafe race:dirstream │
       └──────────┴───────────────┴──────────────────────────┘

       In the current POSIX.1 specification (POSIX.1-2008), readdir() is
not required to be thread-safe.  However, in modern implementations
(including the glibc implementation), concur‐
       rent  calls  to readdir() that specify different directory streams
are thread-safe.  In cases where multiple threads must read from the same
directory stream, using readdir() with
       external synchronization is still preferable to the use of the
deprecated readdir_r(3) function.  It is expected that a future version of
POSIX.1 will require  that  readdir()  be
       thread-safe when concurrently employed on different directory
streams.


So should we do readdir() with external locks for everything instead?


On Thu, Feb 11, 2016 at 2:35 PM, Emmanuel Dreyfus <manu at netbsd.org> wrote:

> Juste to make sure there is no misunderstanding here: unfortunately I
> do not have time right now to submit a fix. It would be nice if someone
> else coule look at it.
>
> On Wed, Feb 10, 2016 at 01:48:52PM +0000, Emmanuel Dreyfus wrote:
> > Hi
> >
> > After obtaining a core in a regression, I noticed there are a few
> readdir()
> > use in threaded code. This is begging for a crash, as readdir() maintains
> > an internal state that will be trashed on concurent use. readdir_r()
> > should be used instead.
> >
> > A quick search shows readdir(à usage here:
> > contrib/fuse-util/mount_util.c:30
> > extras/test/ld-preload-test/ld-preload-test.c:310
> > extras/test/test-ffop.c:550
> > libglusterfs/src/compat.c:256
> > libglusterfs/src/compat.c:315
> > libglusterfs/src/syscall.c:97
> > tests/basic/fops-sanity.c:662
> > tests/utils/arequal-checksum.c:331
> >
> > Occurences in contrib, extra and tests are probably harmless are there
> > are usage in standalone programs that are not threaded. We are left with
> > three groups of problems:
> >
> > 1) libglusterfs/src/compat.c:256 and libglusterfs/src/compat.c:315
> > This is Solaris compatibility code. Is it used at all?
> >
> > 2)  libglusterfs/src/syscall.c:97 This is the sys_readdir() wrapper,
> > which is in turn used in:
> > libglusterfs/src/run.c:284
> > xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c:582
> > xlators/features/changelog/lib/src/gf-history-changelog.c:854
> > xlators/features/index/src/index.c:471
> > xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
> > xlators/storage/posix/src/posix.c:3700
> > xlators/storage/posix/src/posix.c:5896
> >
> > 3) We also find sys_readdir() in libglusterfs/src/common-utils.h for
> > GF_FOR_EACH_ENTRY_IN_DIR() which in turn appears in:
> > libglusterfs/src/common-utils.c:3979
> > libglusterfs/src/common-utils.c:4002
> > xlators/mgmt/glusterd/src/glusterd-hooks.c:365
> > xlators/mgmt/glusterd/src/glusterd-hooks.c:379
> > xlators/mgmt/glusterd/src/glusterd-store.c:651
> > xlators/mgmt/glusterd/src/glusterd-store.c:661
> > xlators/mgmt/glusterd/src/glusterd-store.c:1781
> > xlators/mgmt/glusterd/src/glusterd-store.c:1806
> > xlators/mgmt/glusterd/src/glusterd-store.c:3044
> > xlators/mgmt/glusterd/src/glusterd-store.c:3072
> > xlators/mgmt/glusterd/src/glusterd-store.c:3593
> > xlators/mgmt/glusterd/src/glusterd-store.c:3606
> > xlators/mgmt/glusterd/src/glusterd-store.c:4032
> > xlators/mgmt/glusterd/src/glusterd-store.c:4111
> >
> > There a hive of sprious bugs to squash here.
> >
> > --
> > Emmanuel Dreyfus
> > manu at netbsd.org
> > _______________________________________________
> > Gluster-devel mailing list
> > Gluster-devel at gluster.org
> > http://www.gluster.org/mailman/listinfo/gluster-devel
>
> --
> Emmanuel Dreyfus
> manu at netbsd.org
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
>



-- 
Pranith
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20160723/1977dcb8/attachment.html>