Skip to content

[IO-879] Restore read-only state after failed deletion #881

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.

Sign up for GitHub

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

Open
efegokdemir wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[IO-879] Restore read-only state after failed deletion #881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aad2aaa
IO-879: Restore read-only state after failed deletion
efegokdemir Sep 23, 2026
f301e0b
[IO-879] Restore file state after failed deletion
efegokdemir Oct 25, 2026
b4c1a56
[IO-879] Restore permissions after delete attempts
efegokdemir Oct 28, 2026
01b0416
Merge remote-tracking branch 'refs/remotes/apache/master' into HEAD
efegokdemir Oct 28, 2026
642cb5f
[IO-879] Preserve deletion failures during permission restoration
efegokdemir Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 56 additions & 4 deletions src/main/java/org/apache/commons/io/file/CleaningPathVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.nio.file.LinkOption;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.attribute.PosixFileAttributes;
import java.util.Arrays;
import java.util.Objects;

Expand Down Expand Up @@ -126,12 +127,63 @@ public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttribut
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attributes) throws IOException {
// Files.deleteIfExists() never follows links, so use LinkOption.NOFOLLOW_LINKS in other calls to Files.
if (accept(file) && Files.exists(file, LinkOption.NOFOLLOW_LINKS)) {
if (overrideReadOnly) {
PathUtils.setReadOnly(file, false, LinkOption.NOFOLLOW_LINKS);
final Path parent = file.getParent();
final PosixFileAttributes parentAttributes = overrideReadOnly ? PathUtils.readPosixFileAttributes(parent, LinkOption.NOFOLLOW_LINKS) : null;
boolean fileWasReadOnly = false;
Throwable failure = null;
try {
if (overrideReadOnly) {
fileWasReadOnly = PathUtils.isReadOnly(file, LinkOption.NOFOLLOW_LINKS);
PathUtils.setReadOnly(file, false, LinkOption.NOFOLLOW_LINKS);
}
Files.deleteIfExists(file);
} catch (final IOException | RuntimeException | Error e) {
failure = e;
throw e;
} finally {
Throwable restorationFailure = null;
try {
if (Files.exists(file, LinkOption.NOFOLLOW_LINKS) && fileWasReadOnly) {
PathUtils.setReadOnly(file, true, LinkOption.NOFOLLOW_LINKS);
}
} catch (final IOException | RuntimeException | Error e) {
restorationFailure = e;
}
try {
if (parentAttributes != null) {
Files.setPosixFilePermissions(parent, parentAttributes.permissions());
}
} catch (final IOException | RuntimeException | Error e) {
if (restorationFailure == null) {
restorationFailure = e;
} else if (restorationFailure != e) {
restorationFailure.addSuppressed(e);
}
}
if (restorationFailure != null) {
if (failure == null) {
throwRestorationFailure(restorationFailure);
}
if (failure != restorationFailure) {
failure.addSuppressed(restorationFailure);
}
}
}
Files.deleteIfExists(file);
}
updateFileCounters(file, attributes);
return FileVisitResult.CONTINUE;
}
}

private static void throwRestorationFailure(final Throwable failure) throws IOException {
if (failure instanceof IOException) {
throw (IOException) failure;
}
if (failure instanceof RuntimeException) {
throw (RuntimeException) failure;
}
if (failure instanceof Error) {
throw (Error) failure;
}
throw new IOException(failure);
}
}
81 changes: 67 additions & 14 deletions src/main/java/org/apache/commons/io/file/DeletingPathVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.attribute.PosixFileAttributes;
import java.util.Arrays;
import java.util.Objects;

Expand Down Expand Up @@ -170,25 +171,77 @@ public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttribut
@Override
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException {
if (accept(file)) {
// Include dangling links so their parent permissions can also be overridden.
if (Files.exists(file, linkOptions) || Files.isSymbolicLink(file)) {
if (overrideReadOnly) {
// An absolute path supplies the parent needed by the POSIX permission override.
PathUtils.setReadOnly(file.toAbsolutePath(), false, PathUtils.noFollowLinkOptionArray());
final Path absoluteFile = file.toAbsolutePath();
final Path parent = absoluteFile.getParent();
final LinkOption[] noFollowLinkOptions = PathUtils.noFollowLinkOptionArray();
final PosixFileAttributes parentAttributes = overrideReadOnly ? PathUtils.readPosixFileAttributes(parent, noFollowLinkOptions) : null;
boolean fileWasReadOnly = false;
Throwable failure = null;
try {
// Include dangling links so their parent permissions can also be overridden.
if (Files.exists(file, linkOptions) || Files.isSymbolicLink(file)) {
if (overrideReadOnly) {
fileWasReadOnly = PathUtils.isReadOnly(absoluteFile, noFollowLinkOptions);
// An absolute path supplies the parent needed by the POSIX permission override.
PathUtils.setReadOnly(absoluteFile, false, noFollowLinkOptions);
}
Files.deleteIfExists(file);
}
Files.deleteIfExists(file);
}
// invalid links will survive previous delete, different approach needed:
if (Files.isSymbolicLink(file)) {
// Invalid links may survive the previous delete, so try a direct deletion.
if (Files.isSymbolicLink(file)) {
try {
Files.delete(file);
} catch (final NoSuchFileException ignored) {
// Ignore.
}
}
} catch (final IOException | RuntimeException | Error e) {
failure = e;
throw e;
} finally {
Throwable restorationFailure = null;
try {
// deleteIfExists does not work for this case
Files.delete(file);
} catch (final NoSuchFileException ignored) {
// ignore
if ((Files.exists(file, noFollowLinkOptions) || Files.isSymbolicLink(file)) && fileWasReadOnly) {
PathUtils.setReadOnly(absoluteFile, true, noFollowLinkOptions);
}
} catch (final IOException | RuntimeException | Error e) {
restorationFailure = e;
}
try {
if (parentAttributes != null) {
Files.setPosixFilePermissions(parent, parentAttributes.permissions());
}
} catch (final IOException | RuntimeException | Error e) {
if (restorationFailure == null) {
restorationFailure = e;
} else if (restorationFailure != e) {
restorationFailure.addSuppressed(e);
}
}
if (restorationFailure != null) {
if (failure == null) {
throwRestorationFailure(restorationFailure);
}
if (failure != restorationFailure) {
failure.addSuppressed(restorationFailure);
}
}
}
}
updateFileCounters(file, attrs);
return FileVisitResult.CONTINUE;
}
}

private static void throwRestorationFailure(final Throwable failure) throws IOException {
if (failure instanceof IOException) {
throw (IOException) failure;
}
if (failure instanceof RuntimeException) {
throw (RuntimeException) failure;
}
if (failure instanceof Error) {
throw (Error) failure;
}
throw new IOException(failure);
}
}
56 changes: 54 additions & 2 deletions src/main/java/org/apache/commons/io/file/PathUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,9 +736,12 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp
}
final Path parent = getParent(file);
PosixFileAttributes posixFileAttributes = null;
boolean fileWasReadOnly = false;
Throwable failure = null;
try {
if (overrideReadOnly(deleteOptions)) {
posixFileAttributes = readPosixFileAttributes(parent, linkOptions);
fileWasReadOnly = isReadOnly(file, linkOptions);
setReadOnly(file, false, linkOptions);
}
// Read size _after_ having read/execute access on POSIX.
Expand All @@ -748,14 +751,54 @@ public static PathCounters deleteFile(final Path file, final LinkOption[] linkOp
pathCounts.getFileCounter().increment();
pathCounts.getByteCounter().add(size);
}
} catch (final IOException | RuntimeException | Error e) {
failure = e;
throw e;
} finally {
if (posixFileAttributes != null) {
Files.setPosixFilePermissions(parent, posixFileAttributes.permissions());
Throwable restorationFailure = null;
try {
if (fileWasReadOnly && exists(file, linkOptions)) {
setReadOnly(file, true, linkOptions);
}
} catch (final IOException | RuntimeException | Error e) {
restorationFailure = e;
}
try {
if (posixFileAttributes != null) {
Files.setPosixFilePermissions(parent, posixFileAttributes.permissions());
}
} catch (final IOException | RuntimeException | Error e) {
if (restorationFailure == null) {
restorationFailure = e;
} else if (restorationFailure != e) {
restorationFailure.addSuppressed(e);
}
}
if (restorationFailure != null) {
if (failure == null) {
throwRestorationFailure(restorationFailure);
}
if (failure != restorationFailure) {
failure.addSuppressed(restorationFailure);
}
}
}
return pathCounts;
}

private static void throwRestorationFailure(final Throwable failure) throws IOException {
if (failure instanceof IOException) {
throw (IOException) failure;
}
if (failure instanceof RuntimeException) {
throw (RuntimeException) failure;
}
if (failure instanceof Error) {
throw (Error) failure;
}
throw new IOException(failure);
}

/**
* A null-safe version of {@link Files#deleteIfExists(Path)}.
*
Expand Down Expand Up @@ -1613,6 +1656,15 @@ public static PosixFileAttributes readPosixFileAttributes(final Path path, final
return readAttributes(path, PosixFileAttributes.class, options);
}

static boolean isReadOnly(final Path path, final LinkOption... options) {
final DosFileAttributes dosFileAttributes = readDosFileAttributes(path, options);
if (dosFileAttributes != null) {
return dosFileAttributes.isReadOnly();
}
final PosixFileAttributes posixFileAttributes = readPosixFileAttributes(path, options);
return posixFileAttributes != null && !posixFileAttributes.permissions().contains(PosixFilePermission.OWNER_WRITE);
}

/**
* Reads the file contents at the given path as a String using the Charset.
*
Expand Down
105 changes: 105 additions & 0 deletions src/test/java/org/apache/commons/io/file/CleaningPathVisitorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,30 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertSame;
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.mockito.Answers.CALLS_REAL_METHODS;
import static org.mockito.Mockito.mockStatic;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.util.Set;

import org.apache.commons.io.file.Counters.PathCounters;
import org.junit.jupiter.api.Test;
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.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.MockedStatic;

/**
* Tests {@link DeletingPathVisitor}.
Expand Down Expand Up @@ -138,6 +150,99 @@ void testCleanFolders2FileSize2(final CleaningPathVisitor visitor) throws IOExce
assertEquals(visitFileTree.hashCode(), visitFileTree.hashCode());
}

@Test
@EnabledOnOs(OS.WINDOWS)
void testOverrideReadOnlyRestoresWritableFileAfterFailedDelete() throws IOException {
final Path file = Files.createFile(tempDir.resolve("writable.txt"));
assertTrue(Files.isWritable(file));
PathUtils.setReadOnly(tempDir, true);
try {
final CleaningPathVisitor visitor = new CleaningPathVisitor(Counters.longPathCounters(),
new DeleteOption[] {StandardDeleteOption.OVERRIDE_READ_ONLY});
assertThrows(IOException.class, () -> Files.walkFileTree(tempDir, visitor));
} finally {
PathUtils.setReadOnly(tempDir, false);
}
assertTrue(Files.exists(file));
assertTrue(Files.isWritable(file), "A failed deletion must preserve the file's writable state");
}

@Test
void testOverrideReadOnlyRestoresParentPermissionsAfterSuccessfulDelete() throws IOException {
final Path parent = Files.createDirectory(tempDir.resolve("read-only-parent"));
final Path file = Files.write(parent.resolve("file.txt"), new byte[] {1});
assumeTrue(Files.getFileAttributeView(parent, PosixFileAttributeView.class) != null);
final Set<PosixFilePermission> tempPermissions = Files.getPosixFilePermissions(parent);
final Set<PosixFilePermission> originalPermissions = PosixFilePermissions.fromString("r-x------");
try {
Files.setPosixFilePermissions(parent, originalPermissions);
final CleaningPathVisitor visitor = new CleaningPathVisitor(Counters.longPathCounters(),
new DeleteOption[] {StandardDeleteOption.OVERRIDE_READ_ONLY});
Files.walkFileTree(file, visitor);
assertTrue(Files.notExists(file));
assertEquals(originalPermissions, Files.getPosixFilePermissions(parent));
} finally {
if (Files.exists(parent)) {
Files.setPosixFilePermissions(parent, tempPermissions);
}
}
}

@ParameterizedTest
@ValueSource(booleans = { false, true })
void testOverrideReadOnlyRestoresFileAndParentPermissionsAfterFailedDelete(final boolean readOnly) throws IOException {
final Path parent = Files.createDirectory(tempDir.resolve("failed-delete-parent"));
final Path file = Files.write(parent.resolve("file.txt"), new byte[] {1});
assumeTrue(Files.getFileAttributeView(parent, PosixFileAttributeView.class) != null);
final Set<PosixFilePermission> tempPermissions = Files.getPosixFilePermissions(parent);
final Set<PosixFilePermission> originalFilePermissions = PosixFilePermissions.fromString(readOnly ? "r--------" : "rw-------");
final Set<PosixFilePermission> originalParentPermissions = PosixFilePermissions.fromString("r-x------");
final IOException deleteFailure = new IOException("Cannot delete file");
try {
Files.setPosixFilePermissions(file, originalFilePermissions);
Files.setPosixFilePermissions(parent, originalParentPermissions);
final CleaningPathVisitor visitor = new CleaningPathVisitor(Counters.longPathCounters(),
new DeleteOption[] {StandardDeleteOption.OVERRIDE_READ_ONLY});
try (MockedStatic<Files> files = mockStatic(Files.class, CALLS_REAL_METHODS)) {
files.when(() -> Files.deleteIfExists(file)).thenThrow(deleteFailure);
assertSame(deleteFailure, assertThrows(IOException.class, () -> Files.walkFileTree(file, visitor)));
}
assertTrue(Files.exists(file));
assertEquals(originalFilePermissions, Files.getPosixFilePermissions(file));
assertEquals(originalParentPermissions, Files.getPosixFilePermissions(parent));
} finally {
if (Files.exists(parent)) {
Files.setPosixFilePermissions(parent, tempPermissions);
}
}
}

@Test
void testOverrideReadOnlyDoesNotHideDeleteFailureWhenParentRestorationFails() throws IOException {
final Path parent = Files.createDirectory(tempDir.resolve("restore-failure-parent"));
final Path file = Files.write(parent.resolve("file.txt"), new byte[] {1});
assumeTrue(Files.getFileAttributeView(parent, PosixFileAttributeView.class) != null);
final Set<PosixFilePermission> tempPermissions = Files.getPosixFilePermissions(parent);
final Set<PosixFilePermission> originalParentPermissions = PosixFilePermissions.fromString("r-x------");
final IOException deleteFailure = new IOException("Cannot delete file");
final IOException restoreFailure = new IOException("Cannot restore parent permissions");
try {
Files.setPosixFilePermissions(parent, originalParentPermissions);
final CleaningPathVisitor visitor = new CleaningPathVisitor(Counters.longPathCounters(),
new DeleteOption[] {StandardDeleteOption.OVERRIDE_READ_ONLY});
try (MockedStatic<Files> files = mockStatic(Files.class, CALLS_REAL_METHODS)) {
files.when(() -> Files.deleteIfExists(file)).thenThrow(deleteFailure);
files.when(() -> Files.setPosixFilePermissions(parent, originalParentPermissions)).thenThrow(restoreFailure);
assertSame(deleteFailure, assertThrows(IOException.class, () -> Files.walkFileTree(file, visitor)));
}
assertSame(restoreFailure, deleteFailure.getSuppressed()[0]);
} finally {
if (Files.exists(parent)) {
Files.setPosixFilePermissions(parent, tempPermissions);
}
}
}

@Test
void testEqualsHashCode() {
final CountingPathVisitor visitor0 = CleaningPathVisitor.withLongCounters();
Expand Down
Loading