file copy may not always chmod permissions

[stakach]

the remote volume may allow copy but not changes to file permissions after writing
as is the case with azure cloud storage volumes attached to docker containers

Error changing permissions: '/app/www/1740354627047_3895/scripts/local_auth.js': Operation not permitted (File::Error)
  from /usr/share/crystal/src/crystal/system/unix/file.cr:127:7 in 'system_chmod'
  from /usr/share/crystal/src/file.cr:981:5 in 'chmod'
  from /usr/share/crystal/src/file.cr:855:9 in 'copy'
  from /usr/share/crystal/src/file_utils.cr:94:5 in 'cp'
  from /usr/share/crystal/src/file_utils.cr:136:7 in 'cp_recursive'
  from /usr/share/crystal/src/file_utils.cr:133:9 in 'cp_recursive'
  from /usr/share/crystal/src/file_utils.cr:133:9 in 'cp_recursive'
  from /usr/share/crystal/src/file_utils.cr:124:5 in 'cp_r'
  from /usr/share/crystal/src/file_utils.cr:321:7 in 'mv'
  from lib/git-repository/src/git-repository/generic.cr:94:7 in 'move_into_place'
  from lib/git-repository/src/git-repository/generic.cr:121:7 in 'fetch_commit'
  from src/placeos-frontend-loader/loader.cr:73:11 in 'create_base_www'
  from src/placeos-frontend-loader/loader.cr:52:7 in 'start'

[ysbaddaden]

This is a breaking change: we document that src permissions will be set on dst. Silencing the error means that they may or may not be set and you won't know about it.

[straight-shoota]

Yeah, I don't think swallowing errors is an option. Usually you would want to know if things break at this point.

This is a use-case specific issue, and I'm not sure if there could be a general solution. An application-specific implementation where you explicitly specify that you only want to copy the data and not the permissions might be the only reasonable way to handle this.

It would be interesting to look how other file copy tools (the equivalent in other programing languages, but also programs like cp or rsync) handle this case.

[ysbaddaden]

@straight-shoota Both cp and rsync need an explicit arg to preserve permissions, respectively --preserve and --perms. The man pages don't state what happens when the permissions (and other xattr and acls) can't be set. I'd expect them to fail but that needs to be verified.

[HertzDevil]

Can we add an optional preserve : Bool = true parameter?

And what about the flush call above?

[ysbaddaden]

@HertzDevil yes, though "preserve" means more than just mode.

For example cp:

preserve the specified attributes (default: mode, ownership, timestamps), if possible additional attributes: context, links, xattr, all

And rync has different args: --perms, --acls, --xattrs, ...

[stakach]

ruby has a preserve option that applies the chmod as well as timestamps etc
It defaults to false too

https://github.com/ruby/ruby/blob/master/lib/fileutils.rb#L1077-L1082

  def copy_file(src, dest, preserve = false, dereference = true)
    ent = Entry_.new(src, nil, dereference)
    ent.copy_file dest
    ent.copy_metadata dest if preserve
  end

[straight-shoota]

Enhancing the API could be a great improvement, let's continue that discussion #15518.
However, I believe we can fix the problem of this use case by correcting assumptions in the current implementation.

File.copy opens two file descriptors (to source and destination), copies the data, then copies the permissions from source to destination.

crystal/src/file.cr

Lines 847 to 858 in 62c92a0

	def self.copy(src : String | Path, dst : String | Path) : Nil
	open(src) do |s|
	open(dst, "wb") do |d|
	# TODO use sendfile or copy_file_range syscall. See #8926, #8919
	IO.copy(s, d)
	d.flush # need to flush in case permissions are read-only
	# Set the permissions after the content is written in case src permissions is read-only
	d.chmod(s.info.permissions)
	end
	end
	end

The code comment explains that we only apply the permissions after writing the content to prevent write restrictions from applied permissions.

But that's unnecessary. Changing the permissions on disk does not have any effect on existing file descriptors. The OS only checks permissions when opening a file.

Combine that with telling File.open directly which permissions we want for a new file: This applies directly at file creation time and does not involve additional calls to chmod. The file descriptor itself has still full permissions. We wouldn't be restricting ourselves to read-only access.

File.open won't apply permissions if the file already exists, though. So we still need to explicitly apply the intended permissions sometimes: when overwriting an existing file if it has different permissions.
This can happen before writing the data which reduces the chance for race conditions.

@stakach Please check if #15520 solves your problem.

[ysbaddaden]

@straight-shoota I'm not sure this stands: by the time we call d.chmod the file descriptor hasn't been closed either, be it before after calling IO.copy or calling d.flush 😕

[straight-shoota]

Of course it depends on what exactly doesn't work in this specific setup.
But chances are that we never need to modify permissions at all.
With #15520 we only chmod if a file preexists at the destination path and has different permissions than the source file.