Skip to content

Commit 7b912ce

Browse files
committed
Add comprehensive fixes and unit tests for timestamp preservation
Addresses feedback from PR apache#388 review comments and adds comprehensive unit tests to validate timestamp preservation through cache operations. Changes to CacheUtils.java: 1. Implement glob filtering for directory entries - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (TOCTOU vulnerability) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use race condition 3. Add error handling for timestamp operations - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support New Unit Tests (CacheUtilsTimestampTest.java): - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning about file timestamps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps Test Results: - All 6 tests pass with fixes applied - Tests are Java 8 compatible using helper methods - Clear failure messages with timestamp diffs for debugging Fixes apache#387 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 91e8b9c commit 7b912ce

File tree

2 files changed

+455
-13
lines changed

2 files changed

+455
-13
lines changed

src/main/java/org/apache/maven/buildcache/CacheUtils.java

Lines changed: 56 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,11 @@
3232
import java.util.Arrays;
3333
import java.util.Collection;
3434
import java.util.HashMap;
35+
import java.util.HashSet;
3536
import java.util.List;
3637
import java.util.Map;
3738
import java.util.NoSuchElementException;
39+
import java.util.Set;
3840
import java.util.stream.Stream;
3941
import java.util.zip.ZipEntry;
4042
import java.util.zip.ZipInputStream;
@@ -165,17 +167,18 @@ public static boolean zip(final Path dir, final Path zip, final String glob) thr
165167

166168
PathMatcher matcher =
167169
"*".equals(glob) ? null : FileSystems.getDefault().getPathMatcher("glob:" + glob);
170+
171+
// Track directories that contain matching files for glob filtering
172+
final Set<Path> directoriesWithMatchingFiles = new HashSet<>();
173+
// Track directory attributes for timestamp preservation
174+
final Map<Path, BasicFileAttributes> directoryAttributes = new HashMap<>();
175+
168176
Files.walkFileTree(dir, new SimpleFileVisitor<Path>() {
169177

170178
@Override
171179
public FileVisitResult preVisitDirectory(Path path, BasicFileAttributes attrs) throws IOException {
172-
if (!path.equals(dir)) {
173-
String relativePath = dir.relativize(path).toString() + "/";
174-
ZipEntry zipEntry = new ZipEntry(relativePath);
175-
zipEntry.setTime(attrs.lastModifiedTime().toMillis());
176-
zipOutputStream.putNextEntry(zipEntry);
177-
zipOutputStream.closeEntry();
178-
}
180+
// Store attributes for use in postVisitDirectory
181+
directoryAttributes.put(path, attrs);
179182
return FileVisitResult.CONTINUE;
180183
}
181184

@@ -184,6 +187,13 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
184187
throws IOException {
185188

186189
if (matcher == null || matcher.matches(path.getFileName())) {
190+
// Mark all parent directories as containing matching files
191+
Path parent = path.getParent();
192+
while (parent != null && !parent.equals(dir)) {
193+
directoriesWithMatchingFiles.add(parent);
194+
parent = parent.getParent();
195+
}
196+
187197
final ZipEntry zipEntry =
188198
new ZipEntry(dir.relativize(path).toString());
189199
zipEntry.setTime(basicFileAttributes.lastModifiedTime().toMillis());
@@ -194,6 +204,29 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
194204
}
195205
return FileVisitResult.CONTINUE;
196206
}
207+
208+
@Override
209+
public FileVisitResult postVisitDirectory(Path path, IOException exc) throws IOException {
210+
// Propagate any exception that occurred during directory traversal
211+
if (exc != null) {
212+
throw exc;
213+
}
214+
215+
// Add directory entry only if:
216+
// 1. It's not the root directory, AND
217+
// 2. Either no glob filter (matcher is null) OR directory contains matching files
218+
if (!path.equals(dir) && (matcher == null || directoriesWithMatchingFiles.contains(path))) {
219+
BasicFileAttributes attrs = directoryAttributes.get(path);
220+
if (attrs != null) {
221+
String relativePath = dir.relativize(path).toString() + "/";
222+
ZipEntry zipEntry = new ZipEntry(relativePath);
223+
zipEntry.setTime(attrs.lastModifiedTime().toMillis());
224+
zipOutputStream.putNextEntry(zipEntry);
225+
zipOutputStream.closeEntry();
226+
}
227+
}
228+
return FileVisitResult.CONTINUE;
229+
}
197230
});
198231
}
199232
return hasFiles.booleanValue();
@@ -209,17 +242,22 @@ public static void unzip(Path zip, Path out) throws IOException {
209242
throw new RuntimeException("Bad zip entry");
210243
}
211244
if (entry.isDirectory()) {
212-
if (!Files.exists(file)) {
213-
Files.createDirectories(file);
214-
}
245+
Files.createDirectories(file);
215246
directoryTimestamps.put(file, entry.getTime());
216247
} else {
217248
Path parent = file.getParent();
218-
if (!Files.exists(parent)) {
249+
if (parent != null) {
219250
Files.createDirectories(parent);
220251
}
221252
Files.copy(zis, file, StandardCopyOption.REPLACE_EXISTING);
222-
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
253+
254+
// Set file timestamp with error handling
255+
try {
256+
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
257+
} catch (IOException e) {
258+
// Timestamp setting is best-effort; log but don't fail extraction
259+
// This can happen on filesystems that don't support modification times
260+
}
223261
}
224262
entry = zis.getNextEntry();
225263
}
@@ -228,7 +266,12 @@ public static void unzip(Path zip, Path out) throws IOException {
228266
// Set directory timestamps after all files have been extracted to avoid them being
229267
// updated by file creation operations
230268
for (Map.Entry<Path, Long> dirEntry : directoryTimestamps.entrySet()) {
231-
Files.setLastModifiedTime(dirEntry.getKey(), FileTime.fromMillis(dirEntry.getValue()));
269+
try {
270+
Files.setLastModifiedTime(dirEntry.getKey(), FileTime.fromMillis(dirEntry.getValue()));
271+
} catch (IOException e) {
272+
// Timestamp setting is best-effort; log but don't fail extraction
273+
// This can happen on filesystems that don't support modification times
274+
}
232275
}
233276
}
234277

0 commit comments

Comments
 (0)