Virtual_rules: use copy instead of symlink when copying signatures

[dannywillems]

Fix #4195

In sandbox, symlink requires rw permissions on the directory of the sources to create a symlink. Copy does not require.
To test, you can build this branch, and try to install bls12-381-unix or bls12-381-js.

In addition to the comments in #4195, this issue has not been detected by the CI of bls12-381 (installing via opam is verified here: https://gitlab.com/dannywillems/ocaml-bls12-381/-/blob/master/.gitlab-ci.yml#L136) or by the CI of the public opam-repository (see ocaml/opam-repository#18112) because I think sandbox is not used.

[ghost]

IIUC, Dune is trying to create a symlink from its build directory pointing to $(opam config var lib)/some-lib/some-file and that fails because the destination is part of a sandbox created by opam. Is that correct?

[dannywillems]

IIUC, Dune is trying to create a symlink from its build directory pointing to $(opam config var lib)/some-lib/some-file and that fails because the destination is part of a sandbox created by opam. Is that correct?

No, dune is trying to create a symlink from $(opam config var lib)/some-lib/some-file (the cmi/cmx files in the virtual package in this case) to its build directory (the temporary build directory for the implementation), and after that, it looks like a chmod is performed on the symlink (IIUC). To detect this chmod, I noticed the error was raised here, and caused by the line https://github.com/ocaml/dune/blob/main/src/dune_engine/build_system.ml#L558. I ("stupidly") changed the line 558 to raise the exception instead and it gives a chmod: operation not permitted on _build/default/src/unix/.bls12_381_unix.objs/native/bls12_381.o (for instance, but always in the temporary build directory of dune). So it looks like it tries to change the permissions on the link previously created, and the link is pointed to a readonly file.
To verify it is not possible to execute chmod on a symlink pointed to a file in a readonly directory (400, I suppose that's what ro means in the bwrap/sandbox-exec), I tried:

mkdir /tmp/source /tmp/target
touch /tmp/source/test
chmod 400 /tmp/source/
ln -s /tmp/source/test /tmp/target/test  # works fine
chmod 600 /tmp/target/test

and I got:

chmod: cannot access '/tmp/target/test': Permission denied

Changing the permissions on /tmp/source to 700 for instance allows chmod 600 /tmp/target/test. Note that it does change the permissions on /tmp/source/test, not on /tmp/target/test.

➜  /tmp ls -al /tmp/source
total 0
drwx------  3 dannywillems wheel  96 Feb 15 21:45 .
drwxrwxrwt 12 root         wheel 384 Feb 15 21:45 ..
-rwx------  1 dannywillems wheel   0 Feb 15 21:45 test

➜  /tmp ls -al /tmp/target
total 0
drwxr-xr-x  3 dannywillems wheel  96 Feb 15 21:45 .
drwxrwxrwt 12 root         wheel 384 Feb 15 21:45 ..
lrwxr-xr-x  1 dannywillems wheel  16 Feb 15 21:45 test -> /tmp/source/test

I didn't go that much deep in the code of dune, and my explanation should be narrowed IMHO to find, maybe, a better solution.

[bobot]

To sum-up:

• dune is using symlink to get the interface of installed virtual library
• This file is the target (of this rule or another I don't know)
• Dune makes targets read-only in order to catch error sooner
• chmod on a symlink change the permission of the source which is outside dune build directory (which is bad in itself).

Using a copy here is indeed a quick fix, but I wonder if this problem can't arrive in other cases.

[ghost]

Using a copy here is indeed a quick fix, but I wonder if this problem can't arrive in other cases.

I was wondering this as well. I feel like we should simply never try to chmod symlinks. Looking at the code, we already try to avoid chmod-ing files that are not regular files, however we use Unix.stat rather than Unix.lstat, which means that we miss symlinks. More precisely, this is in the function refresh_and_chmod in src/dune/cached_digest.ml. @dannywillems could you try replacing the Path.stat by a Path.lstat in this function and see if it fixes the issue? (You need to add Path.lstat, in src/stdune/path.mli).

[dannywillems]

Using a copy here is indeed a quick fix, but I wonder if this problem can't arrive in other cases.

I was wondering this as well. I feel like we should simply never try to chmod symlinks. Looking at the code, we already try to avoid chmod-ing files that are not regular files, however we use Unix.stat rather than Unix.lstat, which means that we miss symlinks. More precisely, this is in the function refresh_and_chmod in src/dune/cached_digest.ml. @dannywillems could you try replacing the Path.stat by a Path.lstat in this function and see if it fixes the issue? (You need to add Path.lstat, in src/stdune/path.mli).

I confirm this works. Do you prefer this fix?

[ghost]

Yes, that's a more general fix. Plus using symlinks here is more efficient.

[dannywillems]

Yes, that's a more general fix. Plus using symlinks here is more efficient.

Perfect, i updated the branch.

[ghost]

Looks good, thanks!

[dannywillems]

Thanks for accepting this PR @jeremiedimino. Do you have any plan for a new release of dune in the public opam repository with this fix? It is blocking for some MR in Tezos.

[ghost]

Rudi answered this by adding the 2.8.3 milestone. So it's going to be part of the next bugfix release.