From 72e2755d234d1ed5e22b77c98e09944d5da1afc1 Mon Sep 17 00:00:00 2001 From: Claude Tuxedo Date: Fri, 11 Sep 2026 13:17:26 +0200 Subject: [PATCH] fix: close driver and agent file streams even when an exception interrupts them Three file streams were closed by hand after their read or write, so any exception raised in between skipped the close and leaked the handle. On Windows a leaked handle keeps the file locked, and the pack folder the transfer was building could then neither be finished nor removed. - CipherUtils.addBootFileV2: the `ri` input and `bt` output streams - RawStoryTellerAsyncDriver.dumpSector: the sector dump output stream - UnofficialMetadataAdvice: the thumbnail cache output stream All three now use try-with-resources. No change on the happy path. The failure path cannot be reached from outside without a seam the production code does not need (a missing `ri` fails before the handle exists, a short one is ciphered without error), so this is a correctness fix by inspection. BootFileStreamLifecycleTest pins the nominal contract of addBootFileV2, which had no test: the shape of `bt`, its dependence on the device UUID, idempotence, and that both files are released on return. Closes lgnap/studio#10 Co-Authored-By: Claude Opus 5 (1M context) --- .../agent/UnofficialMetadataAdvice.java | 9 +- .../java/studio/driver/fs/CipherUtils.java | 19 ++- .../driver/raw/RawStoryTellerAsyncDriver.java | 5 +- .../fs/BootFileStreamLifecycleTest.java | 149 ++++++++++++++++++ 4 files changed, 167 insertions(+), 15 deletions(-) create mode 100644 driver/src/test/java/studio/driver/fs/BootFileStreamLifecycleTest.java diff --git a/agent/src/main/java/studio/agent/UnofficialMetadataAdvice.java b/agent/src/main/java/studio/agent/UnofficialMetadataAdvice.java index d14375d6f..74bdecd43 100644 --- a/agent/src/main/java/studio/agent/UnofficialMetadataAdvice.java +++ b/agent/src/main/java/studio/agent/UnofficialMetadataAdvice.java @@ -56,10 +56,11 @@ public static void getInputStream(@Advice.This HttpURLConnection that, @Advice.R String cacheFilePath = luniitheque + "/images/" + UUID.nameUUIDFromBytes(("http:/" + imagePath).getBytes()).toString(); logger.info("Storing unofficial metadata image into local filesystem with path: " + cacheFilePath); - FileOutputStream fos = new FileOutputStream(cacheFilePath); - byte[] bytes = Base64.getDecoder().decode(meta.getThumbnail().substring(meta.getThumbnail().indexOf(";base64,") + 8)); - fos.write(bytes); - fos.close(); + // try-with-resources: a malformed thumbnail must not leave the cache file handle open + try (FileOutputStream fos = new FileOutputStream(cacheFilePath)) { + byte[] bytes = Base64.getDecoder().decode(meta.getThumbnail().substring(meta.getThumbnail().indexOf(";base64,") + 8)); + fos.write(bytes); + } } // Generate JSON document with unofficial metadata diff --git a/driver/src/main/java/studio/driver/fs/CipherUtils.java b/driver/src/main/java/studio/driver/fs/CipherUtils.java index 0ada2531d..e9804e2a0 100644 --- a/driver/src/main/java/studio/driver/fs/CipherUtils.java +++ b/driver/src/main/java/studio/driver/fs/CipherUtils.java @@ -53,15 +53,18 @@ static void addBootFileV2(Path packFolder, byte[] deviceUuid) throws IOException // Compute specific key byte[] specificKey = computeSpecificKeyV2FromUUID(deviceUuid); // Read ciphered block of ri file - FileInputStream riFis = new FileInputStream(new File(packFolder.toFile(), FsStoryPackReader.IMAGE_INDEX_FILENAME)); - byte[] riCipheredBlock = riFis.readNBytes(CIPHER_BLOCK_SIZE_BOOT_V2); - riFis.close(); + // Both streams are closed by try-with-resources: a handle left open by an exception would keep the + // file locked on Windows, and the pack folder could then neither be completed nor removed. + byte[] riCipheredBlock; + try (FileInputStream riFis = new FileInputStream(new File(packFolder.toFile(), FsStoryPackReader.IMAGE_INDEX_FILENAME))) { + riCipheredBlock = riFis.readNBytes(CIPHER_BLOCK_SIZE_BOOT_V2); + } // Add boot file: bt - FileOutputStream btFos = new FileOutputStream(new File(packFolder.toFile(), BOOT_FILENAME)); - // The first **scrambled** 64 bytes of 'ri' file must be ciphered with the device-specific key into 'bt' file - byte[] btCiphered = cipherFirstBlockSpecificKeyV2(riCipheredBlock, specificKey); - btFos.write(btCiphered); - btFos.close(); + try (FileOutputStream btFos = new FileOutputStream(new File(packFolder.toFile(), BOOT_FILENAME))) { + // The first **scrambled** 64 bytes of 'ri' file must be ciphered with the device-specific key into 'bt' file + byte[] btCiphered = cipherFirstBlockSpecificKeyV2(riCipheredBlock, specificKey); + btFos.write(btCiphered); + } } static void addBootFileV3(Path packFolder, FsDeviceKeyV3 deviceKeyV3) throws IOException { diff --git a/driver/src/main/java/studio/driver/raw/RawStoryTellerAsyncDriver.java b/driver/src/main/java/studio/driver/raw/RawStoryTellerAsyncDriver.java index e455b638f..8a470af33 100644 --- a/driver/src/main/java/studio/driver/raw/RawStoryTellerAsyncDriver.java +++ b/driver/src/main/java/studio/driver/raw/RawStoryTellerAsyncDriver.java @@ -512,10 +512,9 @@ private CompletableFuture dumpSector(DeviceHandle handle, int sector, Stri LOGGER.info("Dumping sector " + sector + " into " + dest); return LibUsbMassStorageHelper.asyncReadSDSectors(handle, sector, (short) 1) .thenAccept(read -> { - try { - FileOutputStream sectorOutputStream = new FileOutputStream(dest); + // try-with-resources: a write failure must not leave the dump file locked on Windows + try (FileOutputStream sectorOutputStream = new FileOutputStream(dest)) { writeByteBufferToStream(read, sectorOutputStream); - sectorOutputStream.close(); } catch (IOException e) { throw new StoryTellerException("Failed to dump sector " + sector + " from SD card.", e); } diff --git a/driver/src/test/java/studio/driver/fs/BootFileStreamLifecycleTest.java b/driver/src/test/java/studio/driver/fs/BootFileStreamLifecycleTest.java new file mode 100644 index 000000000..a09a4a6c2 --- /dev/null +++ b/driver/src/test/java/studio/driver/fs/BootFileStreamLifecycleTest.java @@ -0,0 +1,149 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +package studio.driver.fs; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import studio.core.v1.reader.fs.FsStoryPackReader; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Pins what {@code addBootFileV2} produces, now that its two streams are closed by try-with-resources. + * + *

Before this change the {@code ri} input stream and the {@code bt} output stream were closed by + * hand after the read and the write. Any exception raised in between — an I/O error while reading + * {@code ri}, a disk-full error while writing {@code bt} — skipped the {@code close()} and leaked the + * handle. On Windows a leaked handle keeps the file locked, so the very pack folder the transfer was + * building could then neither be finished nor removed. Same family of defect as the directory + * handles fixed in {@link FilesWalkResourceLeakTest}, on a {@code FileInputStream} and a + * {@code FileOutputStream}. + * + *

Why the failure path is not exercised here

+ * + *

There is no input that makes the method fail between opening a stream and closing it. + * A missing or unreadable {@code ri} fails in the {@code FileInputStream} constructor, before any + * handle exists; a short or empty {@code ri} is read and ciphered without error (the cipher trims to + * whatever is there); and the {@code bt} write of at most 64 bytes has no reachable failure short of + * a full disk. The leak was therefore only reachable through genuine I/O errors, which cannot be + * injected from a unit test without a seam the production code does not need. The closing is a + * correctness fix by inspection, like the {@code getFolderSize} closing pinned by + * {@link FilesWalkResourceLeakTest#measuringFolderSizeReleasesTheTree()}. + * + *

What this class does pin is the nominal contract, which the rewrite must not have altered: + * the shape of {@code bt}, its dependence on the device UUID, and the fact that both files are + * released — and thus deletable — once the method has returned. + */ +class BootFileStreamLifecycleTest { + + private static final int BOOT_BLOCK = 64; + + /** + * A device UUID as the driver reads it from the {@code .md} file: 16 ciphered bytes, from which + * the device-specific boot key is derived. The values carry no meaning beyond being distinct. + */ + private static byte[] deviceUuid(int seed) { + byte[] uuid = new byte[16]; + for (int i = 0; i < uuid.length; i++) { + uuid[i] = (byte) (seed + i * 7); + } + return uuid; + } + + private static byte[] pattern(int length) { + byte[] data = new byte[length]; + for (int i = 0; i < length; i++) { + data[i] = (byte) (i % 251); + } + return data; + } + + private static Path packWithImageIndex(Path folder, byte[] ri) throws IOException { + Files.write(folder.resolve(FsStoryPackReader.IMAGE_INDEX_FILENAME), ri); + return folder; + } + + @Test + @DisplayName("writes a 64-byte bt derived from the first block of ri and the device UUID") + void writesBootFileFromImageIndexPrefix(@TempDir Path folder) throws IOException { + byte[] ri = pattern(200); + packWithImageIndex(folder, ri); + + CipherUtils.addBootFileV2(folder, deviceUuid(0x10)); + + byte[] bt = Files.readAllBytes(folder.resolve("bt")); + assertEquals(BOOT_BLOCK, bt.length, "bt is exactly the first ciphered block of ri"); + assertFalse(Arrays.equals(Arrays.copyOf(ri, BOOT_BLOCK), bt), + "bt is ciphered with the device-specific key, not a plain copy of ri"); + assertArrayEquals(ri, Files.readAllBytes(folder.resolve(FsStoryPackReader.IMAGE_INDEX_FILENAME)), + "ri is read, never rewritten"); + } + + @Test + @DisplayName("bt depends on the device UUID, so a pack is bound to one device") + void bootFileDependsOnDeviceUuid(@TempDir Path first, @TempDir Path second) throws IOException { + byte[] ri = pattern(200); + packWithImageIndex(first, ri); + packWithImageIndex(second, ri); + + CipherUtils.addBootFileV2(first, deviceUuid(0x10)); + CipherUtils.addBootFileV2(second, deviceUuid(0x20)); + + assertFalse(Arrays.equals(Files.readAllBytes(first.resolve("bt")), Files.readAllBytes(second.resolve("bt"))), + "two devices must not get the same bt for the same ri"); + } + + @Test + @DisplayName("is deterministic and overwrites a previous bt rather than appending to it") + void isDeterministicAndOverwrites(@TempDir Path folder) throws IOException { + packWithImageIndex(folder, pattern(200)); + + CipherUtils.addBootFileV2(folder, deviceUuid(0x10)); + byte[] once = Files.readAllBytes(folder.resolve("bt")); + CipherUtils.addBootFileV2(folder, deviceUuid(0x10)); + byte[] twice = Files.readAllBytes(folder.resolve("bt")); + + assertArrayEquals(once, twice); + assertEquals(BOOT_BLOCK, twice.length, "a second run replaces bt, it does not grow it"); + } + + @Test + @DisplayName("a short ri is ciphered as-is, which is the only degenerate input the method accepts") + void shortImageIndexIsHandledWithoutError(@TempDir Path folder) throws IOException { + // Documents the reason the failure path cannot be reached from outside: a truncated ri does + // not make the read throw, it just yields a shorter bt. + packWithImageIndex(folder, pattern(10)); + + CipherUtils.addBootFileV2(folder, deviceUuid(0x10)); + + assertEquals(10, Files.readAllBytes(folder.resolve("bt")).length); + } + + @Test + @DisplayName("both files are released once the method has returned") + void releasesBothFilesOnReturn(@TempDir Path folder) throws IOException { + // On Windows this is the observable consequence of a closed handle: the file can be deleted. + // On other platforms the delete succeeds regardless, so this is the nominal control case that + // the FAT32 classes rely on, kept here so the rewrite cannot regress it on the platform where + // it matters. + packWithImageIndex(folder, pattern(200)); + + CipherUtils.addBootFileV2(folder, deviceUuid(0x10)); + + assertTrue(Files.deleteIfExists(folder.resolve("bt")), "bt must be deletable"); + assertTrue(Files.deleteIfExists(folder.resolve(FsStoryPackReader.IMAGE_INDEX_FILENAME)), "ri must be deletable"); + } +}