Restore procedure fails when bundle path is a symbolic link

[szymon-mulczynski-red]

I have crun 1.4.2 on an embedded device environment, where 'bundle' provided on container create and container restore is an absolute path, containing symbolic link (security reasons). In my case the path pattern is: /opt/persistent/mydirectory/AppName/Container. In this path 'Container' is a symlink to the: /opt/persistent/ContainerBundles/AppName.

On restore, the procedure fails due to an error: target 'proc' not under the directory 'opt/persistent/mydirectory/AppName/Container/criu-root'

As I understand the code, it comes from the fact, that utils.c::check_fd_under_path() compares the 'link' taken from the fd (which will give the realpath) with the 'rootfs' taken from the bundle path (a symbolic link given as an argument), using a simple memcmp. Both paths does not match. so the comparison fails although both lead to the same file/folder.

I found that restore.c::crun_command_restore() resolves the realpath only in 2 cases - if there is no bundle provided or we have a relative path:

  if (bundle == NULL)
    {
      bundle = realpath (".", NULL);
    }
  else
    {
      if (bundle[0] != '/')
        {
          bundle_cleanup = realpath (bundle, NULL);
          if (bundle_cleanup == NULL)
            libcrun_fail_with_error (errno, "realpath `%s` failed", bundle);
          bundle = bundle_cleanup;
        }

What if there is an absolute path, which is a symbolic link? Why there is an if statement to check we have a relative path before the 'bundle cleanup'? Removing the statement solves my problem, should I consider this a fix and provide a pull request to crun or the problem is more complicated?

[giuseppe]

the reason why it is not done by default is to avoid the cost of realpath if not needed.

If you know the bundle directory is a symlink, why don't you resolve it before calling into crun crun run -b $(readlink -f $BUNDLE)?

[szymon-mulczynski-red]

In my casecrun is called from the Dobby tool, so it would be rather a question which tool has to be changed, not a simple command. I took a look again and I see the same behavior is done on container create - realpath only in case of relative paths, so I think I missed with the fix. I found an inconsistency here:

On container start, the container.c::container_init_setup calls the

  if (def->root && def->root->path)
    {
      rootfs = realpath (def->root->path, NULL);  
  [...]
  ret = libcrun_set_mounts (container, rootfs, send_sync_cb, &sync_socket, err);

so the 'realpath' is done always and on container init, mounts are created based on the real, absolute path -> in my case it is /opt/persistent/ContainerBundles/AppName.

During the restore, criu.c::libcrun_container_restore_linux_criu has a mention about a 'realpath' but the truth is 'root' contains the string passed to crun as a bundle -> in my case it is the /opt/persistent/mydirectory/AppName/Container . The 'root' is passed to 'prepare_restore_mounts' which calls 'safe_openat' under the hood which leads to unfortunate comparison in 'check_fd_under_path'.

Don't you think there is an inconsistency between the 'libcrun_set_mounts' and 'prepare_restore_mounts' and they should operate on the same path? What does the /* do realpath on root */ mean in practice as prints show the 'root' passed further is still a symlink?

[giuseppe]

I think runc doesn't even allow a symlink in the rootfs and it fails immediately.

Wouldn't it make more sense to do the realpath in dubby so to be compatible with more OCI runtimes?

[szymon-mulczynski-red]

Yes, this change could be done in Dobby, where restore is called. I just wonder and try to understand why crun is able to handle a path given as '--bundle' on create and the same path given as '--bundle' on restore does not work.

[giuseppe]

I don't see any difference in the way restore handles the path compared to create.

Is the issue related to restore or to absolute paths containing symlinks?

[szymon-mulczynski-red]

This issue is related only to restore. The same path provided for create works fine in every case.

The only difference I found is the way how mountpoints are done - I described it above.

[giuseppe]

would it work if we just use realpath for restore?

If you already have a patch, could you please open a PR?

[giuseppe]

I am fine doing it there, as it won't affect containers startup time

[szymon-mulczynski-red]

I created a pull request with my proposition: https://github.com/containers/crun/pull/982

[giuseppe]

closing since #982 is merged