From 3e784c6305a04774334e447c678d6162fb308be6 Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 15 Nov 2017 23:29:34 +0100 Subject: [PATCH] Fixed handling of empty byte arrays Previously decoding of empty byte arrays caused an error because pack stream layer forced pack input to fill in an empty buffer. Later tried to fetch next chunk in such case and still fill the given buffer with nothing. This caused it to fail later on message boundary, when asserting that the whole chunk has been consumed. This commit fixes the problem by making pack input do nothing when asked to fill in an empty buffer. It also adds a branch in pack stream to always return a single empty byte array and not even attempt to read. --- .../internal/net/BufferingChunkedInput.java | 7 +- .../internal/packstream/PackStream.java | 10 ++- .../net/BufferingChunkedInputTest.java | 13 +++ .../internal/packstream/PackStreamTest.java | 28 ++++--- .../driver/v1/integration/ParametersIT.java | 81 ++++++++++++------- 5 files changed, 92 insertions(+), 47 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/BufferingChunkedInput.java b/driver/src/main/java/org/neo4j/driver/internal/net/BufferingChunkedInput.java index 7c62b8c898..8357b7d852 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/BufferingChunkedInput.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/BufferingChunkedInput.java @@ -142,8 +142,11 @@ public double readDouble() throws IOException @Override public PackInput readBytes( byte[] into, int offset, int toRead ) throws IOException { - ByteBuffer dst = ByteBuffer.wrap( into, offset, toRead ); - read( dst ); + if ( toRead != 0 ) + { + ByteBuffer dst = ByteBuffer.wrap( into, offset, toRead ); + read( dst ); + } return this; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java b/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java index e3f69fb591..98fa634443 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java +++ b/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java @@ -71,7 +71,8 @@ * DB11011011RESERVED * DC11011100STRUCT_8Structure (fewer than 28 fields) * DD11011101STRUCT_16Structure (fewer than 216 fields) - * DE11011110STRUCT_32Structure (fewer than 232 fields) + * DE11011110RESERVED + * DF11011111RESERVED * DF11011111RESERVED * E0..EF1110xxxxRESERVED * F0..FF1111xxxx-TINY_INTInteger -1 to -16 @@ -115,7 +116,7 @@ public class PackStream public static final byte RESERVED_DB = (byte) 0xDB; public static final byte STRUCT_8 = (byte) 0xDC; public static final byte STRUCT_16 = (byte) 0xDD; - public static final byte RESERVED_DE = (byte) 0xDE; // TODO STRUCT_32? or the class javadoc is wrong? + public static final byte RESERVED_DE = (byte) 0xDE; public static final byte RESERVED_DF = (byte) 0xDF; public static final byte RESERVED_E0 = (byte) 0xE0; public static final byte RESERVED_E1 = (byte) 0xE1; @@ -144,6 +145,7 @@ public class PackStream private static final long MINUS_2_TO_THE_31 = -2147483648L; private static final String EMPTY_STRING = ""; + private static final byte[] EMPTY_BYTE_ARRAY = new byte[0]; private static final Charset UTF_8 = Charset.forName( "UTF-8" ); private PackStream() {} @@ -614,6 +616,10 @@ private long unpackUINT32() throws IOException private byte[] unpackRawBytes(int size ) throws IOException { + if ( size == 0 ) + { + return EMPTY_BYTE_ARRAY; + } byte[] heapBuffer = new byte[size]; in.readBytes( heapBuffer, 0, heapBuffer.length ); return heapBuffer; diff --git a/driver/src/test/java/org/neo4j/driver/internal/net/BufferingChunkedInputTest.java b/driver/src/test/java/org/neo4j/driver/internal/net/BufferingChunkedInputTest.java index d0527444ea..2c8d32d68e 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/net/BufferingChunkedInputTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/net/BufferingChunkedInputTest.java @@ -41,6 +41,8 @@ import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class BufferingChunkedInputTest @@ -515,6 +517,17 @@ public void shouldKeepBufferCorrectWhenError() throws Throwable assertFalse( channel.isOpen() ); } + @Test + public void shouldNotReadFromChannelWhenEmptyByteBufferRequested() throws IOException + { + ReadableByteChannel channel = mock( ReadableByteChannel.class ); + BufferingChunkedInput input = new BufferingChunkedInput( channel ); + + input.readBytes( new byte[0], 0, 0 ); + + verify( channel, never() ).read( any( ByteBuffer.class ) ); + } + private ReadableByteChannel fillPacket( int size, int value ) { int[] ints = new int[size]; diff --git a/driver/src/test/java/org/neo4j/driver/internal/packstream/PackStreamTest.java b/driver/src/test/java/org/neo4j/driver/internal/packstream/PackStreamTest.java index e5d55cd211..8569cf7923 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/packstream/PackStreamTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/packstream/PackStreamTest.java @@ -326,26 +326,28 @@ public void testCanPackAndUnpackPowersOfTwoMinusABitAsDoubles() throws Throwable @Test public void testCanPackAndUnpackByteArrays() throws Throwable { - // Given Machine machine = new Machine( 17000000 ); + testByteArrayPackingAndUnpacking( machine, 0 ); for ( int i = 0; i < 24; i++ ) { - byte[] array = new byte[(int) Math.pow( 2, i )]; + testByteArrayPackingAndUnpacking( machine, (int) Math.pow( 2, i ) ); + } + } - // When - machine.reset(); - machine.packer().pack( array ); - machine.packer().flush(); + private void testByteArrayPackingAndUnpacking( Machine machine, int length ) throws Throwable + { + byte[] array = new byte[length]; - // Then - PackStream.Unpacker unpacker = newUnpacker( machine.output() ); - PackType packType = unpacker.peekNextType(); + machine.reset(); + machine.packer().pack( array ); + machine.packer().flush(); - // Then - assertThat( packType, equalTo( PackType.BYTES ) ); - assertArrayEquals( array, unpacker.unpackBytes() ); - } + PackStream.Unpacker unpacker = newUnpacker( machine.output() ); + PackType packType = unpacker.peekNextType(); + + assertThat( packType, equalTo( PackType.BYTES ) ); + assertArrayEquals( array, unpacker.unpackBytes() ); } @Test diff --git a/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java b/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java index e0babc32d7..a376d83c29 100644 --- a/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java +++ b/driver/src/test/java/org/neo4j/driver/v1/integration/ParametersIT.java @@ -22,6 +22,8 @@ import org.junit.Test; import org.junit.rules.ExpectedException; +import java.util.concurrent.ThreadLocalRandom; + import org.neo4j.driver.internal.util.ServerVersion; import org.neo4j.driver.v1.Record; import org.neo4j.driver.v1.StatementResult; @@ -150,30 +152,17 @@ public void shouldBeAbleToSetAndReturnDoubleProperty() } } - private boolean supportsBytes() - { - String versionString = session.run( "RETURN 1" ).consume().server().version(); - return version( versionString ).greaterThanOrEqual( ServerVersion.v3_2_0 ); - } - @Test public void shouldBeAbleToSetAndReturnBytesProperty() { assumeTrue( supportsBytes() ); - // Given - byte[] byteArray = "hello, world".getBytes(); - - // When - StatementResult result = session.run( - "CREATE (a {value:{value}}) RETURN a.value", parameters( "value", byteArray ) ); - - // Then - for ( Record record : result.list() ) + testBytesProperty( new byte[0] ); + for ( int i = 0; i < 16; i++ ) { - Value value = record.get( "a.value" ); - assertThat( value.hasType( session.typeSystem().BYTES() ), equalTo( true ) ); - assertThat( value.asByteArray(), equalTo( byteArray ) ); + int length = (int) Math.pow( 2, i ); + testBytesProperty( randomByteArray( length ) ); + testBytesProperty( randomByteArray( length - 1 ) ); } } @@ -202,18 +191,10 @@ public void shouldThrowExceptionWhenServerDoesNotSupportBytes() @Test public void shouldBeAbleToSetAndReturnStringProperty() { - // When - StatementResult result = session.run( - "CREATE (a {value:{value}}) RETURN a.value", parameters( "value", "Mjölnir" ) ); - - // Then - for ( Record record : result.list() ) - { - Value value = record.get( "a.value" ); - assertThat( value.hasType( session.typeSystem().STRING() ), equalTo( true ) ); - assertThat( value.asString(), equalTo( "Mjölnir" ) ); - } - + testStringProperty( "" ); + testStringProperty( "π≈3.14" ); + testStringProperty( "Mjölnir" ); + testStringProperty( "*** Hello World! ***" ); } @Test @@ -460,4 +441,44 @@ public void shouldNotBePossibleToUsePathAsParameter() // WHEN session.run( "RETURN {a}", parameters( "a", path ) ); } + + private void testBytesProperty( byte[] array ) + { + assumeTrue( supportsBytes() ); + + StatementResult result = session.run( + "CREATE (a {value:{value}}) RETURN a.value", parameters( "value", array ) ); + + for ( Record record : result.list() ) + { + Value value = record.get( "a.value" ); + assertThat( value.hasType( session.typeSystem().BYTES() ), equalTo( true ) ); + assertThat( value.asByteArray(), equalTo( array ) ); + } + } + + private void testStringProperty( String string ) + { + StatementResult result = session.run( + "CREATE (a {value:{value}}) RETURN a.value", parameters( "value", string ) ); + + for ( Record record : result.list() ) + { + Value value = record.get( "a.value" ); + assertThat( value.hasType( session.typeSystem().STRING() ), equalTo( true ) ); + assertThat( value.asString(), equalTo( string ) ); + } + } + + private boolean supportsBytes() + { + return version( session.driver() ).greaterThanOrEqual( ServerVersion.v3_2_0 ); + } + + private static byte[] randomByteArray( int length ) + { + byte[] result = new byte[length]; + ThreadLocalRandom.current().nextBytes( result ); + return result; + } }