Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mitigate copying/deletion of unrecognised symlinks #5953

Merged
merged 1 commit into from
May 15, 2024

Conversation

dra27
Copy link
Member

@dra27 dra27 commented May 14, 2024

Partially addresses the issue reported in #5782 (comment)

In these situations, Cygwin is most likely to create a JUNCTION, which can't be read by Unix.lstat. The fix for now I propose is simply to warn. This has the very big benefit of making ocamlbuild install without needing Developer Mode.

It has the downside that we fail to copy the file... I'm suggesting we might choose to live with that for now.

Note that OpamSystem.remove_file_t contained an unnecessary second call to lstat. I needed to re-use the actual removal logic for the exception case in remove_dir_t, but lstat is very expensive in C on Unix, even more expensive in OCaml and indescribably more expensive on Windows, so eliminating the double-call seems better!

@dra27
Copy link
Member Author

dra27 commented May 14, 2024

With this, the issue at least reduces to:

C:\Scratch>opam source ocp-indent
[WARNING] Warning: cannot copy C:\Users\DRA\AppData\Local\Temp\opam-54072-ca16dd\ocp-indent-1.8.1\tests\inplace\link.ml to
          C:\Scratch\ocp-indent.1.8.1\tests\inplace
Successfully extracted to C:\Scratch\ocp-indent.1.8.1

@@ -129,19 +129,15 @@ let win32_unlink fn =
with _ -> raise e)

let remove_file_t ?(with_log=true) file =
if
try ignore (Unix.lstat file); true with Unix.Unix_error _ -> false
Copy link
Member

@kit-ty-kate kit-ty-kate May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove_file_t on a non-existing file used to simply ignore it. This PR changes this behaviour to error out instead.
Was the old behaviour used anywhere that we know?

Copy link
Member Author

@dra27 dra27 May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - its only other use is in the cross-device fallback for mv. The only way it could be called with a file which didn't exist is if the file has been removed between the test in the calling function and the test here, and there was already a race for that anyway (between the test I've deleted here and the internal_error handler below).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is also being used in remove_file and remove_dir which are used everywhere in opam's code (e.g. via OpamFilename.remove)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove_file doesn't use remove_file_t (LL202-215) and remove_dir already checks whether its argument exists?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH! I see. There are two toplevel versions of remove_file in the same file. Only the latter is exported. Sorry for missing that. This is all good then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, until you’d said, I’d missed that too! I was only sure that the code was ok because I’d deleted the first remove_file as a result of an unused value warning and already searched for other uses of remove_file_t 🙂

@kit-ty-kate
Copy link
Member

maybe this also warrants an addition to the API section of the master changelog actually

@dra27
Copy link
Member Author

dra27 commented May 15, 2024

The API hasn’t changed though?

@kit-ty-kate
Copy link
Member

The API hasn’t changed though?

no but its behaviour has (e.g. copy_dir can now display a warning on windows)

There are two problems we can encounter with symlinks on Windows:
- The user cannot create native symlinks (Developer Mode is not enabled)
- At the point that the symlink is created, the file it refers to does
  not exist

In this instance, Cygwin's tar falls back to its internal mechanisms,
which can't be read by opam and cause issues when copying directories,
etc.

This commit mitigates the directory copy by displaying a warning and
fixes the deletion of the directory by deleting the file anyway.
@dra27
Copy link
Member Author

dra27 commented May 15, 2024

Ah, yes, indeed. I added:

  * `OpamSystem.copy_dir` and `OpamSystem.mv` may display a warning on Windows if an invalid symlink (e.g. an LXSS Junction) is found [#5953 @dra27]

@kit-ty-kate
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants