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"); + } +}