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

copyExecutable function overwrites file perfmission for non owners #125

Open
xerial opened this issue Jan 23, 2018 · 14 comments
Open

copyExecutable function overwrites file perfmission for non owners #125

xerial opened this issue Jan 23, 2018 · 14 comments
Labels
Needinfo uncategorized Used for Waffle integration

Comments

@xerial
Copy link

xerial commented Jan 23, 2018

In the current implementation, copyExecutable does not favor the original file permission except the file owner:

  /** Transfers the executable property of `sourceFile` to `targetFile`. */
  def copyExecutable(sourceFile: File, targetFile: File) = {
    val executable = sourceFile.canExecute
    if (executable) targetFile.setExecutable(true) /// equivalent to  setExecutable(executable=true, ownerOnly=true)
  }

For example, if the original file permission is 775 (rwxrwxr-x), and copied by sbt.io.IO.copyExecutable, it will results in 764 (rwxrw-r--) executable flag are removed from the group and others).

We've found this issue in xerial/sbt-pack#135

@xerial
Copy link
Author

xerial commented Jan 23, 2018

It seems in sbt 0.13.x, the file permission was retained.

@xerial
Copy link
Author

xerial commented Jan 23, 2018

We need to retain the original file permission before setting these flags. I'll create a PR to fix this.

@xerial
Copy link
Author

xerial commented Jan 23, 2018

@eed3si9n One question. Should we add preserverWritable flag in addition to preserveExecutable?

@eed3si9n
Copy link
Member

If you need more detailed permission, maybe use POSIX permission operations that were added in #76?

@xerial
Copy link
Author

xerial commented Jan 23, 2018

@eed3si9n Yes. That is an option as well.

If disregarding the source file permission is the expected behavior, sbt-pack sets the permission manually. If not, sbt-io needs to be modified to retain the original permission.

@xerial
Copy link
Author

xerial commented Jan 23, 2018

For preserving the original file permission, setting preserveExecutable=false conflicts with preservePermssion (like cp -p) behavior. That's my concern.

@eed3si9n
Copy link
Member

In sbt 0.13 there was no parameter on copyFile to set the execution flag.

If disregarding the source file permission is the expected behavior

Disregarding the source file permission, sort of is the behavior of File#setExecutable flag. Looking at the history, this was introduced as #53 in sbt 1 to as part of forward porting (sbt/sbt#2384, sbt/sbt@1e61b9a).

In reality, as you point out, the result came out to be inconsistent and clumsy at least on the system that you're testing (macOS?). I am not sure how we should proceed on this.
I think not copying when perserveExecutable = false would even be more confusing if the existing code relies on the behavior.

At the least, copyExecutable should NOT lose permission.

/cc @dwijnand

@eed3si9n eed3si9n added the Bug label Jan 23, 2018
@eed3si9n
Copy link
Member

It seems in sbt 0.13.x, the file permission was retained.

Is this your observation from running the code?
I haven't tested it, but I am not seeing the code that's responsible for transferring the permissions in 0.13 - https://github.com/sbt/sbt/blob/v0.13.17-RC2/util/io/src/main/scala/sbt/IO.scala#L647-L669

@xerial
Copy link
Author

xerial commented Jan 23, 2018

@eed3si9n 0.13 behavior is reported by a sbt-pack user, which simply uses IO.copyFile.

I submitted a PR for retaining the original file permission #126 .

@eed3si9n
Copy link
Member

I just tested on macOS, and I was not able to reproduce the behavior that preserves the permission in 0.13.

scala> IO.copyFile(new File("/tmp/io/A.sh"), new File("/tmp/io/B.sh"), false)

scala> IO.copyFile(new File("/tmp/io/A.sh"), new File("/tmp/io/C.sh"), true)

Here's the result:

-rwxrwxrwx   1 eed3si9n  wheel    0 Jan 23 18:08 A.sh*
-rw-r--r--   1 eed3si9n  wheel    0 Jan 23 18:10 B.sh
-rw-r--r--   1 eed3si9n  wheel    0 Jan 23 18:08 C.sh

@xerial
Copy link
Author

xerial commented Jan 23, 2018

@eed3si9n OK. So this behavior has been the same for a long time. It means #126 is good to have, but it may break the behavior of existing sbt plugins.

@eed3si9n
Copy link
Member

Right. also it makes copying POSIX specific, so Windows ppl might run into issues.

@eed3si9n eed3si9n added Needinfo and removed Bug labels Jan 23, 2018
@eed3si9n
Copy link
Member

Pending question here - xerial/sbt-pack#135 (comment)

@eed3si9n eed3si9n added the uncategorized Used for Waffle integration label Sep 18, 2018
@dskrvk
Copy link

dskrvk commented Jul 30, 2021

@eed3si9n I responded in the referenced issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needinfo uncategorized Used for Waffle integration
Projects
None yet
Development

No branches or pull requests

3 participants