readdir_r is deprecated as of glibc-2.24

[cuviper]

Per bug 19056, glibc-2.24 has deprecated readdir_r and readdir64_r in favor of plain readdir and readdir64. The reasons are discussed on the updated manpage.

It states that readdir is already thread-safe in "modern" implementations, including glibc. We should see if all platforms targeted by unix/fs.rs satisfy this. Note that the Solaris (Illumos) port is already using plain readdir.

It requires external synchronization if a particular directory stream may be shared among threads, but I believe we avoid that naturally from the lack of &mut aliasing. Dir is Sync, but only ReadDir accesses it, and only from its mutable Iterator implementation.

[alexcrichton]

Interesting! Are we sure that readdir is threadsafe on all Unix platforms? Presumably deprecation doesn't mean it'll be removed and/or stop working, so it shouldn't be harmful to keep using, right?

We also support back to glibc 2.5 I believe so we may want to make sure that 2.5-2.24 are all threadsafe before we stop using readdir_r

[cuviper]

I don't know about all Unix platforms -- "modern" is deliciously vague. That needs to be investigated.

The only difference in glibc's readdir since 2.5 is to skip the directory lock when built into modules other than libc, which is irrelevant to outsiders. So its general promise of thread safety should hold.

It should be ok to continue using the deprecated readdir_r, as long as you accept the limitations that led to deprecation, mainly regarding NAME_MAX and ENAMETOOLONG. For instance, this was responsible for CVE-2013-4237 (glibc bug 14699).

Actually, that CVE was not fixed in RHEL5, which I'm guessing is where you got the baseline of glibc 2.5. So I guess that's not a good idea to keep using it after all, for safety rather than deprecation.

[nagisa]

In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is not required to be thread-safe.

This essentially makes this issue inactionable until…

It is expected that a future version of POSIX.1 will make readdir_r() obsolete, and require that readdir() be thread-safe when concurrently employed on different directory streams.

1. … a new version of POSIX is released; and
2. every single version of every single POSIX platform we support actually implements that part of the currently-not-even-released POSIX spec¹.

If we are concerned about the CVE thing, the only option in foreseeable future is to re-implement readdir by ourselves in the way we need it to be implemented (i.e. MT-safely) without calling out to the platform’s libc.

¹: Confirming that every target we currently support has a MT-safe implementation is not enough, because any target could easily make the function non-MT-safe without breaking any promises.

[cuviper]

Confirming that every target we currently support has a MT-safe implementation is not enough, because any target could easily make the function non-MT-safe without breaking any promises.

I think that's overly strict. Surely there are other cases where we rely on effective platform behavior, not merely what is POSIX-required?

[alexcrichton]

Just to make sure I understand as well, this CVE business is only a problem if you're using a buggy glibc, right? If you're running with an updated glibc there's no reason to not use readdir_r? Also to be clear we're not shipping glibc 2.5 or anything, that's just what's installed on the dist bot.

If that's the case I'm tempted to leave this as-is for now and give the world a chance to catch up with the readdir_r deprecation before we rush to change everything. Mostly because everything works and there's no need to cause unnecessary churn otherwise.

[cuviper]

Even with that particular bug fixed, readdir_r is limited to NAME_MAX bytes. An error ENAMEOVERFLOW is not so nice when readdir would have handled it just fine.

One hassle of readdir is in detecting errors. You have to clear errno first, then check it after a NULL result to distinguish from the end of stream. I believe the current Solaris code is wrong here.

[alexcrichton]

Aha, good point! I suspect it may not happen too much in practice though as I'm not sure I've ever seen a bug report with ENAMEOVERFLOW at this point as well. In that sense I feel like we should probably stick with what we have today until platforms catch up or we verify that existing platforms all implement readdir in a threadsafe fashion.

PRs are of course always welcome for the Solaris bits though!

[binarycrusader]

One hassle of readdir is in detecting errors. You have to clear errno first, then check it after a NULL result to distinguish from the end of stream. I believe the current Solaris code is wrong here.

Per readdir(3c) on Solaris:

   Upon successful completion, readdir() returns a pointer to an object of
   type  struct  dirent.  When  an error is encountered, a null pointer is
   returned and errno is set to indicate the error. When the  end  of  the
   directory  is  encountered, a null pointer is returned and errno is not
   changed.

So yes, you are correct.

As for MT-Safe on Solaris, readdir(3c) provides this guidance:

   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 read-
   dir() function is generally preferred over the readdir_r() function.

...

   The readdir() function is Safe when used by a single thread on a  given
   DIR  pointer  (that  is, opendir(), readdir(), ..., closedir()), and is
   the recommended usage. The readdir_r() function is Safe to be  used  by
   multiple threads on a given DIR pointer.

This seems roughly equivalent to the guidance provided by glibc's man page.

[jhudsoncedaron]

Ugh. And I just found a use case for the non-extant opendir_r. That is, to walk a directory in a fork() child. Hint: if you're multi-threaded it's not safe to call malloc() in a fork() child.