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

[JENKINS-36088] Use NIO implementations of chmod and mode by default #3135

Merged
merged 19 commits into from Dec 1, 2017

Conversation

4 participants
@dwnusbaum
Copy link
Member

dwnusbaum commented Nov 9, 2017

See JENKINS-36088.

This PR changes the default implementations of FilePath#chmod and IOUtils#mode to use NIO. Because NIO does not provide a way to set the setgid/setuid/sticky bits, this could potentially cause issues, so setting -Dhudson.Util.useNativeChmodAndMode=true reverts to the native implementations.

Potential issues:

  • Files archived and then unarchived will lose their setgid/setuid/sticky bits.
  • Files copied from one Unix machine to another with FilePath#copyToWithPermissions will lose their setgid/setuid/sticky bits.
  • Calls to IOUtils#mode will no longer include file type bits (i.e. 0o100000 for a regular file, 0o120000 for symbolic links, etc. (See man 2 stat, search for st_mode). We could return those bits for symbolic links, directories, and regular files, but we do not have a way to detect the other file types.

Proposed changelog entries

  • Use NIO to read and write Unix file permissions by default. The old behavior can be restored with -Dhudson.Util.useNativeChmodAndMode=true.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
    * Should we do a run of the PCT to make sure plugins aren't trying to set invalid bits? (I didn't notice any invalid usages in a search of the jenkinsci org, but wouldn't hurt to run the tests.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Nov 9, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

for (int i = 0; i < allPermissions.length; i++) {
result <<= 1;
result |= permissions.contains(allPermissions[i]) ? 1 : 0;
}

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 9, 2017

Author Member

If the bitwise operations here seem confusing I am happy to introduce constants and get rid of the loop for clarity.

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

You wrote testModeToPermissions, so good enough for me.

if ((mode & 1) == 1) {
result.add(allPermissions[allPermissions.length - i - 1]);
}
mode >>= 1;

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 9, 2017

Author Member

Same as above regarding bitwise operations.

* overwritten by Jenkins erroneously.
*/
@Restricted(value = NoExternalUse.class)
public static boolean NATIVE_CHMOD_MODE = SystemProperties.getBoolean(Util.class.getName() + ".useNativeChmodAndMode");

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 9, 2017

Author Member

I hate to introduce another system property... Is there a better way to this?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

Fine for now. I'd guess at some point we will create GUI configurer for System properties. It is my TODO list for years :(

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

FindBugs should als grumble that the field is not final. Does not matter for now, FindBugs cleanup still needs to be done in the core

@@ -242,7 +242,7 @@ public void close() throws IOException {
throw x;
}
} finally {
toF.chmod(700);
toF.chmod(0700);

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 9, 2017

Author Member

Not necessary but looked like a typo in the original test.

This comment has been minimized.

@jglick
assumeFalse(Functions.isWindows());
File f = temp.newFolder("folder");
FilePath fp = new FilePath(f);
int invalidMode = 01770; // Full permissions for owner and group plus sticky bit.

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 9, 2017

Author Member

If we decide not to merge this PR because of the setgid/setuid/sticky bit issues, could still be worth adding this test after inverting the logic as a regression test for the future.

assertEquals(dirMode,e.child("subdir").mode());
assertEquals(0100644,e.child("subdir/b.txt").mode());
assertEquals(0644,e.child("subdir/b.txt").mode());

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 9, 2017

Author Member

We could try to set the file type bits in the new IOUtils#mode, but we would only be able to reliably set the type for regular files, symbolic links, and directories, so it seems better to only return the basic permissions and document that fact.

@dwnusbaum dwnusbaum changed the title [JENKINS-36088] Use NIO implementations of `FilePath#chmod` and `IOUtils#mode` by default [JENKINS-36088] Use NIO implementations of FilePath#chmod and IOUtils#mode by default Nov 9, 2017

@dwnusbaum dwnusbaum changed the title [JENKINS-36088] Use NIO implementations of FilePath#chmod and IOUtils#mode by default [JENKINS-36088] Use NIO implementations of chmod and mode by default Nov 9, 2017

try {
File targetFile = new File(target.remote).getAbsoluteFile();
mkdirs(targetFile.getParentFile());
Files.copy(reading(f).toPath(), writing(targetFile).toPath(), StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING);

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 10, 2017

Author Member

COPY_ATTRIBUTES does preserve setgid/setuid/sticky bits, unlike setPosixFilePermissions.
I don't see a likely use case for maintaining the special permission bits when copying across different machines, but on the same machine I think it makes sense, and this should help prevent regressions for valid use cases of the special bits.

@dwnusbaum dwnusbaum requested a review from jglick Nov 10, 2017

@oleg-nenashev oleg-nenashev self-requested a review Nov 12, 2017

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

Needs a change in mkdirs() and probably refactoring of Runtime exception handling to a utility class.

} else {
try {
Files.setPosixFilePermissions(f.toPath().toAbsolutePath(), Util.modeToPermissions(mask));
} catch (InvalidPathException e) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

I would propose to move it to a Restricted utility method, because it is going to be endless fun in this API.
Something like that: https://github.com/jenkinsci/remoting/blob/master/src/main/java/org/jenkinsci/remoting/util/PathUtils.java

public Void invoke(File f, VirtualChannel channel) throws IOException {
try {
File targetFile = new File(target.remote).getAbsoluteFile();
mkdirs(targetFile.getParentFile());

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

Should be Files.createDirectory(), otherwise you miss the directory creation failure event 🐛

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 15, 2017

Author Member

Yes good catch, I was originally going to update the implementation of mkdirs(File) in this PR to use Files#createDirectories but did not yet do so. I'll fix it.

throw new IOException("Invalid mode: " + mode);
}
PosixFilePermission[] allPermissions = PosixFilePermission.values();
Set<PosixFilePermission> result = EnumSet.noneOf(PosixFilePermission.class);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

Is it covered by tests? https://docs.oracle.com/javase/7/docs/api/java/util/EnumSet.html#noneOf(java.lang.Class) is not explicit, but I am aware that some modifications can return a non-modifyable set (performance tweaks).

This comment has been minimized.

@dwnusbaum

dwnusbaum Nov 15, 2017

Author Member

There is a new test for the method (UtilTest#testPermissionsToMode) which should cover modification, but I will double check the coverage report. I can update to use a HashSet if you prefer.

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

It is a restricted method so I am not concerned.

* overwritten by Jenkins erroneously.
*/
@Restricted(value = NoExternalUse.class)
public static boolean NATIVE_CHMOD_MODE = SystemProperties.getBoolean(Util.class.getName() + ".useNativeChmodAndMode");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

Fine for now. I'd guess at some point we will create GUI configurer for System properties. It is my TODO list for years :(

* overwritten by Jenkins erroneously.
*/
@Restricted(value = NoExternalUse.class)
public static boolean NATIVE_CHMOD_MODE = SystemProperties.getBoolean(Util.class.getName() + ".useNativeChmodAndMode");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

FindBugs should als grumble that the field is not final. Does not matter for now, FindBugs cleanup still needs to be done in the core

if(Functions.isWindows()) return -1;
return PosixAPI.jnr().stat(f.getPath()).mode();
try {
if (Functions.isWindows()) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 15, 2017

Member

Should be outside try/catch

@jglick

jglick approved these changes Nov 15, 2017

@@ -1582,6 +1583,11 @@ public long getUsableDiskSpace() throws IOException, InterruptedException {
* <p>
* please note mask is expected to be an octal if you use <a href="http://en.wikipedia.org/wiki/Chmod">chmod command line values</a>,
* so preceded by a '0' in java notation, ie <code>chmod(0644)</code>
* <p>
* Only supports setting read, write, or execute permissions for the
* owner, group, or others, so the largest permissibly value is 0777.

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

permissible

* Only supports setting read, write, or execute permissions for the
* owner, group, or others, so the largest permissibly value is 0777.
* Attempting to set larger values (i.e. the setgid, setuid, or sticky
* bits will cause an IOException to be thrown

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

missing )?

@@ -1688,6 +1689,52 @@ public static void closeAndLogFailures(@CheckForNull Closeable toClose, @Nonnull
}
}

@Restricted(NoExternalUse.class)
public static int permissionsToMode(Set<PosixFilePermission> permissions) {

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

Surprises me that PosixFilePermissions does not include this obvious-seeming function.

for (int i = 0; i < allPermissions.length; i++) {
result <<= 1;
result |= permissions.contains(allPermissions[i]) ? 1 : 0;
}

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

You wrote testModeToPermissions, so good enough for me.

if (Util.NATIVE_CHMOD_MODE) {
PosixAPI.jnr().chmod(f.getAbsolutePath(), mask);
} else {
Files.setPosixFilePermissions(Util.fileToPath(f).toAbsolutePath(), Util.modeToPermissions(mask));

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

Why the call toAbsolutePath? Should be unnecessary.

if (this.channel == target.channel) {
act(new SecureFileCallable<Void>() {
public Void invoke(File f, VirtualChannel channel) throws IOException {
File targetFile = new File(target.remote).getAbsoluteFile();

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

As above, I doubt the call to getAbsoluteFile actually does anything here.

public static Set<PosixFilePermission> modeToPermissions(int mode) throws IOException {
// Anything larger is a file type, not a permission.
int PERMISSIONS_MASK = 07777;
// setigd/setuid/sticky are not supported.

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

setgid

int PERMISSIONS_MASK = 07777;
// setigd/setuid/sticky are not supported.
// rwxrwxrwx
int MAX_SUPPORTED_MODE = 0b111111111;

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member
0777

right?

throw new IOException("Invalid mode: " + mode);
}
PosixFilePermission[] allPermissions = PosixFilePermission.values();
Set<PosixFilePermission> result = EnumSet.noneOf(PosixFilePermission.class);

This comment has been minimized.

@jglick

jglick Nov 15, 2017

Member

It is a restricted method so I am not concerned.

@@ -242,7 +242,7 @@ public void close() throws IOException {
throw x;
}
} finally {
toF.chmod(700);
toF.chmod(0700);

This comment has been minimized.

@jglick

@dwnusbaum dwnusbaum requested a review from oleg-nenashev Nov 16, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Nov 16, 2017

@dwnusbaum resolved the merge conflict, will take a look at changes

@oleg-nenashev oleg-nenashev requested a review from jglick Nov 16, 2017

@jglick

jglick approved these changes Nov 17, 2017

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

dwnusbaum commented Nov 27, 2017

@oleg-nenashev I believe I've addressed your requested changes, can you please take another look?

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

Yes, IMHO it is ready to go. 🐝 and @reviewbybees done

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Nov 28, 2017

Will merge it into the next weekly if there is no negative feedback

@oleg-nenashev oleg-nenashev merged commit fdccc0e into jenkinsci:master Dec 1, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@jglick jglick referenced this pull request Mar 14, 2018

Merged

[JEP-202] Extending VirtualFile #3302

6 of 6 tasks complete
@jglick

This comment has been minimized.

Copy link
Member

jglick commented Mar 14, 2018

Note that in #3302 I am clarifying the Javadoc of mode.

@dwnusbaum dwnusbaum referenced this pull request Jun 21, 2018

Open

[JENKINS-46725] stop relying on jnr-posix #3511

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.