From 1308bbc633dc1f23811fec76df44e1c24335dd07 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 09:44:51 +0200 Subject: [PATCH 1/7] Improve `FileUtils.forceDelete()` tests on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, the `DeleteFile` Win32 API has a little quirk: it refuses to delete files with the legacy **DOS read-only attribute** set. (Because apparently 99% of Windows users don’t realize that “deleting a file” is actually an operation on the *directory*, not the file itself 😉). So the usual drill is: clear the read-only flag first, then delete. * Until JDK 25, `File.delete()` did this for you behind the scenes: it quietly stripped the flag before calling into `DeleteFile`. That meant your file might be left behind (with the flag missing) if the *real* ACLs didn’t allow deletion. * From JDK 25 onward, `File.delete()` doesn’t touch the flag anymore. If the bit is set, `DeleteFile` fails, end of story. * `FileUtils.forceDelete()` already knows how to juggle the flag itself, so its behavior didn’t change. This PR: * Updates two tests that were (unfairly) comparing `File.delete` with `FileUtils.forceDelete`. With JDK 25, their expectations diverged. * Adds a new test to confirm that `FileUtils.forceDelete` restores the read-only flag if the actual deletion fails. > [!WARNING] > I didn’t develop this on a Windows box, so I couldn’t test it locally. Leaving this as a **draft PR** until CI tells us whether Windows agrees with me. --- .../org/apache/commons/io/FileUtilsTest.java | 128 +++++++++++++++--- 1 file changed, 109 insertions(+), 19 deletions(-) diff --git a/src/test/java/org/apache/commons/io/FileUtilsTest.java b/src/test/java/org/apache/commons/io/FileUtilsTest.java index 4f6efa0f946..fabbe881625 100644 --- a/src/test/java/org/apache/commons/io/FileUtilsTest.java +++ b/src/test/java/org/apache/commons/io/FileUtilsTest.java @@ -46,12 +46,17 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.nio.charset.UnsupportedCharsetException; +import java.nio.file.AccessDeniedException; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.AclEntry; +import java.nio.file.attribute.AclEntryPermission; +import java.nio.file.attribute.AclEntryType; import java.nio.file.attribute.AclFileAttributeView; +import java.nio.file.attribute.DosFileAttributeView; import java.nio.file.attribute.FileTime; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; @@ -99,6 +104,7 @@ import org.junit.jupiter.api.condition.EnabledIf; import org.junit.jupiter.api.condition.EnabledOnOs; import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -1731,25 +1737,103 @@ void testForceDeleteReadOnlyDirectory() throws Exception { } } + private boolean isAtLeastJava25() { + try { + return Integer.parseInt(SystemUtils.JAVA_SPECIFICATION_VERSION) >= 25; + } catch (final NumberFormatException e) { + return false; + } + } + @Test void testForceDeleteReadOnlyFile() throws Exception { try (TempFile destination = TempFile.create("test-", ".txt")) { final File file = destination.toFile(); - assertTrue(file.setReadOnly()); - assertTrue(file.canRead()); - assertFalse(file.canWrite()); + assertTrue(file.setReadOnly(), "Setting file read-only successful"); + assertTrue(file.canRead(), "File must be readable"); + assertFalse(file.canWrite(), "File must not be writable"); + assertTrue(file.exists(), "File doesn't exist to delete"); // sanity check that File.delete() deletes read-only files. - assertTrue(file.delete()); + if (SystemUtils.IS_OS_WINDOWS && isAtLeastJava25()) { + // On Windows with Java 25+ File.delete() no longer deletes read-only files. + // See: https://bugs.openjdk.org/browse/JDK-8355954 + assertTrue(file.setWritable(true), "Setting file writable successful"); + } + assertTrue(file.delete(), "File.delete() must delete read-only file"); } try (TempFile destination = TempFile.create("test-", ".txt")) { final File file = destination.toFile(); // real test - assertTrue(file.setReadOnly()); - assertTrue(file.canRead()); - assertFalse(file.canWrite()); + assertTrue(file.setReadOnly(), "Setting file read-only successful"); + assertTrue(file.canRead(), "File must be readable"); + assertFalse(file.canWrite(), "File must not be writable"); assertTrue(file.exists(), "File doesn't exist to delete"); FileUtils.forceDelete(file); - assertFalse(file.exists(), "Check deletion"); + assertFalse(file.exists(), "FileUtils.forceDelete() must delete read-only file"); + } + } + + private static boolean supportsDos(Path p) { + return Files.getFileAttributeView(p, DosFileAttributeView.class) != null; + } + + private static boolean supportsAcl(Path p) { + return Files.getFileAttributeView(p, AclFileAttributeView.class) != null; + } + + private static boolean isDosReadOnly(Path p) throws IOException { + return (Boolean) Files.getAttribute(p, "dos:readonly", LinkOption.NOFOLLOW_LINKS); + } + + private static void prependDenyForOwner(AclFileAttributeView view, AclEntryPermission permission) throws IOException { + final AclEntry denyEntry = AclEntry.newBuilder().setType(AclEntryType.DENY).setPrincipal(view.getOwner()).setPermissions(permission).build(); + final List acl = new ArrayList<>(view.getAcl()); + // ACL is processed in order, so add to the start of the list + acl.add(0, denyEntry); + view.setAcl(acl); + } + + @Test + @EnabledOnOs(value = OS.WINDOWS) + void testForceDeleteRestoresReadOnlyOnFailure(@TempDir Path tempDir) throws Exception { + // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares). + assumeTrue(supportsDos(tempDir), "DOS attributes not supported"); + assumeTrue(supportsAcl(tempDir), "ACLs not supported"); + + final Path readOnly = tempDir.resolve("read-only-file.txt"); + Files.createFile(readOnly); + + // 1) Set the DOS readonly bit (what we want to ensure is restored on failure) + Files.setAttribute(readOnly, "dos:readonly", true); + assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly"); + + final AclFileAttributeView parentAclView = Files.getFileAttributeView(tempDir, AclFileAttributeView.class); + final List parentOriginalAcl = parentAclView.getAcl(); + final AclFileAttributeView aclView = Files.getFileAttributeView(readOnly, AclFileAttributeView.class); + final List originalAcl = aclView.getAcl(); + + try { + // 2) Cause deletion to fail due to permissions: + // deny DELETE on the file, and deny DELETE_CHILD on the parent directory. + prependDenyForOwner(aclView, AclEntryPermission.DELETE); + prependDenyForOwner(parentAclView, AclEntryPermission.DELETE_CHILD); + + // Sanity checks: + // canWrite() on Windows reflects only the DOS readonly bit (ignores the ACL for write). + final File file = readOnly.toFile(); + assertFalse(file.canWrite(), "Sanity: Windows canWrite() should be false when dos:readonly is set"); + + // 3) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. + assertThrows(AccessDeniedException.class, () -> FileUtils.forceDelete(file), "Deletion must fail because DELETE/DELETE_CHILD are denied"); + + // 4) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. + assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); + assertFalse(file.canWrite(), "File must still not be writable per DOS attribute"); + assertTrue(Files.exists(readOnly), "File should still exist after failed deletion"); + } finally { + // Cleanup: restore ACL to allow deletion + aclView.setAcl(originalAcl); + parentAclView.setAcl(parentOriginalAcl); } } @@ -1823,23 +1907,29 @@ void testForceDeleteUnwritableDirectory() throws Exception { void testForceDeleteUnwritableFile() throws Exception { try (TempFile destination = TempFile.create("test-", ".txt")) { final File file = destination.toFile(); - assertTrue(file.canWrite()); - assertTrue(file.setWritable(false)); - assertFalse(file.canWrite()); - assertTrue(file.canRead()); + assertTrue(file.canWrite(), "File must be writable"); + assertTrue(file.setWritable(false), "Setting file unwritable successful"); + assertFalse(file.canWrite(), "File must not be writable"); + assertTrue(file.canRead(), "File must be readable"); + assertTrue(file.exists(), "File must exist to delete"); // sanity check that File.delete() deletes unwritable files. - assertTrue(file.delete()); + if (SystemUtils.IS_OS_WINDOWS && isAtLeastJava25()) { + // On Windows with Java 25+ File.delete() no longer deletes read-only files. + // See: https://bugs.openjdk.org/browse/JDK-8355954 + assertTrue(file.setWritable(true), "Setting file writable successful"); + } + assertTrue(file.delete(), "File.delete() must delete unwritable file"); } try (TempFile destination = TempFile.create("test-", ".txt")) { final File file = destination.toFile(); // real test - assertTrue(file.canWrite()); - assertTrue(file.setWritable(false)); - assertFalse(file.canWrite()); - assertTrue(file.canRead()); - assertTrue(file.exists(), "File doesn't exist to delete"); + assertTrue(file.canWrite(), "File must be writable"); + assertTrue(file.setWritable(false), "Setting file unwritable successful"); + assertFalse(file.canWrite(), "File must not be writable"); + assertTrue(file.canRead(), "File must be readable"); + assertTrue(file.exists(), "File must exist to delete"); FileUtils.forceDelete(file); - assertFalse(file.exists(), "Check deletion"); + assertFalse(file.exists(), "FileUtils.forceDelete() must delete unwritable file"); } } From 733daefa27322b5a0554c51394753b733b430520 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 10:21:40 +0200 Subject: [PATCH 2/7] fix: always clear read-only bit --- .../org/apache/commons/io/FileUtilsTest.java | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/test/java/org/apache/commons/io/FileUtilsTest.java b/src/test/java/org/apache/commons/io/FileUtilsTest.java index fabbe881625..0b0faaf8dc8 100644 --- a/src/test/java/org/apache/commons/io/FileUtilsTest.java +++ b/src/test/java/org/apache/commons/io/FileUtilsTest.java @@ -1737,14 +1737,6 @@ void testForceDeleteReadOnlyDirectory() throws Exception { } } - private boolean isAtLeastJava25() { - try { - return Integer.parseInt(SystemUtils.JAVA_SPECIFICATION_VERSION) >= 25; - } catch (final NumberFormatException e) { - return false; - } - } - @Test void testForceDeleteReadOnlyFile() throws Exception { try (TempFile destination = TempFile.create("test-", ".txt")) { @@ -1753,11 +1745,11 @@ void testForceDeleteReadOnlyFile() throws Exception { assertTrue(file.canRead(), "File must be readable"); assertFalse(file.canWrite(), "File must not be writable"); assertTrue(file.exists(), "File doesn't exist to delete"); - // sanity check that File.delete() deletes read-only files. - if (SystemUtils.IS_OS_WINDOWS && isAtLeastJava25()) { - // On Windows with Java 25+ File.delete() no longer deletes read-only files. - // See: https://bugs.openjdk.org/browse/JDK-8355954 - assertTrue(file.setWritable(true), "Setting file writable successful"); + // Since JDK 25 on Windows, File.delete() refuses to remove files + // with the DOS readonly bit set (JDK-8355954). + // We clear the bit here for consistency across JDK versions. + if (supportsDos(file.toPath())) { + setDosReadOnly(file.toPath(), false); } assertTrue(file.delete(), "File.delete() must delete read-only file"); } @@ -1785,6 +1777,10 @@ private static boolean isDosReadOnly(Path p) throws IOException { return (Boolean) Files.getAttribute(p, "dos:readonly", LinkOption.NOFOLLOW_LINKS); } + private static void setDosReadOnly(Path p, boolean readOnly) throws IOException { + Files.setAttribute(p, "dos:readonly", readOnly, LinkOption.NOFOLLOW_LINKS); + } + private static void prependDenyForOwner(AclFileAttributeView view, AclEntryPermission permission) throws IOException { final AclEntry denyEntry = AclEntry.newBuilder().setType(AclEntryType.DENY).setPrincipal(view.getOwner()).setPermissions(permission).build(); final List acl = new ArrayList<>(view.getAcl()); @@ -1912,11 +1908,11 @@ void testForceDeleteUnwritableFile() throws Exception { assertFalse(file.canWrite(), "File must not be writable"); assertTrue(file.canRead(), "File must be readable"); assertTrue(file.exists(), "File must exist to delete"); - // sanity check that File.delete() deletes unwritable files. - if (SystemUtils.IS_OS_WINDOWS && isAtLeastJava25()) { - // On Windows with Java 25+ File.delete() no longer deletes read-only files. - // See: https://bugs.openjdk.org/browse/JDK-8355954 - assertTrue(file.setWritable(true), "Setting file writable successful"); + // Since JDK 25 on Windows, File.delete() refuses to remove files + // with the DOS readonly bit set (JDK-8355954). + // We clear the bit here for consistency across JDK versions. + if (supportsDos(file.toPath())) { + setDosReadOnly(file.toPath(), false); } assertTrue(file.delete(), "File.delete() must delete unwritable file"); } From 3dd65cb58925cf1d45d6b6c9a96299caa3687261 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 10:25:47 +0200 Subject: [PATCH 3/7] fix: check for `IOException`, not `AccessDeniedException` --- src/test/java/org/apache/commons/io/FileUtilsTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/apache/commons/io/FileUtilsTest.java b/src/test/java/org/apache/commons/io/FileUtilsTest.java index 0b0faaf8dc8..abf079f20cb 100644 --- a/src/test/java/org/apache/commons/io/FileUtilsTest.java +++ b/src/test/java/org/apache/commons/io/FileUtilsTest.java @@ -20,6 +20,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -1820,7 +1821,11 @@ void testForceDeleteRestoresReadOnlyOnFailure(@TempDir Path tempDir) throws Exce assertFalse(file.canWrite(), "Sanity: Windows canWrite() should be false when dos:readonly is set"); // 3) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. - assertThrows(AccessDeniedException.class, () -> FileUtils.forceDelete(file), "Deletion must fail because DELETE/DELETE_CHILD are denied"); + final IOException wrappedException = assertThrows(IOException.class, + () -> FileUtils.forceDelete(file), + "Deletion must fail because DELETE/DELETE_CHILD are denied"); + final Throwable cause = wrappedException.getCause(); + assertInstanceOf(AccessDeniedException.class, cause, "Cause must be AccessDeniedException"); // 4) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); From 4e821bfc822820681c3831734fe221c27d076f6b Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 13:22:37 +0200 Subject: [PATCH 4/7] feat: add holder for file and parent attributes Introduce a helper that snapshots file and parent directory attributes before `setReadOnly` is applied. If a deletion attempt fails, the holder can restore the original attributes to keep the filesystem state consistent. --- src/changes/changes.xml | 1 + .../org/apache/commons/io/file/PathUtils.java | 78 ++++++++- .../org/apache/commons/io/FileUtilsTest.java | 80 +-------- .../org/apache/commons/io/WindowsTest.java | 152 ++++++++++++++++++ 4 files changed, 231 insertions(+), 80 deletions(-) create mode 100644 src/test/java/org/apache/commons/io/WindowsTest.java diff --git a/src/changes/changes.xml b/src/changes/changes.xml index fb68216e20a..fe6e749e547 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -55,6 +55,7 @@ The type attribute can be add,update,fix,remove. Deprecate IOUtils.readFully(InputStream, int) in favor of toByteArray(InputStream, int). IOUtils.toByteArray(InputStream) now throws IOException on byte array overflow. Javadoc general improvements. + FileUtils.forceDelete and PathUtils.deleteFile restores the DOS read-only flag when deletion fails. FileUtils#byteCountToDisplaySize() supports Zettabyte, Yottabyte, Ronnabyte and Quettabyte #763. Add org.apache.commons.io.FileUtils.ONE_RB #763. diff --git a/src/main/java/org/apache/commons/io/file/PathUtils.java b/src/main/java/org/apache/commons/io/file/PathUtils.java index 055bbe06a0b..8217d0d916b 100644 --- a/src/main/java/org/apache/commons/io/file/PathUtils.java +++ b/src/main/java/org/apache/commons/io/file/PathUtils.java @@ -30,6 +30,7 @@ import java.nio.file.AccessDeniedException; import java.nio.file.CopyOption; import java.nio.file.DirectoryStream; +import java.nio.file.FileStore; import java.nio.file.FileSystem; import java.nio.file.FileVisitOption; import java.nio.file.FileVisitResult; @@ -198,6 +199,66 @@ private RelativeSortedPaths(final Path dir1, final Path dir2, final int maxDepth } } + /** + * Captures the relevant attributes of a file and its parent directory so they can be + * restored after a non-atomic operation. If an {@link IOException} occurs, call the + * restore methods to put both the file and the parent back to their previous state. + * + *

Only DOS and POSIX attributes are tracked (depending on what the underlying + * file store supports). If neither view is available, nothing is captured/restored.

+ */ + private static final class FileAndParentAttributesHolder { + private final Path file; + private final BasicFileAttributes fileAttributes; + private final BasicFileAttributes parentAttributes; + private final LinkOption[] linkOptions; + + FileAndParentAttributesHolder(final Path file, final LinkOption... linkOptions) throws IOException { + this.file = file; + this.fileAttributes = getAttributes(file, linkOptions); + this.parentAttributes = getAttributes(file.getParent(), linkOptions); + this.linkOptions = linkOptions; + } + + void restoreFileAttributes() throws IOException { + restoreAttributes(file, fileAttributes, linkOptions); + } + + void restoreParentAttributes() throws IOException { + restoreAttributes(file.getParent(), parentAttributes, linkOptions); + } + + private static BasicFileAttributes getAttributes(final Path path, final LinkOption... linkOptions) throws IOException { + if (path == null || Files.notExists(path, linkOptions)) { + return null; + } + final FileStore fileStore = Files.getFileStore(path); + // WindowsFileStore does not support PosixFileAttributeView, while LinuxFileStore might support DosFileAttributeView. + // Therefore, we check for DosFileAttributeView first. + if (fileStore.supportsFileAttributeView(DosFileAttributeView.class)) { + return Files.readAttributes(path, DosFileAttributes.class, linkOptions); + } + if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class)) { + return Files.readAttributes(path, PosixFileAttributes.class, linkOptions); + } + return null; + } + + private static void restoreAttributes(final Path path, final BasicFileAttributes attributes, final LinkOption... linkOptions) throws IOException { + if (path == null || Files.notExists(path, linkOptions) || attributes == null) { + return; + } + if (attributes instanceof DosFileAttributes) { + final DosFileAttributes dosAttributes = (DosFileAttributes) attributes; + setDosReadOnly(path, dosAttributes.isReadOnly(), linkOptions); + } + if (attributes instanceof PosixFileAttributes) { + final PosixFileAttributes posixAttributes = (PosixFileAttributes) attributes; + Files.setPosixFilePermissions(path, posixAttributes.permissions()); + } + } + } + private static final OpenOption[] OPEN_OPTIONS_TRUNCATE = { StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING }; private static final OpenOption[] OPEN_OPTIONS_APPEND = { StandardOpenOption.CREATE, StandardOpenOption.APPEND }; /** @@ -656,10 +717,10 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp // Ignore and try again below. } final Path parent = getParent(file); - PosixFileAttributes posixFileAttributes = null; + FileAndParentAttributesHolder attributesHolder = null; try { if (overrideReadOnly(deleteOptions)) { - posixFileAttributes = readPosixFileAttributes(parent, linkOptions); + attributesHolder = new FileAndParentAttributesHolder(file, linkOptions); setReadOnly(file, false, linkOptions); } // Read size _after_ having read/execute access on POSIX. @@ -669,9 +730,18 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp pathCounts.getFileCounter().increment(); pathCounts.getByteCounter().add(size); } + } catch (IOException e) { + if (attributesHolder != null) { + try { + attributesHolder.restoreFileAttributes(); + } catch (final IOException ex) { + e.addSuppressed(ex); + } + } + throw e; } finally { - if (posixFileAttributes != null) { - Files.setPosixFilePermissions(parent, posixFileAttributes.permissions()); + if (attributesHolder != null) { + attributesHolder.restoreParentAttributes(); } } return pathCounts; diff --git a/src/test/java/org/apache/commons/io/FileUtilsTest.java b/src/test/java/org/apache/commons/io/FileUtilsTest.java index abf079f20cb..1124622cbbc 100644 --- a/src/test/java/org/apache/commons/io/FileUtilsTest.java +++ b/src/test/java/org/apache/commons/io/FileUtilsTest.java @@ -20,7 +20,6 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -47,15 +46,11 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.nio.charset.UnsupportedCharsetException; -import java.nio.file.AccessDeniedException; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardCopyOption; -import java.nio.file.attribute.AclEntry; -import java.nio.file.attribute.AclEntryPermission; -import java.nio.file.attribute.AclEntryType; import java.nio.file.attribute.AclFileAttributeView; import java.nio.file.attribute.DosFileAttributeView; import java.nio.file.attribute.FileTime; @@ -105,7 +100,6 @@ import org.junit.jupiter.api.condition.EnabledIf; import org.junit.jupiter.api.condition.EnabledOnOs; import org.junit.jupiter.api.condition.OS; -import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -1749,9 +1743,7 @@ void testForceDeleteReadOnlyFile() throws Exception { // Since JDK 25 on Windows, File.delete() refuses to remove files // with the DOS readonly bit set (JDK-8355954). // We clear the bit here for consistency across JDK versions. - if (supportsDos(file.toPath())) { - setDosReadOnly(file.toPath(), false); - } + setDosReadOnly(file.toPath(), false); assertTrue(file.delete(), "File.delete() must delete read-only file"); } try (TempFile destination = TempFile.create("test-", ".txt")) { @@ -1770,71 +1762,9 @@ private static boolean supportsDos(Path p) { return Files.getFileAttributeView(p, DosFileAttributeView.class) != null; } - private static boolean supportsAcl(Path p) { - return Files.getFileAttributeView(p, AclFileAttributeView.class) != null; - } - - private static boolean isDosReadOnly(Path p) throws IOException { - return (Boolean) Files.getAttribute(p, "dos:readonly", LinkOption.NOFOLLOW_LINKS); - } - private static void setDosReadOnly(Path p, boolean readOnly) throws IOException { - Files.setAttribute(p, "dos:readonly", readOnly, LinkOption.NOFOLLOW_LINKS); - } - - private static void prependDenyForOwner(AclFileAttributeView view, AclEntryPermission permission) throws IOException { - final AclEntry denyEntry = AclEntry.newBuilder().setType(AclEntryType.DENY).setPrincipal(view.getOwner()).setPermissions(permission).build(); - final List acl = new ArrayList<>(view.getAcl()); - // ACL is processed in order, so add to the start of the list - acl.add(0, denyEntry); - view.setAcl(acl); - } - - @Test - @EnabledOnOs(value = OS.WINDOWS) - void testForceDeleteRestoresReadOnlyOnFailure(@TempDir Path tempDir) throws Exception { - // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares). - assumeTrue(supportsDos(tempDir), "DOS attributes not supported"); - assumeTrue(supportsAcl(tempDir), "ACLs not supported"); - - final Path readOnly = tempDir.resolve("read-only-file.txt"); - Files.createFile(readOnly); - - // 1) Set the DOS readonly bit (what we want to ensure is restored on failure) - Files.setAttribute(readOnly, "dos:readonly", true); - assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly"); - - final AclFileAttributeView parentAclView = Files.getFileAttributeView(tempDir, AclFileAttributeView.class); - final List parentOriginalAcl = parentAclView.getAcl(); - final AclFileAttributeView aclView = Files.getFileAttributeView(readOnly, AclFileAttributeView.class); - final List originalAcl = aclView.getAcl(); - - try { - // 2) Cause deletion to fail due to permissions: - // deny DELETE on the file, and deny DELETE_CHILD on the parent directory. - prependDenyForOwner(aclView, AclEntryPermission.DELETE); - prependDenyForOwner(parentAclView, AclEntryPermission.DELETE_CHILD); - - // Sanity checks: - // canWrite() on Windows reflects only the DOS readonly bit (ignores the ACL for write). - final File file = readOnly.toFile(); - assertFalse(file.canWrite(), "Sanity: Windows canWrite() should be false when dos:readonly is set"); - - // 3) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. - final IOException wrappedException = assertThrows(IOException.class, - () -> FileUtils.forceDelete(file), - "Deletion must fail because DELETE/DELETE_CHILD are denied"); - final Throwable cause = wrappedException.getCause(); - assertInstanceOf(AccessDeniedException.class, cause, "Cause must be AccessDeniedException"); - - // 4) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. - assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); - assertFalse(file.canWrite(), "File must still not be writable per DOS attribute"); - assertTrue(Files.exists(readOnly), "File should still exist after failed deletion"); - } finally { - // Cleanup: restore ACL to allow deletion - aclView.setAcl(originalAcl); - parentAclView.setAcl(parentOriginalAcl); + if (Files.getFileStore(p).supportsFileAttributeView(DosFileAttributeView.class)) { + Files.setAttribute(p, "dos:readonly", readOnly, LinkOption.NOFOLLOW_LINKS); } } @@ -1916,9 +1846,7 @@ void testForceDeleteUnwritableFile() throws Exception { // Since JDK 25 on Windows, File.delete() refuses to remove files // with the DOS readonly bit set (JDK-8355954). // We clear the bit here for consistency across JDK versions. - if (supportsDos(file.toPath())) { - setDosReadOnly(file.toPath(), false); - } + setDosReadOnly(file.toPath(), false); assertTrue(file.delete(), "File.delete() must delete unwritable file"); } try (TempFile destination = TempFile.create("test-", ".txt")) { diff --git a/src/test/java/org/apache/commons/io/WindowsTest.java b/src/test/java/org/apache/commons/io/WindowsTest.java new file mode 100644 index 00000000000..17b9c6673ac --- /dev/null +++ b/src/test/java/org/apache/commons/io/WindowsTest.java @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.io; + +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import static org.junit.jupiter.api.condition.OS.WINDOWS; + +import java.io.Closeable; +import java.io.IOException; +import java.nio.file.AccessDeniedException; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.attribute.AclEntry; +import java.nio.file.attribute.AclEntryPermission; +import java.nio.file.attribute.AclEntryType; +import java.nio.file.attribute.AclFileAttributeView; +import java.nio.file.attribute.DosFileAttributeView; +import java.util.ArrayList; +import java.util.List; + +import org.apache.commons.io.file.PathUtils; +import org.apache.commons.io.file.StandardDeleteOption; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.io.TempDir; + +/** + * Windows-specific test cases for {@code FileUtils} and {@code PathUtils}. + * + *

Keeping these tests in a separate class makes it clear when a test + * was skipped due to the operating system, rather than failing for other + * reasons.

+ */ +@EnabledOnOs(WINDOWS) +class WindowsTest { + + private static class AclAttributesHolder implements Closeable { + private final Path path; + private final List acl; + private final Path parent; + private final List parentAcl; + + AclAttributesHolder(Path path) throws IOException { + this.path = path; + this.acl = Files.getFileAttributeView(path, AclFileAttributeView.class).getAcl(); + this.parent = path.getParent(); + this.parentAcl = parent == null ? null : Files.getFileAttributeView(parent, AclFileAttributeView.class).getAcl(); + } + + @Override + public void close() throws IOException { + Files.getFileAttributeView(path, AclFileAttributeView.class).setAcl(acl); + if (parent != null) { + Files.getFileAttributeView(parent, AclFileAttributeView.class).setAcl(parentAcl); + } + } + } + + private static boolean supportsDosAndAcl(Path p) { + return Files.getFileAttributeView(p, DosFileAttributeView.class) != null && Files.getFileAttributeView(p, AclFileAttributeView.class) != null; + } + + private static boolean isDosReadOnly(Path p) throws IOException { + return (Boolean) Files.getAttribute(p, "dos:readonly", LinkOption.NOFOLLOW_LINKS); + } + + private static void denyDeleteForOwner(Path path) throws IOException { + prependDenyForOwner(Files.getFileAttributeView(path, AclFileAttributeView.class), AclEntryPermission.DELETE); + final Path parent = path.getParent(); + if (parent != null) { + prependDenyForOwner(Files.getFileAttributeView(parent, AclFileAttributeView.class), AclEntryPermission.DELETE_CHILD); + } + } + + private static void prependDenyForOwner(AclFileAttributeView view, AclEntryPermission permission) throws IOException { + final AclEntry denyEntry = AclEntry.newBuilder().setType(AclEntryType.DENY).setPrincipal(view.getOwner()).setPermissions(permission).build(); + final List acl = new ArrayList<>(view.getAcl()); + // ACL is processed in order, so add to the start of the list + acl.add(0, denyEntry); + view.setAcl(acl); + } + + @Test + void testFileUtilsForceDeleteRestoresAttributesOnFailure(@TempDir Path tempDir) throws Exception { + // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares). + assumeTrue(supportsDosAndAcl(tempDir), "ACL and DOS attributes not supported"); + + final Path readOnly = tempDir.resolve("read-only-file.txt"); + Files.createFile(readOnly); + + // 1) Set the DOS readonly bit (what we want to ensure is restored on failure) + Files.setAttribute(readOnly, "dos:readonly", true); + assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly"); + + try (AclAttributesHolder ignored = new AclAttributesHolder(readOnly)) { + denyDeleteForOwner(readOnly); + + // 2) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. + final IOException wrappedException = assertThrows(IOException.class, () -> FileUtils.forceDelete(readOnly.toFile()), + "Deletion must fail because DELETE/DELETE_CHILD are denied"); + final Throwable cause = wrappedException.getCause(); + assertInstanceOf(AccessDeniedException.class, cause, "Cause must be AccessDeniedException"); + + // 3) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. + assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); + } + } + + @Test + void testPathUtilsDeleteFileRestoresAttributesOnFailure(@TempDir Path tempDir) throws Exception { + // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares). + assumeTrue(supportsDosAndAcl(tempDir), "ACL and DOS attributes not supported"); + + final Path readOnly = tempDir.resolve("read-only-file.txt"); + Files.createFile(readOnly); + + // 1) Set the DOS readonly bit (what we want to ensure is restored on failure) + Files.setAttribute(readOnly, "dos:readonly", true); + assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly"); + + try (AclAttributesHolder ignored = new AclAttributesHolder(readOnly)) { + denyDeleteForOwner(readOnly); + + // 2) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. + final IOException wrappedException = assertThrows(IOException.class, () -> PathUtils.deleteFile(readOnly, + StandardDeleteOption.OVERRIDE_READ_ONLY), "Deletion must fail because DELETE/DELETE_CHILD are denied"); + final Throwable cause = wrappedException.getCause(); + assertInstanceOf(AccessDeniedException.class, cause, "Cause must be AccessDeniedException"); + + // 3) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. + assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); + } + } +} From 8b486d47d8da83e5caec6716e1cde7e5030ed7f6 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 13:33:57 +0200 Subject: [PATCH 5/7] fix: Spotbugs failure --- src/main/java/org/apache/commons/io/file/PathUtils.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/apache/commons/io/file/PathUtils.java b/src/main/java/org/apache/commons/io/file/PathUtils.java index 8217d0d916b..cd13528e1ed 100644 --- a/src/main/java/org/apache/commons/io/file/PathUtils.java +++ b/src/main/java/org/apache/commons/io/file/PathUtils.java @@ -716,7 +716,6 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp } catch (final AccessDeniedException ignored) { // Ignore and try again below. } - final Path parent = getParent(file); FileAndParentAttributesHolder attributesHolder = null; try { if (overrideReadOnly(deleteOptions)) { From f6683b81b427bf4a0b6772db99cf5f09a7891772 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 14:23:24 +0200 Subject: [PATCH 6/7] fix: prefer POSIX to DOS attributes when both are available On Linux/Unix, both POSIX and DOS attribute views may be supported, while on Windows only DOS attributes are available. Check for POSIX first to ensure the correct view is used across platforms. --- .../org/apache/commons/io/file/PathUtils.java | 81 +++++++++---------- .../commons/io/DeleteDirectoryTest.java | 1 - 2 files changed, 40 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/apache/commons/io/file/PathUtils.java b/src/main/java/org/apache/commons/io/file/PathUtils.java index cd13528e1ed..428f68b6b4d 100644 --- a/src/main/java/org/apache/commons/io/file/PathUtils.java +++ b/src/main/java/org/apache/commons/io/file/PathUtils.java @@ -229,32 +229,34 @@ void restoreParentAttributes() throws IOException { } private static BasicFileAttributes getAttributes(final Path path, final LinkOption... linkOptions) throws IOException { - if (path == null || Files.notExists(path, linkOptions)) { + // Attention: Files.notExists() is not the inverse of Files.exists()! + if (path == null || !Files.exists(path, linkOptions)) { return null; } final FileStore fileStore = Files.getFileStore(path); - // WindowsFileStore does not support PosixFileAttributeView, while LinuxFileStore might support DosFileAttributeView. - // Therefore, we check for DosFileAttributeView first. - if (fileStore.supportsFileAttributeView(DosFileAttributeView.class)) { - return Files.readAttributes(path, DosFileAttributes.class, linkOptions); - } + // On Windows, only DosFileAttributeView is supported. + // On Unix-like systems, PosixFileAttributeView is standard, but + // DosFileAttributeView may also be available (newer JDKs / some FS). + // Prefer Posix if present, otherwise fall back to Dos. if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class)) { return Files.readAttributes(path, PosixFileAttributes.class, linkOptions); + } else if (fileStore.supportsFileAttributeView(DosFileAttributeView.class)) { + return Files.readAttributes(path, DosFileAttributes.class, linkOptions); } return null; } private static void restoreAttributes(final Path path, final BasicFileAttributes attributes, final LinkOption... linkOptions) throws IOException { - if (path == null || Files.notExists(path, linkOptions) || attributes == null) { + // Attention: Files.notExists() is not the inverse of Files.exists()! + if (path == null || !Files.exists(path, linkOptions) || attributes == null) { return; } - if (attributes instanceof DosFileAttributes) { - final DosFileAttributes dosAttributes = (DosFileAttributes) attributes; - setDosReadOnly(path, dosAttributes.isReadOnly(), linkOptions); - } if (attributes instanceof PosixFileAttributes) { final PosixFileAttributes posixAttributes = (PosixFileAttributes) attributes; Files.setPosixFilePermissions(path, posixAttributes.permissions()); + } else if (attributes instanceof DosFileAttributes) { + final DosFileAttributes dosAttributes = (DosFileAttributes) attributes; + setDosReadOnly(path, dosAttributes.isReadOnly(), linkOptions); } } } @@ -1651,13 +1653,8 @@ static Path resolve(final Path targetDirectory, final Path otherPath) { return targetDirectory.resolve(Objects.equals(separatorSource, separatorTarget) ? otherString : otherString.replace(separatorSource, separatorTarget)); } - private static boolean setDosReadOnly(final Path path, final boolean readOnly, final LinkOption... linkOptions) throws IOException { - final DosFileAttributeView dosFileAttributeView = getDosFileAttributeView(path, linkOptions); - if (dosFileAttributeView != null) { - dosFileAttributeView.setReadOnly(readOnly); - return true; - } - return false; + private static void setDosReadOnly(final Path path, final boolean readOnly, final LinkOption... linkOptions) throws IOException { + Files.setAttribute(path, "dos:readonly", readOnly, linkOptions); } /** @@ -1763,7 +1760,7 @@ private static void setPosixReadOnlyFile(final Path path, final boolean readOnly * This behavior is OS dependent. *

* - * @param path The path to set. + * @param path The path to set, not {@code null}. * @param readOnly true for read-only, false for not read-only. * @param linkOptions options indicating how to handle symbolic links. * @return The given path. @@ -1771,30 +1768,32 @@ private static void setPosixReadOnlyFile(final Path path, final boolean readOnly * @since 2.8.0 */ public static Path setReadOnly(final Path path, final boolean readOnly, final LinkOption... linkOptions) throws IOException { - try { - // Windows is simplest - if (setDosReadOnly(path, readOnly, linkOptions)) { - return path; - } - } catch (final IOException ignored) { - // Retry with POSIX below. - } + // On Windows, only DosFileAttributeView is supported. + // On Unix-like systems, PosixFileAttributeView is standard, but + // DosFileAttributeView may also be available (newer JDKs / some FS). + // Prefer Posix if present, otherwise fall back to Dos. final Path parent = getParent(path); - if (!isPosix(parent, linkOptions)) { // Test parent because we may not the permissions to test the file. - throw new IOException(String.format("DOS or POSIX file operations not available for '%s', linkOptions %s", path, Arrays.toString(linkOptions))); - } - // POSIX - if (readOnly) { - // RO - // File, then parent dir (if any). - setPosixReadOnlyFile(path, readOnly, linkOptions); - setPosixDeletePermissions(parent, false, linkOptions); - } else { - // RE - // Parent dir (if any), then file. - setPosixDeletePermissions(parent, true, linkOptions); + final FileStore fileStore = Files.getFileStore(parent != null ? parent : path); + // Test parent because we may not the permissions to test the file. + if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class)) { + // POSIX + if (readOnly) { + // RO + // File, then parent dir (if any). + setPosixReadOnlyFile(path, readOnly, linkOptions); + setPosixDeletePermissions(parent, false, linkOptions); + } else { + // RE + // Parent dir (if any), then file. + setPosixDeletePermissions(parent, true, linkOptions); + } + return path; + } else if (fileStore.supportsFileAttributeView(DosFileAttributeView.class)) { + // Windows + setDosReadOnly(path, readOnly, linkOptions); + return path; } - return path; + throw new IOException(String.format("DOS or POSIX file operations not available for '%s', linkOptions %s", path, Arrays.toString(linkOptions))); } /** diff --git a/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java b/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java index eee6836d5ad..187b3c567d8 100644 --- a/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java +++ b/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java @@ -92,7 +92,6 @@ void testDeleteDirectoryWithPathUtilsOverrideReadOnly() throws IOException { } @Test - @DisabledOnOs(OS.LINUX) // TODO void testDeleteFileCheckParentAccess() throws IOException { // Create a test directory final Path testDir = tempDirPath.resolve("dir"); From a00010069ed66176b3a40eeda0c6c0199a275a99 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 2 Oct 2025 14:53:25 +0200 Subject: [PATCH 7/7] fix: revert read-only related changes --- src/changes/changes.xml | 1 - .../org/apache/commons/io/file/PathUtils.java | 138 ++++------------ .../commons/io/DeleteDirectoryTest.java | 1 + .../org/apache/commons/io/FileUtilsTest.java | 4 - .../org/apache/commons/io/WindowsTest.java | 152 ------------------ 5 files changed, 36 insertions(+), 260 deletions(-) delete mode 100644 src/test/java/org/apache/commons/io/WindowsTest.java diff --git a/src/changes/changes.xml b/src/changes/changes.xml index fe6e749e547..fb68216e20a 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -55,7 +55,6 @@ The type attribute can be add,update,fix,remove. Deprecate IOUtils.readFully(InputStream, int) in favor of toByteArray(InputStream, int). IOUtils.toByteArray(InputStream) now throws IOException on byte array overflow. Javadoc general improvements. - FileUtils.forceDelete and PathUtils.deleteFile restores the DOS read-only flag when deletion fails. FileUtils#byteCountToDisplaySize() supports Zettabyte, Yottabyte, Ronnabyte and Quettabyte #763. Add org.apache.commons.io.FileUtils.ONE_RB #763. diff --git a/src/main/java/org/apache/commons/io/file/PathUtils.java b/src/main/java/org/apache/commons/io/file/PathUtils.java index 428f68b6b4d..055bbe06a0b 100644 --- a/src/main/java/org/apache/commons/io/file/PathUtils.java +++ b/src/main/java/org/apache/commons/io/file/PathUtils.java @@ -30,7 +30,6 @@ import java.nio.file.AccessDeniedException; import java.nio.file.CopyOption; import java.nio.file.DirectoryStream; -import java.nio.file.FileStore; import java.nio.file.FileSystem; import java.nio.file.FileVisitOption; import java.nio.file.FileVisitResult; @@ -199,68 +198,6 @@ private RelativeSortedPaths(final Path dir1, final Path dir2, final int maxDepth } } - /** - * Captures the relevant attributes of a file and its parent directory so they can be - * restored after a non-atomic operation. If an {@link IOException} occurs, call the - * restore methods to put both the file and the parent back to their previous state. - * - *

Only DOS and POSIX attributes are tracked (depending on what the underlying - * file store supports). If neither view is available, nothing is captured/restored.

- */ - private static final class FileAndParentAttributesHolder { - private final Path file; - private final BasicFileAttributes fileAttributes; - private final BasicFileAttributes parentAttributes; - private final LinkOption[] linkOptions; - - FileAndParentAttributesHolder(final Path file, final LinkOption... linkOptions) throws IOException { - this.file = file; - this.fileAttributes = getAttributes(file, linkOptions); - this.parentAttributes = getAttributes(file.getParent(), linkOptions); - this.linkOptions = linkOptions; - } - - void restoreFileAttributes() throws IOException { - restoreAttributes(file, fileAttributes, linkOptions); - } - - void restoreParentAttributes() throws IOException { - restoreAttributes(file.getParent(), parentAttributes, linkOptions); - } - - private static BasicFileAttributes getAttributes(final Path path, final LinkOption... linkOptions) throws IOException { - // Attention: Files.notExists() is not the inverse of Files.exists()! - if (path == null || !Files.exists(path, linkOptions)) { - return null; - } - final FileStore fileStore = Files.getFileStore(path); - // On Windows, only DosFileAttributeView is supported. - // On Unix-like systems, PosixFileAttributeView is standard, but - // DosFileAttributeView may also be available (newer JDKs / some FS). - // Prefer Posix if present, otherwise fall back to Dos. - if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class)) { - return Files.readAttributes(path, PosixFileAttributes.class, linkOptions); - } else if (fileStore.supportsFileAttributeView(DosFileAttributeView.class)) { - return Files.readAttributes(path, DosFileAttributes.class, linkOptions); - } - return null; - } - - private static void restoreAttributes(final Path path, final BasicFileAttributes attributes, final LinkOption... linkOptions) throws IOException { - // Attention: Files.notExists() is not the inverse of Files.exists()! - if (path == null || !Files.exists(path, linkOptions) || attributes == null) { - return; - } - if (attributes instanceof PosixFileAttributes) { - final PosixFileAttributes posixAttributes = (PosixFileAttributes) attributes; - Files.setPosixFilePermissions(path, posixAttributes.permissions()); - } else if (attributes instanceof DosFileAttributes) { - final DosFileAttributes dosAttributes = (DosFileAttributes) attributes; - setDosReadOnly(path, dosAttributes.isReadOnly(), linkOptions); - } - } - } - private static final OpenOption[] OPEN_OPTIONS_TRUNCATE = { StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING }; private static final OpenOption[] OPEN_OPTIONS_APPEND = { StandardOpenOption.CREATE, StandardOpenOption.APPEND }; /** @@ -718,10 +655,11 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp } catch (final AccessDeniedException ignored) { // Ignore and try again below. } - FileAndParentAttributesHolder attributesHolder = null; + final Path parent = getParent(file); + PosixFileAttributes posixFileAttributes = null; try { if (overrideReadOnly(deleteOptions)) { - attributesHolder = new FileAndParentAttributesHolder(file, linkOptions); + posixFileAttributes = readPosixFileAttributes(parent, linkOptions); setReadOnly(file, false, linkOptions); } // Read size _after_ having read/execute access on POSIX. @@ -731,18 +669,9 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp pathCounts.getFileCounter().increment(); pathCounts.getByteCounter().add(size); } - } catch (IOException e) { - if (attributesHolder != null) { - try { - attributesHolder.restoreFileAttributes(); - } catch (final IOException ex) { - e.addSuppressed(ex); - } - } - throw e; } finally { - if (attributesHolder != null) { - attributesHolder.restoreParentAttributes(); + if (posixFileAttributes != null) { + Files.setPosixFilePermissions(parent, posixFileAttributes.permissions()); } } return pathCounts; @@ -1653,8 +1582,13 @@ static Path resolve(final Path targetDirectory, final Path otherPath) { return targetDirectory.resolve(Objects.equals(separatorSource, separatorTarget) ? otherString : otherString.replace(separatorSource, separatorTarget)); } - private static void setDosReadOnly(final Path path, final boolean readOnly, final LinkOption... linkOptions) throws IOException { - Files.setAttribute(path, "dos:readonly", readOnly, linkOptions); + private static boolean setDosReadOnly(final Path path, final boolean readOnly, final LinkOption... linkOptions) throws IOException { + final DosFileAttributeView dosFileAttributeView = getDosFileAttributeView(path, linkOptions); + if (dosFileAttributeView != null) { + dosFileAttributeView.setReadOnly(readOnly); + return true; + } + return false; } /** @@ -1760,7 +1694,7 @@ private static void setPosixReadOnlyFile(final Path path, final boolean readOnly * This behavior is OS dependent. *

* - * @param path The path to set, not {@code null}. + * @param path The path to set. * @param readOnly true for read-only, false for not read-only. * @param linkOptions options indicating how to handle symbolic links. * @return The given path. @@ -1768,32 +1702,30 @@ private static void setPosixReadOnlyFile(final Path path, final boolean readOnly * @since 2.8.0 */ public static Path setReadOnly(final Path path, final boolean readOnly, final LinkOption... linkOptions) throws IOException { - // On Windows, only DosFileAttributeView is supported. - // On Unix-like systems, PosixFileAttributeView is standard, but - // DosFileAttributeView may also be available (newer JDKs / some FS). - // Prefer Posix if present, otherwise fall back to Dos. - final Path parent = getParent(path); - final FileStore fileStore = Files.getFileStore(parent != null ? parent : path); - // Test parent because we may not the permissions to test the file. - if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class)) { - // POSIX - if (readOnly) { - // RO - // File, then parent dir (if any). - setPosixReadOnlyFile(path, readOnly, linkOptions); - setPosixDeletePermissions(parent, false, linkOptions); - } else { - // RE - // Parent dir (if any), then file. - setPosixDeletePermissions(parent, true, linkOptions); + try { + // Windows is simplest + if (setDosReadOnly(path, readOnly, linkOptions)) { + return path; } - return path; - } else if (fileStore.supportsFileAttributeView(DosFileAttributeView.class)) { - // Windows - setDosReadOnly(path, readOnly, linkOptions); - return path; + } catch (final IOException ignored) { + // Retry with POSIX below. + } + final Path parent = getParent(path); + if (!isPosix(parent, linkOptions)) { // Test parent because we may not the permissions to test the file. + throw new IOException(String.format("DOS or POSIX file operations not available for '%s', linkOptions %s", path, Arrays.toString(linkOptions))); } - throw new IOException(String.format("DOS or POSIX file operations not available for '%s', linkOptions %s", path, Arrays.toString(linkOptions))); + // POSIX + if (readOnly) { + // RO + // File, then parent dir (if any). + setPosixReadOnlyFile(path, readOnly, linkOptions); + setPosixDeletePermissions(parent, false, linkOptions); + } else { + // RE + // Parent dir (if any), then file. + setPosixDeletePermissions(parent, true, linkOptions); + } + return path; } /** diff --git a/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java b/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java index 187b3c567d8..eee6836d5ad 100644 --- a/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java +++ b/src/test/java/org/apache/commons/io/DeleteDirectoryTest.java @@ -92,6 +92,7 @@ void testDeleteDirectoryWithPathUtilsOverrideReadOnly() throws IOException { } @Test + @DisabledOnOs(OS.LINUX) // TODO void testDeleteFileCheckParentAccess() throws IOException { // Create a test directory final Path testDir = tempDirPath.resolve("dir"); diff --git a/src/test/java/org/apache/commons/io/FileUtilsTest.java b/src/test/java/org/apache/commons/io/FileUtilsTest.java index 1124622cbbc..ced4b106638 100644 --- a/src/test/java/org/apache/commons/io/FileUtilsTest.java +++ b/src/test/java/org/apache/commons/io/FileUtilsTest.java @@ -1758,10 +1758,6 @@ void testForceDeleteReadOnlyFile() throws Exception { } } - private static boolean supportsDos(Path p) { - return Files.getFileAttributeView(p, DosFileAttributeView.class) != null; - } - private static void setDosReadOnly(Path p, boolean readOnly) throws IOException { if (Files.getFileStore(p).supportsFileAttributeView(DosFileAttributeView.class)) { Files.setAttribute(p, "dos:readonly", readOnly, LinkOption.NOFOLLOW_LINKS); diff --git a/src/test/java/org/apache/commons/io/WindowsTest.java b/src/test/java/org/apache/commons/io/WindowsTest.java deleted file mode 100644 index 17b9c6673ac..00000000000 --- a/src/test/java/org/apache/commons/io/WindowsTest.java +++ /dev/null @@ -1,152 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.commons.io; - -import static org.junit.jupiter.api.Assertions.assertInstanceOf; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assumptions.assumeTrue; -import static org.junit.jupiter.api.condition.OS.WINDOWS; - -import java.io.Closeable; -import java.io.IOException; -import java.nio.file.AccessDeniedException; -import java.nio.file.Files; -import java.nio.file.LinkOption; -import java.nio.file.Path; -import java.nio.file.attribute.AclEntry; -import java.nio.file.attribute.AclEntryPermission; -import java.nio.file.attribute.AclEntryType; -import java.nio.file.attribute.AclFileAttributeView; -import java.nio.file.attribute.DosFileAttributeView; -import java.util.ArrayList; -import java.util.List; - -import org.apache.commons.io.file.PathUtils; -import org.apache.commons.io.file.StandardDeleteOption; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.condition.EnabledOnOs; -import org.junit.jupiter.api.io.TempDir; - -/** - * Windows-specific test cases for {@code FileUtils} and {@code PathUtils}. - * - *

Keeping these tests in a separate class makes it clear when a test - * was skipped due to the operating system, rather than failing for other - * reasons.

- */ -@EnabledOnOs(WINDOWS) -class WindowsTest { - - private static class AclAttributesHolder implements Closeable { - private final Path path; - private final List acl; - private final Path parent; - private final List parentAcl; - - AclAttributesHolder(Path path) throws IOException { - this.path = path; - this.acl = Files.getFileAttributeView(path, AclFileAttributeView.class).getAcl(); - this.parent = path.getParent(); - this.parentAcl = parent == null ? null : Files.getFileAttributeView(parent, AclFileAttributeView.class).getAcl(); - } - - @Override - public void close() throws IOException { - Files.getFileAttributeView(path, AclFileAttributeView.class).setAcl(acl); - if (parent != null) { - Files.getFileAttributeView(parent, AclFileAttributeView.class).setAcl(parentAcl); - } - } - } - - private static boolean supportsDosAndAcl(Path p) { - return Files.getFileAttributeView(p, DosFileAttributeView.class) != null && Files.getFileAttributeView(p, AclFileAttributeView.class) != null; - } - - private static boolean isDosReadOnly(Path p) throws IOException { - return (Boolean) Files.getAttribute(p, "dos:readonly", LinkOption.NOFOLLOW_LINKS); - } - - private static void denyDeleteForOwner(Path path) throws IOException { - prependDenyForOwner(Files.getFileAttributeView(path, AclFileAttributeView.class), AclEntryPermission.DELETE); - final Path parent = path.getParent(); - if (parent != null) { - prependDenyForOwner(Files.getFileAttributeView(parent, AclFileAttributeView.class), AclEntryPermission.DELETE_CHILD); - } - } - - private static void prependDenyForOwner(AclFileAttributeView view, AclEntryPermission permission) throws IOException { - final AclEntry denyEntry = AclEntry.newBuilder().setType(AclEntryType.DENY).setPrincipal(view.getOwner()).setPermissions(permission).build(); - final List acl = new ArrayList<>(view.getAcl()); - // ACL is processed in order, so add to the start of the list - acl.add(0, denyEntry); - view.setAcl(acl); - } - - @Test - void testFileUtilsForceDeleteRestoresAttributesOnFailure(@TempDir Path tempDir) throws Exception { - // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares). - assumeTrue(supportsDosAndAcl(tempDir), "ACL and DOS attributes not supported"); - - final Path readOnly = tempDir.resolve("read-only-file.txt"); - Files.createFile(readOnly); - - // 1) Set the DOS readonly bit (what we want to ensure is restored on failure) - Files.setAttribute(readOnly, "dos:readonly", true); - assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly"); - - try (AclAttributesHolder ignored = new AclAttributesHolder(readOnly)) { - denyDeleteForOwner(readOnly); - - // 2) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. - final IOException wrappedException = assertThrows(IOException.class, () -> FileUtils.forceDelete(readOnly.toFile()), - "Deletion must fail because DELETE/DELETE_CHILD are denied"); - final Throwable cause = wrappedException.getCause(); - assertInstanceOf(AccessDeniedException.class, cause, "Cause must be AccessDeniedException"); - - // 3) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. - assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); - } - } - - @Test - void testPathUtilsDeleteFileRestoresAttributesOnFailure(@TempDir Path tempDir) throws Exception { - // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares). - assumeTrue(supportsDosAndAcl(tempDir), "ACL and DOS attributes not supported"); - - final Path readOnly = tempDir.resolve("read-only-file.txt"); - Files.createFile(readOnly); - - // 1) Set the DOS readonly bit (what we want to ensure is restored on failure) - Files.setAttribute(readOnly, "dos:readonly", true); - assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly"); - - try (AclAttributesHolder ignored = new AclAttributesHolder(readOnly)) { - denyDeleteForOwner(readOnly); - - // 2) Attempt forced deletion; should fail with AccessDeniedException due to ACLs. - final IOException wrappedException = assertThrows(IOException.class, () -> PathUtils.deleteFile(readOnly, - StandardDeleteOption.OVERRIDE_READ_ONLY), "Deletion must fail because DELETE/DELETE_CHILD are denied"); - final Throwable cause = wrappedException.getCause(); - assertInstanceOf(AccessDeniedException.class, cause, "Cause must be AccessDeniedException"); - - // 3) Critical assertion: even though deletion failed, the DOS readonly flag must be restored. - assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion"); - } - } -}