stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8322565: (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits

Browse Source 
Reviewed-by: clanger, lancea
This commit is contained in:
Eirik Bjørsnøs 2024-01-10 21:42:23 +00:00
parent d89602a53f
commit e70cb4e6c7
2 changed files with 136 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
View File

@ -1,5 +1,5 @@
/* /*
* Copyright (c) 2009, 2023, Oracle and/or its affiliates. All rights reserved. * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
* *
* This code is free software; you can redistribute it and/or modify it * This code is free software; you can redistribute it and/or modify it
@ -644,7 +644,12 @@ class ZipFileSystem extends FileSystem {
if (e.type == Entry.CEN) { if (e.type == Entry.CEN) {
e.type = Entry.COPY; // copy e e.type = Entry.COPY; // copy e
} }
e.posixPerms = perms == null ? -1 : ZipUtils.permsToFlags(perms); if (perms == null) {
e.posixPerms = -1;
} else {
e.posixPerms = ZipUtils.permsToFlags(perms) |
(e.posixPerms & 0xFE00); // Preserve unrelated bits
}
update(e); update(e);
} finally { } finally {
endWrite(); endWrite();
@ -3008,7 +3013,7 @@ class ZipFileSystem extends FileSystem {
attrsEx = CENATX(cen, pos); attrsEx = CENATX(cen, pos);
*/ */
if (CENVEM_FA(cen, pos) == FILE_ATTRIBUTES_UNIX) { if (CENVEM_FA(cen, pos) == FILE_ATTRIBUTES_UNIX) {
posixPerms = CENATX_PERMS(cen, pos) & 0xFFF; // 12 bits for setuid, setgid, sticky + perms posixPerms = (CENATX_PERMS(cen, pos) & 0xFFFF); // 16 bits for file type, setuid, setgid, sticky + perms
} }
locoff = CENOFF(cen, pos); locoff = CENOFF(cen, pos);
pos += CENHDR; pos += CENHDR;

161
test/jdk/jdk/nio/zipfs/TestPosix.java
View File

@ -1,5 +1,5 @@
/* /*
* Copyright (c) 2019, 2022, SAP SE. All rights reserved. * Copyright (c) 2019, 2024, SAP SE. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
* *
* This code is free software; you can redistribute it and/or modify it * This code is free software; you can redistribute it and/or modify it
@ -25,23 +25,16 @@ import java.io.File;
import java.io.FileOutputStream; import java.io.FileOutputStream;
import java.io.IOException; import java.io.IOException;
import java.io.InputStream; import java.io.InputStream;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.file.*; import java.nio.file.*;
import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.*;
import java.nio.file.attribute.GroupPrincipal;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFileAttributes;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.nio.file.attribute.UserPrincipal;
import java.security.AccessController; import java.security.AccessController;
import java.security.PrivilegedAction; import java.security.PrivilegedAction;
import java.security.PrivilegedActionException; import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction; import java.security.PrivilegedExceptionAction;
import java.util.Collections; import java.time.Instant;
import java.util.Enumeration; import java.util.*;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicInteger;
import java.util.jar.JarEntry; import java.util.jar.JarEntry;
import java.util.jar.JarFile; import java.util.jar.JarFile;
@ -49,7 +42,7 @@ import java.util.spi.ToolProvider;
import java.util.zip.ZipEntry; import java.util.zip.ZipEntry;
import java.util.zip.ZipFile; import java.util.zip.ZipFile;
import org.testng.annotations.Test; import org.junit.jupiter.api.Test;
import static java.nio.file.attribute.PosixFilePermission.GROUP_EXECUTE; import static java.nio.file.attribute.PosixFilePermission.GROUP_EXECUTE;
import static java.nio.file.attribute.PosixFilePermission.GROUP_READ; import static java.nio.file.attribute.PosixFilePermission.GROUP_READ;
@ -60,11 +53,11 @@ import static java.nio.file.attribute.PosixFilePermission.OTHERS_WRITE;
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE; import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
import static org.testng.Assert.assertEquals; import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.testng.Assert.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.testng.Assert.assertNull; import static org.junit.jupiter.api.Assertions.assertNull;
import static org.testng.Assert.assertTrue; import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.testng.Assert.fail; import static org.junit.jupiter.api.Assertions.fail;
/** /**
* @test * @test
@ -72,8 +65,8 @@ import static org.testng.Assert.fail;
* @summary Test POSIX ZIP file operations. * @summary Test POSIX ZIP file operations.
* @modules jdk.zipfs * @modules jdk.zipfs
* jdk.jartool * jdk.jartool
* @run testng TestPosix * @run junit TestPosix
* @run testng/othervm/java.security.policy=test.policy.posix TestPosix * @run junit/othervm/java.security.policy=test.policy.posix TestPosix
*/ */
public class TestPosix { public class TestPosix {
private static final ToolProvider JAR_TOOL = ToolProvider.findFirst("jar") private static final ToolProvider JAR_TOOL = ToolProvider.findFirst("jar")
@ -364,7 +357,7 @@ public class TestPosix {
fail("Caught IOException reading file attributes (basic) " + name + ": " + e.getMessage()); fail("Caught IOException reading file attributes (basic) " + name + ": " + e.getMessage());
} }
} }
assertEquals(Files.isDirectory(file), ei.isDir, "Unexpected directory attribute for:" + System.lineSeparator() + attrs); assertEquals(ei.isDir, Files.isDirectory(file), "Unexpected directory attribute for:" + System.lineSeparator() + attrs);
if (expected == checkExpects.contentOnly) { if (expected == checkExpects.contentOnly) {
return; return;
@ -402,7 +395,7 @@ public class TestPosix {
}); });
} }
System.out.println("Number of entries: " + entries.get() + "."); System.out.println("Number of entries: " + entries.get() + ".");
assertEquals(entries.get(), entriesCreated, "File contained wrong number of entries."); assertEquals(entriesCreated, entries.get(), "File contained wrong number of entries.");
} }
private void checkEntries(FileSystem fs, checkExpects expected) throws IOException { private void checkEntries(FileSystem fs, checkExpects expected) throws IOException {
@ -429,7 +422,7 @@ public class TestPosix {
assertNull(actual, "Permissions are not null"); assertNull(actual, "Permissions are not null");
} else { } else {
assertNotNull(actual, "Permissions are null."); assertNotNull(actual, "Permissions are null.");
assertEquals(actual.size(), expected.size(), "Unexpected number of permissions (" + assertEquals(expected.size(), actual.size(), "Unexpected number of permissions (" +
actual.size() + " received vs " + expected.size() + " expected)."); actual.size() + " received vs " + expected.size() + " expected).");
for (PosixFilePermission p : expected) { for (PosixFilePermission p : expected) {
assertTrue(actual.contains(p), "Posix permission " + p + " missing."); assertTrue(actual.contains(p), "Posix permission " + p + " missing.");
@ -609,18 +602,18 @@ public class TestPosix {
var owner = Files.getOwner(entry); var owner = Files.getOwner(entry);
assertNotNull(owner, "owner should not be null"); assertNotNull(owner, "owner should not be null");
if (defaultOwner != null) { if (defaultOwner != null) {
assertEquals(owner.getName(), defaultOwner); assertEquals(defaultOwner, owner.getName());
} }
Files.setOwner(entry, DUMMY_USER); Files.setOwner(entry, DUMMY_USER);
assertEquals(Files.getOwner(entry), DUMMY_USER); assertEquals(DUMMY_USER, Files.getOwner(entry));
var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class); var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class);
var group = view.readAttributes().group(); var group = view.readAttributes().group();
assertNotNull(group, "group must not be null"); assertNotNull(group, "group must not be null");
if (defaultGroup != null) { if (defaultGroup != null) {
assertEquals(group.getName(), defaultGroup); assertEquals(defaultGroup, group.getName());
} }
view.setGroup(DUMMY_GROUP); view.setGroup(DUMMY_GROUP);
assertEquals(view.readAttributes().group(), DUMMY_GROUP); assertEquals(DUMMY_GROUP, view.readAttributes().group());
entry = zipIn.getPath("/uexec"); entry = zipIn.getPath("/uexec");
Files.setPosixFilePermissions(entry, GR); // will be persisted Files.setPosixFilePermissions(entry, GR); // will be persisted
comparePermissions(GR, Files.getPosixFilePermissions(entry)); comparePermissions(GR, Files.getPosixFilePermissions(entry));
@ -632,9 +625,9 @@ public class TestPosix {
{ {
var entry = zipIn.getPath("/noperms"); var entry = zipIn.getPath("/noperms");
comparePermissions(UR, Files.getPosixFilePermissions(entry)); comparePermissions(UR, Files.getPosixFilePermissions(entry));
assertEquals(Files.getOwner(entry).getName(), "auser"); assertEquals("auser", Files.getOwner(entry).getName());
var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class); var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class);
assertEquals(view.readAttributes().group().getName(), "agroup"); assertEquals("agroup", view.readAttributes().group().getName());
// check if the change to permissions of /uexec was persisted // check if the change to permissions of /uexec was persisted
comparePermissions(GR, Files.getPosixFilePermissions(zipIn.getPath("/uexec"))); comparePermissions(GR, Files.getPosixFilePermissions(zipIn.getPath("/uexec")));
} }
@ -645,9 +638,9 @@ public class TestPosix {
{ {
var entry = zipIn.getPath("/noperms"); var entry = zipIn.getPath("/noperms");
comparePermissions(UR, Files.getPosixFilePermissions(entry)); comparePermissions(UR, Files.getPosixFilePermissions(entry));
assertEquals(Files.getOwner(entry), DUMMY_USER); assertEquals(DUMMY_USER, Files.getOwner(entry));
var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class); var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class);
assertEquals(view.readAttributes().group(), DUMMY_GROUP); assertEquals(DUMMY_GROUP, view.readAttributes().group());
} }
} }
@ -722,6 +715,108 @@ public class TestPosix {
// the run method catches IOExceptions, we need to expose them // the run method catches IOExceptions, we need to expose them
int rc = JAR_TOOL.run(System.out, System.err, "xvf", JAR_FILE.toString()); int rc = JAR_TOOL.run(System.out, System.err, "xvf", JAR_FILE.toString());
assertEquals(rc, 0, "Return code of jar call is " + rc + " but expected 0"); assertEquals(0, rc, "Return code of jar call is " + rc + " but expected 0");
}
/**
* Verify that calling Files.setPosixPermissions with the current
* permission set does not change the 'external file attributes' field.
*
* @throws IOException if an unexpected IOException occurs
*/
@Test
public void setPermissionsShouldPreserveRemainingBits() throws IOException {
assertExternalFileAttributeUnchanged(fs -> {
Path path = fs.getPath("hello.txt");
// Set permissions to their current value
Files.setPosixFilePermissions(path, Files.getPosixFilePermissions(path));
});
}
/**
* Verify that a non-POSIX operation such as Files.setLastModifiedTime
* does not change the 'external file attributes' field.
*
* @throws IOException if an unexpected IOException occurs
*/
@Test
public void setLastModifiedTimeShouldNotChangeExternalFileAttribute() throws IOException {
assertExternalFileAttributeUnchanged(fs -> {
Path path = fs.getPath("hello.txt");
Files.setLastModifiedTime(path, FileTime.from(Instant.now()));
});
}
// Represents an operation performed on a FileSystem
static interface FileSystemOperation {
void accept(FileSystem fileSystem) throws IOException;
}
/**
* Assert that running the given operation on a ZipFileSystem does not
* change the 'external file attributes' value of the 'hello.txt' entry
* @param action the action to run on the file system
*
* @throws IOException if an unexpected IOException occurs
*/
private void assertExternalFileAttributeUnchanged(FileSystemOperation action) throws IOException {
/*
* The ZIP test vector used here is created using:
* % touch hello.txt
* % chmod u+s hello.txt # setuid
* % chmod g+s hello.txt # setgid
* % chmod +t hello.txt # sticky
* % zip hello.zip hello.txt
* % cat hello.zip | xxd -ps
*/
byte[] zip = HexFormat.of().parseHex("""
504b03040a0000000000d994945700000000000000000000000009001c00
68656c6c6f2e7478745554090003aa268365aa26836575780b000104f501
00000414000000504b01021e030a0000000000d994945700000000000000
0000000000090018000000000000000000a48f0000000068656c6c6f2e74
78745554050003aa26836575780b000104f50100000414000000504b0506
00000000010001004f000000430000000000
""".replaceAll("\n",""));
// Expected bit values of the 'external file attributes' CEN field in the ZIP above
String expectedBits = "1000111110100100";
// ^^^^ file type: 1000 (regular file)
// ^ setuid: ON
// ^ setgid: ON
// ^ sticky: ON
// ^^^^^^^^^ rwxr--r-- (9 bits)
// Sanity check that 'external file attributes' has the expected value
verifyExternalFileAttribute(zip, expectedBits);
Path zipFile = Path.of("preserve-external-file-attrs.zip");
Files.write(zipFile, zip);
// Run the provided action on the ZipFileSystem
try (FileSystem fs = FileSystems.newFileSystem(zipFile, ENV_POSIX)) {
action.accept(fs);
}
// Running the action should not change the 'external file attributes' value
verifyExternalFileAttribute(Files.readAllBytes(zipFile), expectedBits);
}
/**
* Verify that the first 16 bits of the CEN field 'external file attributes' matches
* a given bit string
* @param zip the ZIP file to parse
* @param expectedBits a string of '0' or '1' representing the expected bits
*/
private void verifyExternalFileAttribute(byte[] zip, String expectedBits) {
// Buffer to help parse the ZIP
ByteBuffer buffer = ByteBuffer.wrap(zip).order(ByteOrder.LITTLE_ENDIAN);
// Look up offset of first CEN header from the END header
int cenOff = buffer.getInt(buffer.capacity() - ZipFile.ENDHDR + ZipFile.ENDOFF);
// We're interested in the first 16 'unix' bits of the 32-bit 'external file attributes' field
int externalFileAttr = (buffer.getInt(cenOff + ZipFile.CENATX) >> 16) & 0xFFFF;
// Verify that the expected bits are set
assertEquals(expectedBits, Integer.toBinaryString(externalFileAttr),
"The 'external file attributes' field does not match the expected value:");
} }
} }