Fix filelib:safe_relative_path/2

[juhlig]

Fixes #6460.

The function would return a wrong path if the second argument was a link and the path in the first argument would descend and then ascend again to the top level.

The function would also report a path as unsafe by falsely detecting a link loop if a link would target another link, which in turn would target a file of the same name like bar --> foo/bar --> (foo/)foo/bar.

As the link resolving and CWD-breakout and loop detection is pretty tricky, I'm not 100% sure it is correct in every aspect and corner case. Please review carefully.

[github-actions]

CT Test Results

       2 files       89 suites   40m 3s ⏱️
1 908 tests 1 860 ✔️ 48 💤 0 ❌
2 202 runs  2 152 ✔️ 50 💤 0 ❌

Results for commit 8e83401.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[juhlig]

It might be worth noting that a path is always reported as unsafe if it contains a link with an absolute target, even if that target is really located under the given Cwd. This behavior is the same as in the current implementation.

[juhlig]

It may also be worth noting that the given Cwd is not checked in any way, ie assumed to be safe. That means that it may actually contain link loops, without safe_relative_path reporting the fact. Only the given Path is checked.

[juhlig]

Finally, it may be worth noting that if a call to safe_relative_path reports a given Path as safe (or unsafe for that matter), it is only so at the very moment when a path segment is checked. The actual directory/link structure may change immediately after or even while the path is being checked.

[Maria-12648430]

@rickard-green @bjorng @jhogberg what is the status of this PR, on your side? The issue behind this looks somewhat serious to me, which should be discussed and seen to 😅

[Maria-12648430]

That said, I would suggest as much as the complete removal of the function because, as @juhlig already said, the result is not unconditionally trustworthy on OSes that support symlinks: A path that is reported as safe (or unsafe for that matter) may not be safe (or unsafe) when it is used afterwards, as the filesystem may have changed. Even while it is being checked, actually.

Nevertheless, it carries a strong connotation of "if you just use this function, nothing bad can happen". The alternative is to plaster the documentation with appropriate warnings.

Just my 2ct 😉

[bjorng]

@Maria-12648430 We intend to review this pull request but haven't gotten to it yet.

[bjorng]

I have taken a first look at the code. So far I haven't found any flaws. I will do some more thinking and probably discuss this PR with one or more colleagues.

[juhlig]

@bjorng suggestions applied 👍

[bjorng]

Thanks! Added to our daily builds.

[bjorng]

Thanks for your pull request.