race condition in boost::filesystem::v4: `directory_iterator` calls `directory_entry` constructor, which calls `refresh()`, which throws exception if the file no longer exists

[gh-user-2022]

race condition in boost::filesystem::v4: `directory_iterator` calls `directory_entry` constructor, which calls `refresh()`, which throws exception if the file no longer exists

[Lastique]

Boost.Filesystem (and std::filesystem) is generally not protected against filesystem races. The behavior is undefined if the filesystem is modified concurrently with Boost.Filesystem operations. This is by design.

That said, Boost.Filesystem takes some measures to protect itself from filesystem races, when possible. The amount and robustness of such protection is not guaranteed and should not be relied upon (i.e. it is a matter of QoI). In particular, directory_iterator does not construct directory_entry in the normal way, and initializes its cached status and symlink status, when this information is made available by the OS during iteration. In this case, the race you describe should not happen because refresh() won't be implicitly called.

[gh-user-2022]

I would ask you to reconsider, due to following reason: there is no easy way for calling code to add workaround in this case, because the race condition happens inside the Boost.Filesystem itself. So it is worth to add a workaround inside the library.

In Boost.Filesystem v3 the exception would happen not inside the increment of the iterator, but when calling code would attempt to do anything with the directory_entry. And in such case the workaround can be implemented in calling code.

In this case, the race you describe should not happen because refresh() won't be implicitly called.

Well, the documentation explicitly says that for Boost.Filesystem v4 the refresh() is called.
So is there a contradiction between the documentation and the implementation?

Also, debugging shows that refresh() is indeed called during increment of the iterator.
(Let me know if you need a sample code to reproduce the issue.)

Another consideration is: calling refresh() always for each directory_entry is a performance penalty (additional syscall) in both Linux and Windows.

[Lastique]

I would ask you to reconsider, due to following reason: there is no easy way for calling code to add workaround in this case, because the race condition happens inside the Boost.Filesystem itself. So it is worth to add a workaround inside the library.

Preserving status obtained during iteration is the workaround. If this information is not provided by the OS, there is no other way the library could obtain status information other than to query the filesystem, which may result in a filesystem race.

And I'll reiterate, whether the OS provides file status on iteration or not, user's code must be prepared that incrementing or dereferencing a directory iterator can fail (with an exception or an error code) for a multitude of reasons, filesystem race being just one of them.

So is there a contradiction between the documentation and the implementation?

No, the effects described in the documentation are maintained, provided that there is no filesystem race. As I said, with a filesystem race, the behavior is undefined.

Also, debugging shows that refresh() is indeed called during increment of the iterator.
(Let me know if you need a sample code to reproduce the issue.)

Does this reproduce on the current develop? On what OS and filesystem? If this happens on develop, I would like a reproducer, please.

[Lastique]

Well, the documentation explicitly says that for Boost.Filesystem v4 the refresh() is called.

Also, I'll reiterate that the documentation doesn't say this. The directory_entry constructor from path calls refresh(), yes, but directory_iterator::operator++() doesn't call that constructor.