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

Reviewed-by: clanger, lancea

Author: Eirik Bjørsnøs

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

@@ -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.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -644,7 +644,12 @@ void setPermissions(byte[] path, Set<PosixFilePermission> perms) throws IOExcept
             if (e.type == Entry.CEN) {
                 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);
         } finally {
             endWrite();
@@ -3008,7 +3013,7 @@ private void readCEN(ZipFileSystem zipfs, IndexNode inode) throws IOException {
             attrsEx     = CENATX(cen, pos);
             */
             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);
             pos += CENHDR;

test/jdk/jdk/nio/zipfs/TestPosix.java

@@ -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.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,31 +25,24 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
 import java.nio.file.*;
-import java.nio.file.attribute.BasicFileAttributes;
-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.nio.file.attribute.*;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
-import java.util.Collections;
-import java.util.Enumeration;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
+import java.time.Instant;
+import java.util.*;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
 import java.util.spi.ToolProvider;
 import java.util.zip.ZipEntry;
 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_READ;
@@ -60,20 +53,20 @@
 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_WRITE;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertNull;
-import static org.testng.Assert.assertTrue;
-import static org.testng.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * @test
  * @bug 8213031 8273935
  * @summary Test POSIX ZIP file operations.
  * @modules jdk.zipfs
  *          jdk.jartool
- * @run testng TestPosix
- * @run testng/othervm/java.security.policy=test.policy.posix TestPosix
+ * @run junit TestPosix
+ * @run junit/othervm/java.security.policy=test.policy.posix TestPosix
  */
 public class TestPosix {
     private static final ToolProvider JAR_TOOL = ToolProvider.findFirst("jar")
@@ -364,7 +357,7 @@ private void checkEntry(Path file, checkExpects expected) {
                 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) {
             return;
@@ -402,7 +395,7 @@ private void doCheckEntries(Path path, checkExpects expected) throws IOException
             });
         }
         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 {
@@ -429,7 +422,7 @@ private void comparePermissions(Set<PosixFilePermission> expected, Set<PosixFile
             assertNull(actual, "Permissions are not null");
         } else {
             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).");
             for (PosixFilePermission p : expected) {
                 assertTrue(actual.contains(p), "Posix permission " + p + " missing.");
@@ -609,18 +602,18 @@ public void testPosixDefaults() throws IOException {
             var owner = Files.getOwner(entry);
             assertNotNull(owner, "owner should not be null");
             if (defaultOwner != null) {
-                assertEquals(owner.getName(), defaultOwner);
+                assertEquals(defaultOwner, owner.getName());
             }
             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 group = view.readAttributes().group();
             assertNotNull(group, "group must not be null");
             if (defaultGroup != null) {
-                assertEquals(group.getName(), defaultGroup);
+                assertEquals(defaultGroup, group.getName());
             }
             view.setGroup(DUMMY_GROUP);
-            assertEquals(view.readAttributes().group(), DUMMY_GROUP);
+            assertEquals(DUMMY_GROUP, view.readAttributes().group());
             entry = zipIn.getPath("/uexec");
             Files.setPosixFilePermissions(entry, GR); // will be persisted
             comparePermissions(GR, Files.getPosixFilePermissions(entry));
@@ -632,9 +625,9 @@ public void testPosixDefaults() throws IOException {
         {
             var entry = zipIn.getPath("/noperms");
             comparePermissions(UR, Files.getPosixFilePermissions(entry));
-            assertEquals(Files.getOwner(entry).getName(), "auser");
+            assertEquals("auser", Files.getOwner(entry).getName());
             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
             comparePermissions(GR, Files.getPosixFilePermissions(zipIn.getPath("/uexec")));
         }
@@ -645,9 +638,9 @@ public void testPosixDefaults() throws IOException {
         {
             var entry = zipIn.getPath("/noperms");
             comparePermissions(UR, Files.getPosixFilePermissions(entry));
-            assertEquals(Files.getOwner(entry), DUMMY_USER);
+            assertEquals(DUMMY_USER, Files.getOwner(entry));
             var view = Files.getFileAttributeView(entry, PosixFileAttributeView.class);
-            assertEquals(view.readAttributes().group(), DUMMY_GROUP);
+            assertEquals(DUMMY_GROUP, view.readAttributes().group());
         }
     }
 
@@ -722,6 +715,108 @@ public void testJarFile() throws IOException {
 
         // the run method catches IOExceptions, we need to expose them
         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:");
     }
 }