1
0
mirror of https://github.com/SquidDev-CC/CC-Tweaked synced 2024-06-25 22:53:22 +00:00

Fix several off-by-one issues in UploadFileMessage

We now fuzz UploadFileMessage, generating random files and checking they
round-trip correctly.

The joy of having a long-lasting refactor branch with an absolutely
massive diff, is that you end up spotting bugs, and then it's a massive
pain to merge the fix back into trunk!
This commit is contained in:
Jonathan Coates 2022-10-21 23:10:18 +01:00
parent b663028f42
commit 1e703f1b07
No known key found for this signature in database
GPG Key ID: B9E431FF07C98D06
12 changed files with 572 additions and 20 deletions

1
.gitignore vendored
View File

@ -6,6 +6,7 @@
/out
/doc/out/
/node_modules
/.jqwik-database
# Runtime directories
/run

View File

@ -14,9 +14,10 @@
id "cc-tweaked.illuaminate"
}
import org.apache.tools.ant.taskdefs.condition.Os
import cc.tweaked.gradle.IlluaminateExec
import cc.tweaked.gradle.IlluaminateExecToDir
import org.apache.tools.ant.taskdefs.condition.Os
version = mod_version
@ -143,12 +144,12 @@ accessTransformer file('src/testMod/resources/META-INF/accesstransformer.cfg')
shade 'org.squiddev:Cobalt:0.5.7'
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.0'
testImplementation 'org.junit.jupiter:junit-jupiter-params:5.7.0'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
testImplementation 'org.hamcrest:hamcrest:2.2'
testImplementation 'org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.0'
testImplementation 'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.6.2'
testCompileOnly(libs.autoService)
testAnnotationProcessor(libs.autoService)
testImplementation(libs.bundles.test)
testImplementation(libs.bundles.kotlin)
testRuntimeOnly(libs.bundles.testRuntime)
testModImplementation sourceSets.main.output

29
gradle/libs.versions.toml Normal file
View File

@ -0,0 +1,29 @@
[versions]
autoService = "1.0.1"
kotlin = "1.7.10"
kotlin-coroutines = "1.6.0"
# Testing
hamcrest = "2.2"
jqwik = "1.7.0"
junit = "5.9.1"
[libraries]
autoService = { module = "com.google.auto.service:auto-service", version.ref = "autoService" }
kotlin-coroutines = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-core", version.ref = "kotlin-coroutines" }
kotlin-stdlib = { module = "org.jetbrains.kotlin:kotlin-stdlib-jdk8", version.ref = "kotlin" }
# Testing
hamcrest = { module = "org.hamcrest:hamcrest", version.ref = "hamcrest" }
jqwik-api = { module = "net.jqwik:jqwik-api", version.ref = "jqwik" }
jqwik-engine = { module = "net.jqwik:jqwik-engine", version.ref = "jqwik" }
junit-jupiter-api = { module = "org.junit.jupiter:junit-jupiter-api", version.ref = "junit" }
junit-jupiter-engine = { module = "org.junit.jupiter:junit-jupiter-engine", version.ref = "junit" }
junit-jupiter-params = { module = "org.junit.jupiter:junit-jupiter-params", version.ref = "junit" }
[bundles]
kotlin = ["kotlin-stdlib", "kotlin-coroutines"]
# Testing
test = ["junit-jupiter-api", "junit-jupiter-params", "hamcrest", "jqwik-api"]
testRuntime = ["junit-jupiter-engine", "jqwik-engine"]

View File

@ -191,7 +191,7 @@ public void onFilesDrop( @Nonnull List<Path> files )
return;
}
if( toUpload.size() > 0 ) UploadFileMessage.send( menu, toUpload );
if( toUpload.size() > 0 ) UploadFileMessage.send( menu, toUpload, NetworkHandler::sendToServer );
}
public void uploadResult( UploadResult result, ITextComponent message )

View File

@ -23,7 +23,7 @@
/**
* A snapshot of a terminal's state.
*
* <p>
* This is somewhat memory inefficient (we build a buffer, only to write it elsewhere), however it means we get a
* complete and accurate description of a terminal, which avoids a lot of complexities with resizing terminals, dirty
* states, etc...

View File

@ -5,11 +5,11 @@
*/
package dan200.computercraft.shared.network.server;
import com.google.common.annotations.VisibleForTesting;
import dan200.computercraft.shared.computer.menu.ComputerMenu;
import dan200.computercraft.shared.computer.menu.ServerInputHandler;
import dan200.computercraft.shared.computer.upload.FileSlice;
import dan200.computercraft.shared.computer.upload.FileUpload;
import dan200.computercraft.shared.network.NetworkHandler;
import io.netty.handler.codec.DecoderException;
import net.minecraft.entity.player.ServerPlayerEntity;
import net.minecraft.inventory.container.Container;
@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.function.Consumer;
public class UploadFileMessage extends ComputerServerMessage
{
@ -30,13 +31,13 @@ public class UploadFileMessage extends ComputerServerMessage
public static final int MAX_FILES = 32;
public static final int MAX_FILE_NAME = 128;
private static final int FLAG_FIRST = 1;
private static final int FLAG_LAST = 2;
static final @VisibleForTesting int FLAG_FIRST = 1;
static final @VisibleForTesting int FLAG_LAST = 2;
private final UUID uuid;
private final int flag;
private final List<FileUpload> files;
private final List<FileSlice> slices;
final @VisibleForTesting int flag;
final @VisibleForTesting List<FileUpload> files;
final @VisibleForTesting List<FileSlice> slices;
UploadFileMessage( Container menu, UUID uuid, int flag, List<FileUpload> files, List<FileSlice> slices )
{
@ -57,14 +58,14 @@ public UploadFileMessage( @Nonnull PacketBuffer buf )
if( (flag & FLAG_FIRST) != 0 )
{
int nFiles = buf.readVarInt();
if( nFiles >= MAX_FILES ) throw new DecoderException( "Too many files" );
if( nFiles > MAX_FILES ) throw new DecoderException( "Too many files" );
List<FileUpload> files = this.files = new ArrayList<>( nFiles );
for( int i = 0; i < nFiles; i++ )
{
String name = buf.readUtf( MAX_FILE_NAME );
int size = buf.readVarInt();
if( size > MAX_SIZE || (totalSize += size) >= MAX_SIZE )
if( size > MAX_SIZE || (totalSize += size) > MAX_SIZE )
{
throw new DecoderException( "Files are too large" );
}
@ -128,7 +129,7 @@ public void toBytes( @Nonnull PacketBuffer buf )
}
}
public static void send( Container container, List<FileUpload> files )
public static void send( Container container, List<FileUpload> files, Consumer<UploadFileMessage> send )
{
UUID uuid = UUID.randomUUID();
@ -148,7 +149,7 @@ public static void send( Container container, List<FileUpload> files )
{
if( remaining <= 0 )
{
NetworkHandler.sendToServer( first
send.accept( first
? new UploadFileMessage( container, uuid, FLAG_FIRST, files, new ArrayList<>( slices ) )
: new UploadFileMessage( container, uuid, 0, null, new ArrayList<>( slices ) ) );
slices.clear();
@ -167,7 +168,7 @@ public static void send( Container container, List<FileUpload> files )
contents.position( 0 ).limit( capacity );
}
NetworkHandler.sendToServer( first
send.accept( first
? new UploadFileMessage( container, uuid, FLAG_FIRST | FLAG_LAST, files, new ArrayList<>( slices ) )
: new UploadFileMessage( container, uuid, FLAG_LAST, null, new ArrayList<>( slices ) ) );
}

View File

@ -0,0 +1,152 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.shared.network.server;
import dan200.computercraft.shared.computer.upload.FileSlice;
import dan200.computercraft.shared.computer.upload.FileUpload;
import dan200.computercraft.support.ArbitraryByteBuffer;
import dan200.computercraft.support.FakeContainer;
import io.netty.buffer.Unpooled;
import net.jqwik.api.*;
import net.minecraft.network.PacketBuffer;
import org.hamcrest.Matcher;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
import static dan200.computercraft.shared.network.server.UploadFileMessage.*;
import static dan200.computercraft.support.ByteBufferMatcher.bufferEqual;
import static dan200.computercraft.support.ContramapMatcher.contramap;
import static dan200.computercraft.support.CustomMatchers.containsWith;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.*;
import static org.junit.jupiter.api.Assertions.assertEquals;
public class UploadFileMessageTest
{
/**
* Sends packets on a roundtrip, ensuring that their contents are reassembled on the other end.
*
* @param sentFiles The files to send.
*/
@Property( tries = 500 )
@Tag( "slow" )
public void testRoundTrip( @ForAll( "fileUploads" ) List<FileUpload> sentFiles )
{
List<FileUpload> receivedFiles = receive( roundtripPackets( send( sentFiles ) ) );
assertThat( receivedFiles, containsWith( sentFiles, UploadFileMessageTest::uploadEqual ) );
}
/**
* "Send" our file uploads, converting them to a list of packets.
*
* @param uploads The files to send.
* @return The list of packets.
*/
private static List<UploadFileMessage> send( List<FileUpload> uploads )
{
List<UploadFileMessage> packets = new ArrayList<>();
UploadFileMessage.send( new FakeContainer(), uploads, packets::add );
return packets;
}
/**
* Write our packets to a buffer and then read them out again.
*
* @param packets The packets to roundtrip.
* @return The
*/
private static List<UploadFileMessage> roundtripPackets( List<UploadFileMessage> packets )
{
return packets.stream().map( packet -> {
PacketBuffer buffer = new PacketBuffer( Unpooled.directBuffer() );
packet.toBytes( buffer );
// We include things like file size in the packet, but not in the count, so grant a slightly larger threshold.
assertThat( "Packet is too large", buffer.writerIndex(), lessThanOrEqualTo( MAX_PACKET_SIZE + 128 ) );
if( (packet.flag & FLAG_LAST) == 0 )
{
int expectedSize = (packet.flag & FLAG_FIRST) != 0
? MAX_PACKET_SIZE - MAX_FILE_NAME * MAX_FILES
: MAX_PACKET_SIZE;
assertThat(
"Non-final packets should be efficiently packed", buffer.writerIndex(), greaterThanOrEqualTo( expectedSize )
);
}
UploadFileMessage result = new UploadFileMessage( buffer );
buffer.release();
assertEquals( 0, buffer.refCnt(), "Buffer should have no references" );
return result;
} ).collect( Collectors.toList() );
}
/**
* "Receive" our upload packets.
*
* @param packets The packets to receive. Note that this will clobber the {@link FileUpload}s in the first packet,
* so you may want to copy (or {@linkplain #roundtripPackets(List) roundtrip} first.
* @return The consumed file uploads.
*/
private static List<FileUpload> receive( List<UploadFileMessage> packets )
{
List<FileUpload> files = packets.get( 0 ).files;
for( int i = 0; i < packets.size(); i++ )
{
UploadFileMessage packet = packets.get( i );
boolean isFirst = i == 0;
boolean isLast = i == packets.size() - 1;
assertEquals( isFirst, (packet.flag & FLAG_FIRST) != 0, "FLAG_FIRST" );
assertEquals( isLast, (packet.flag & FLAG_LAST) != 0, "FLAG_LAST" );
for( FileSlice slice : packet.slices ) slice.apply( files );
}
return files;
}
@Provide
Arbitrary<FileUpload> fileUpload()
{
return Combinators.combine(
Arbitraries.oneOf( Arrays.asList(
// 1.16 doesn't correctly handle unicode file names. We'll be generous in our tests here.
Arbitraries.strings().ofMinLength( 1 ).ascii().ofMaxLength( MAX_FILE_NAME ),
Arbitraries.strings().ofMinLength( 1 ).ofMaxLength( MAX_FILE_NAME / 4 )
) ),
ArbitraryByteBuffer.bytes().ofMaxSize( MAX_SIZE )
).as( UploadFileMessageTest::file );
}
@Provide
Arbitrary<List<FileUpload>> fileUploads()
{
return fileUpload().list()
.ofMinSize( 1 ).ofMaxSize( MAX_FILES )
.filter( us -> us.stream().mapToInt( u -> u.getBytes().remaining() ).sum() <= MAX_SIZE );
}
private static FileUpload file( String name, ByteBuffer buffer )
{
byte[] checksum = FileUpload.getDigest( buffer );
if( checksum == null ) throw new IllegalStateException( "Failed to compute checksum" );
return new FileUpload( name, buffer, checksum );
}
public static Matcher<FileUpload> uploadEqual( FileUpload upload )
{
return allOf(
contramap( equalTo( upload.getName() ), "name", FileUpload::getName ),
contramap( equalTo( upload.getChecksum() ), "checksum", FileUpload::getChecksum ),
contramap( bufferEqual( upload.getBytes() ), "bytes", FileUpload::getBytes )
);
}
}

View File

@ -0,0 +1,196 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.support;
import net.jqwik.api.*;
import net.jqwik.api.arbitraries.SizableArbitrary;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.math.BigInteger;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.Random;
import java.util.Spliterators;
import java.util.function.Consumer;
import java.util.function.ToIntFunction;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
/**
* Generate arbitrary byte buffers with irrelevant (but random) contents.
* <p>
* This is more efficient than using {@link Arbitraries#bytes()} and {@link Arbitrary#array(Class)}, as it does not
* try to shrink the contents, only the size.
*/
public final class ArbitraryByteBuffer implements SizableArbitrary<ByteBuffer>
{
private static final ArbitraryByteBuffer DEFAULT = new ArbitraryByteBuffer( 0, null, null );
private int minSize = 0;
private final @Nullable Integer maxSize;
private final @Nullable RandomDistribution distribution;
private ArbitraryByteBuffer( int minSize, @Nullable Integer maxSize, @Nullable RandomDistribution distribution )
{
this.minSize = minSize;
this.maxSize = maxSize;
this.distribution = distribution;
}
public static ArbitraryByteBuffer bytes()
{
return DEFAULT;
}
@Nonnull
@Override
public SizableArbitrary<ByteBuffer> ofMinSize( int minSize )
{
return new ArbitraryByteBuffer( minSize, maxSize, distribution );
}
@Nonnull
@Override
public SizableArbitrary<ByteBuffer> ofMaxSize( int maxSize )
{
return new ArbitraryByteBuffer( minSize, maxSize, distribution );
}
@Nonnull
@Override
public SizableArbitrary<ByteBuffer> withSizeDistribution( @Nonnull RandomDistribution distribution )
{
return new ArbitraryByteBuffer( minSize, maxSize, distribution );
}
@Nonnull
@Override
public RandomGenerator<ByteBuffer> generator( int genSize )
{
BigInteger min = BigInteger.valueOf( minSize );
ToIntFunction<Random> generator;
if( distribution == null )
{
generator = sizeGeneratorWithCutoff( minSize, getMaxSize(), genSize );
}
else
{
RandomDistribution.RandomNumericGenerator gen = distribution.createGenerator( genSize, min, BigInteger.valueOf( getMaxSize() ), min );
generator = r -> gen.next( r ).intValueExact();
}
return r -> {
int size = generator.applyAsInt( r );
return new ShrinkableBuffer( allocateRandom( size, r ), minSize );
};
}
@Nonnull
@Override
public EdgeCases<ByteBuffer> edgeCases( int maxEdgeCases )
{
return EdgeCases.fromSuppliers( Arrays.asList(
() -> new ShrinkableBuffer( allocateRandom( minSize, new Random() ), minSize ),
() -> new ShrinkableBuffer( allocateRandom( getMaxSize(), new Random() ), minSize )
) );
}
private int getMaxSize()
{
return maxSize == null ? Math.max( minSize * 2, 255 ) : maxSize;
}
private static ToIntFunction<Random> sizeGeneratorWithCutoff( int minSize, int maxSize, int genSize )
{
// If we've a large range, we either pick between generating small (<10) or large lists.
int range = maxSize - minSize;
int offset = (int) Math.max( Math.round( Math.sqrt( genSize ) ), 10 );
int cutoff = range <= offset ? maxSize : Math.min( offset + minSize, maxSize );
if( cutoff >= maxSize ) return random -> nextInt( random, minSize, maxSize );
// Choose size below cutoff with probability of 0.1.
double maxSizeProbability = Math.min( 0.02, 1.0 / (genSize / 10.0) );
double cutoffProbability = 0.1;
return random -> {
if( random.nextDouble() <= maxSizeProbability )
{
return maxSize;
}
else if( random.nextDouble() <= cutoffProbability + maxSizeProbability )
{
return nextInt( random, cutoff + 1, maxSize );
}
else
{
return nextInt( random, minSize, cutoff );
}
};
}
private static int nextInt( Random random, int minSize, int maxSize )
{
return random.nextInt( maxSize - minSize + 1 ) + minSize;
}
private static ByteBuffer allocateRandom( int size, Random random )
{
ByteBuffer buffer = ByteBuffer.allocate( size );
for( int i = 0; i < size; i++ ) buffer.put( i, (byte) random.nextInt() );
return buffer.asReadOnlyBuffer();
}
private static final class ShrinkableBuffer implements Shrinkable<ByteBuffer>
{
private final ByteBuffer value;
private final int minSize;
private ShrinkableBuffer( ByteBuffer value, int minSize )
{
this.value = value;
this.minSize = minSize;
}
@Nonnull
@Override
public ByteBuffer value()
{
return value;
}
@Nonnull
@Override
public Stream<Shrinkable<ByteBuffer>> shrink()
{
return StreamSupport.stream( new Spliterators.AbstractSpliterator<Shrinkable<ByteBuffer>>( 3, 0 )
{
int size = value.remaining();
@Override
public boolean tryAdvance( Consumer<? super Shrinkable<ByteBuffer>> action )
{
if( size <= minSize ) return false;
int half = (size / 2) - (minSize / 2);
size = half == 0 ? minSize : size - half;
ByteBuffer slice = value.duplicate();
slice.limit( size );
action.accept( new ShrinkableBuffer( slice.slice(), minSize ) );
return true;
}
}, false );
}
@Nonnull
@Override
public ShrinkingDistance distance()
{
return ShrinkingDistance.of( value.remaining() - minSize );
}
}
}

View File

@ -0,0 +1,75 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.support;
import org.hamcrest.Description;
import org.hamcrest.Matcher;
import org.hamcrest.TypeSafeMatcher;
import java.nio.ByteBuffer;
public final class ByteBufferMatcher extends TypeSafeMatcher<ByteBuffer>
{
private final ByteBuffer expected;
private ByteBufferMatcher( ByteBuffer expected )
{
this.expected = expected;
}
@Override
protected boolean matchesSafely( ByteBuffer actual )
{
return expected.equals( actual );
}
@Override
public void describeTo( Description description )
{
description.appendValue( expected );
}
@Override
protected void describeMismatchSafely( ByteBuffer actual, Description mismatchDescription )
{
if( expected.remaining() != actual.remaining() )
{
mismatchDescription
.appendValue( actual ).appendText( " has " ).appendValue( actual.remaining() ).appendText( " bytes remaining" );
return;
}
int remaining = expected.remaining();
int expectedPos = expected.position();
int actualPos = actual.position();
for( int i = 0; i < remaining; i++ )
{
if( expected.get( expectedPos + i ) == actual.get( actualPos + i ) ) continue;
int offset = Math.max( i - 5, 0 );
int length = Math.min( i + 5, remaining - 1 ) - offset + 1;
byte[] expectedBytes = new byte[length];
expected.duplicate().position( expectedPos + offset );
expected.get( expectedBytes );
byte[] actualBytes = new byte[length];
actual.duplicate().position( actualPos + offset );
actual.get( actualBytes );
mismatchDescription
.appendText( "failed at " ).appendValue( i ).appendText( System.lineSeparator() )
.appendText( "expected " ).appendValue( expectedBytes ).appendText( System.lineSeparator() )
.appendText( "was " ).appendValue( actual );
return;
}
}
public static Matcher<ByteBuffer> bufferEqual( ByteBuffer buffer )
{
return new ByteBufferMatcher( buffer );
}
}

View File

@ -0,0 +1,32 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.support;
import org.hamcrest.Matcher;
import java.util.List;
import java.util.function.Function;
import java.util.stream.Collectors;
import static org.hamcrest.Matchers.contains;
public class CustomMatchers
{
/**
* Assert two lists are equal according to some matcher.
* <p>
* This method is simple, but helps avoid some issues with generics we'd see otherwise.
*
* @param items The items the matched list should be equal to.
* @param matcher Generate a matcher for a single item in the list.
* @param <T> The type to compare against.
* @return A matcher which compares against a list of items.
*/
public static <T> Matcher<Iterable<? extends T>> containsWith( List<T> items, Function<T, Matcher<? super T>> matcher )
{
return contains( items.stream().map( matcher ).collect( Collectors.toList() ) );
}
}

View File

@ -0,0 +1,40 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.support;
import com.google.auto.service.AutoService;
import dan200.computercraft.shared.computer.upload.FileUpload;
import net.jqwik.api.SampleReportingFormat;
import javax.annotation.Nonnull;
/**
* Custom jqwik formatters for some of our internal types.
*/
@AutoService( SampleReportingFormat.class )
public class CustomSampleUploadReporter implements SampleReportingFormat
{
@Override
public boolean appliesTo( @Nonnull Object value )
{
return value instanceof FileUpload;
}
@Nonnull
@Override
public Object report( @Nonnull Object value )
{
if( value instanceof FileUpload )
{
FileUpload upload = (FileUpload) value;
return String.format( "FileUpload(name=%s, contents=%s)", upload.getName(), upload.getBytes() );
}
else
{
throw new IllegalStateException( "Unexpected value " + value );
}
}
}

View File

@ -0,0 +1,25 @@
/*
* This file is part of ComputerCraft - http://www.computercraft.info
* Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
* Send enquiries to dratcliffe@gmail.com
*/
package dan200.computercraft.support;
import net.minecraft.entity.player.PlayerEntity;
import net.minecraft.inventory.container.Container;
import javax.annotation.Nonnull;
public class FakeContainer extends Container
{
public FakeContainer()
{
super( null, 0 );
}
@Override
public boolean stillValid( @Nonnull PlayerEntity player )
{
return true;
}
}