boost::filesystem::remove() throws on self referenced symlinks

[dizzy@…]

If one creates a symlink like this "ln -s sym sym" (to itself symlink), bfs::exists("sym") will throw because the documentation defines it to do so (and I have nothing against it doing so, although in my programs because of this I am forced to just use "lstat()" and I can't replace using "lstat()" to check if something exists with bfs::exists).

However, I do think that bfs::remove("sym") throwing for the same reason (because it calls upon bfs::exists()) is the wrong thing to do. The source around bfs::remove() implementation even try to solve the issue of "dangling symlinks" as they call it by having an exception in case bfs::exists returns false and the target path points to a dangling symlink and still tries to remove it. Unfortunetly, exists will throw in my example of a symlink (not even dangling technically speaking :) ) even tho a remove on that kind of symlink should work just fine (it does work in shell or just doing std::remove()).

If boost::exists is going to be left in place to throw for those "valid" symlinks (because exists is suposed to try to dereference symlinks and dereferencing such a recursive symlink is always an error) then the only solution I see is to not use exists at all. Because I also found that remove and remove_all are fairly inefficient regarding repetitive calls fo stat/lstat on the same path I've created the attached patch that fixes the reported problem and would be more efficient "system call" wise.

I am using Linux and ext3 filesystem tho in theory it should perform the same on any symlink supporting platform (since symlinks are just containers of strings as defined by POSIX and if you try to dereference a self referencing symlink you should always get an error different than ENOENT and others for which exists() does not throw).

[dizzy@…]

Reworks remove/remove_all so they do not error on self referencing symlinks and are more efficient "system call" wise

[Beman Dawes]

Thanks for the patch!

It has been applied.

--Beman

[anonymous]

Thanks for applying the patch. While looking for it I noticed it's not in 1.36 release but it is in the boost trunk. This way I also noticed that the trunk code seems a sort of 1.35 code with my patch instead of being a 1.36 version with some sort of adapted version of my patch. In 1.36:

• remove():
  • has a default argument for error code thus allows to be called in a nothrow way
  • does not return bool but void and thus no need for any stat/exists() I/O call (errors based on the result of the system dependent remove API error)
• remove_all()
  • has a default argument for error code thus allows to be called in a nothrow way

I will adapt my patch for the 1.36 code since I think the current version in boost trunk reverses on good changes that were in 1.36 (unless you think otherwise of course :) ).

[dizzy@…]

Err, remove_all() had no no changes in 1.36 so I was wrong above about the remove_all() default argument.

Only remove() had some changes in 1.36 that seem to be reversed by application of my patch. All that's needed to fix this actually is to get remove() back to the version from 1.36. This also fixes what my patch intended to do because 1.36 remove() does not calle exists() or status() on the parameter thus no reason to throw on self referenced symlinks.