diff --git a/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt index 0dd1730..8d8cbd3 100644 --- a/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt +++ b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt @@ -64,9 +64,18 @@ object InputQuery { * A join's total is only as good as its worst-known part. Adding up the ones that answered * would produce a lower bound that reads exactly like a real total, and the space check has * no way to tell the two apart — which is the same conflation this whole file exists to end. + * + * The sum saturates rather than wrapping. Sizes reach here non-negative — both of the ways one + * is found reject a negative answer — but nothing bounds their *sum*, and a total that wrapped + * negative would not be harmless nonsense: `OutputPublisher.hasSpaceFor` compares it against + * free space, so the largest join representable would come back as the one with the most room. */ fun total(sizes: List): Long? = sizes.fold(0L as Long?) { running, size -> - if (running == null || size == null) null else running + size + when { + running == null || size == null -> null + size > Long.MAX_VALUE - running -> Long.MAX_VALUE + else -> running + size + } } /** diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 504510b..430b135 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -67,8 +67,18 @@ open class OutputPublisher(private val context: Context) { * through its engine, with a message of its own. * * Open so a test can force a full disk; see `FakeFailures` in the instrumented source set. + * + * Written as `free - headroom > required` rather than the equivalent-looking + * `free > required + headroom`. The second overflows: a request within 128 MiB of + * [Long.MAX_VALUE] wraps the sum negative, every free-space measurement beats a negative + * number, and the check answers "plenty of room" to the largest request it can be given. That + * is reachable rather than theoretical — [InputQuery.total] sums a join's inputs, so the number + * arriving here is not bounded by any single file. Both operands are clamped at zero first, so + * the subtraction cannot underflow and a nonsense negative size decides exactly as zero does + * instead of buying slack. */ - open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES + open fun hasSpaceFor(bytes: Long): Boolean = + stagingDir.usableSpace.coerceAtLeast(0L) - SPACE_HEADROOM_BYTES > bytes.coerceAtLeast(0L) /** * The same check for a job whose input size nobody could determine — see [InputQuery]. @@ -128,14 +138,22 @@ open class OutputPublisher(private val context: Context) { * which is the right way round, since a flush that failed means the bytes are not * durably there to begin with. * - * A failure from `openOutputStream` itself is deliberately outside the guard. Nothing - * has been written at that point, so there is nothing of ours to remove. + * `openOutputStream` is inside the guard as well, and the reasoning that used to keep it + * out -- "nothing has been written at that point, so there is nothing of ours to remove" -- + * was wrong about what exists. SAF's `CreateDocument` contract creates the document *before* + * this is called, which is why every fixture in `OutputPublisherPublishTest` starts as an + * existing empty file. So a provider that hands out no stream at all -- gone between the + * picker and the write, or simply returning null -- left a zero-byte file at the name the + * user chose while the UI said "Could not save the file". The two bounds above are what make + * removing it safe, and they apply to this case exactly as they do to a failed copy. A + * provider that will not open its own empty document may well refuse to delete it too, which + * is already [deletePartialOutput]'s documented no-op path. */ open fun publish(staged: File, destination: Uri) { val destinationWasEmpty = destinationIsKnownEmpty(destination) - val out = context.contentResolver.openOutputStream(destination) - ?: error("Could not open destination for writing: $destination") try { + val out = context.contentResolver.openOutputStream(destination) + ?: error("Could not open destination for writing: $destination") out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } } catch (failure: Throwable) { if (destinationWasEmpty) deletePartialOutput(destination, failure) diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index 09f1adc..88431f6 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -265,10 +265,34 @@ class OutputPublisherPublishTest { ) } + @Test + fun `a destination the provider will not open does not stay behind as an empty file`() { + // No stream supplier is registered for this URI and the fake provider does not implement + // openFile, which is a provider that has gone away between the picker and the write. + // + // The document exists all the same: SAF's CreateDocument contract created it before + // publish() was ever called, so "nothing has been written yet" was never the same claim as + // "there is nothing of ours here". Leaving it means a zero-byte file at the name the user + // chose, while the screen says the save failed. + val destination = FakeSafProvider.backingFile(documentUri) + assertEquals("the fixture starts as the empty document SAF hands back", 0L, destination.length()) + + val failure = runCatching { publisher.publish(staged, documentUri) }.exceptionOrNull() + + assertTrue("a destination that will not open must not appear to succeed, got $failure", failure != null) + assertEquals(listOf(documentUri), FakeSafProvider.deleteRequests) + assertFalse( + "a zero-byte file must not be left at the name the user picked", + destination.exists(), + ) + } + @Test fun `a destination that cannot be opened at all fails without any cleanup`() { - // The JVM twin of UnopenableUriTest's unwritable-destination case. Nothing was - // written, so there is nothing of ours to remove. + // The JVM twin of UnopenableUriTest's unwritable-destination case. The open sits inside + // the guarded region now, so what keeps this one untouched is the guard rather than the + // placement: nothing answers for that authority, so no size can be read, and "I could not + // tell" must never authorise a delete. val failure = runCatching { publisher.publish(staged, deadUri) }.exceptionOrNull() assertTrue("publishing to a dead provider must not appear to succeed, got $failure", failure != null) diff --git a/app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt b/app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt new file mode 100644 index 0000000..da32743 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/SpaceArithmeticTest.kt @@ -0,0 +1,79 @@ +package org.libremediaconverter.convert + +import android.content.Context +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * The two sums the space check is made of, at the sizes where addition stops working. + * + * `SpaceCheckTest` pins which *question* each worker asks; this pins what the answer is once the + * number is large. Both halves were live on main: `hasSpaceFor` added the headroom to the request + * before comparing, and [InputQuery.total] folded a join's inputs with nothing stopping the sum + * from wrapping. A wrapped total is not merely nonsense — it is negative, and every free-space + * measurement beats a negative number, so the check that exists to refuse impossible jobs approved + * the most impossible one it can be handed. + * + * Nothing here is about the *allocatable-versus-usable* question, which is a separate decision + * still parked. This is the arithmetic on whichever number that decision ends up producing. + */ +@RunWith(RobolectricTestRunner::class) +class SpaceArithmeticTest { + + private lateinit var context: Context + private lateinit var publisher: OutputPublisher + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication() + publisher = OutputPublisher(context) + } + + @Test + fun `a request no disk could hold is refused rather than wrapping into plenty of room`() { + assertFalse("eight exabytes do not fit anywhere", publisher.hasSpaceFor(Long.MAX_VALUE)) + // Just inside the headroom of the maximum, which is the arithmetic's actual edge: this is + // the range where `bytes + headroom` goes negative while `bytes` alone still looks huge. + assertFalse(publisher.hasSpaceFor(Long.MAX_VALUE - ONE_HUNDRED_MIB)) + } + + // The clamp on a negative size is deliberately NOT asserted here. It only changes the answer + // when free space is below the headroom, which this test cannot arrange -- the publisher reads + // the host's real cache volume -- so any assertion available would pass against the unclamped + // arithmetic too, and a test that cannot fail is worse than the gap it appears to close. + + @Test + fun `an ordinary request is still allowed, so the refusals above are not vacuous`() { + val free = File(context.cacheDir, "conversions").usableSpace + assertTrue( + "a one-byte conversion must fit; the volume under the cache reports $free bytes free", + publisher.hasSpaceFor(1L), + ) + } + + @Test + fun `a join total too large to represent saturates instead of turning negative`() { + val enormous = listOf(FOUR_EXABYTES, FOUR_EXABYTES, FOUR_EXABYTES) + + val total = InputQuery.total(enormous) + + assertEquals(Long.MAX_VALUE, total) + // The whole point, in the shape the defect had: this total is handed straight to the space + // check by ConcatWorker, and before the clamp it arrived negative and was approved. + assertFalse("a join of three four-exabyte files does not fit", publisher.hasSpaceFor(total!!)) + } + + private companion object { + const val ONE_HUNDRED_MIB = 100L * 1024 * 1024 + + /** Big enough that three of them overflow, small enough to be a plausible `statSize`. */ + const val FOUR_EXABYTES = 4_000_000_000_000_000_000L + } +}