Re: readdir() harmful in threaded code

[Pranith Kumar Karampuri <pkarampu@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxx
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel@xxxxxxxxxxx
> http://www.gluster.org/mailman/listinfo/gluster-devel

--
Emmanuel Dreyfus
manu@xxxxxxxxxx
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel

--

Pranith

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel