Fixes for message length encoding

Author: mikera

convex-core/src/main/java/convex/core/data/ABlob.java

@@ -295,6 +295,7 @@ public final int encode(byte[] bs, int pos) {
 	public int hashCode() {
 		// note: We use a salted hash of the last bytes for blobs. 
 		// SECURITY: This is decent for small blobs, DoS risk for user generated large blobs. Be careful putting large keys in Java HashMaps.....
+		// TODO: consider salted psuedorandom selection of bytes to include in hash?
 		return Bits.hash32(longValue());
 	}
 

convex-core/src/main/java/convex/core/data/Format.java

@@ -103,9 +103,7 @@ public class Format {
 	 * @return Length of VLQ encoding in bytes
 	 */
 	public static int getVLQLongLength(long x) {
-		if ((x < 64) && (x >= -64)) {
-			return 1;
-		}
+		if ((x<64)&&(x>=-64)) return 1;
 		int bitLength = Utils.bitLength(x);
 		int blen = (bitLength + 6) / 7;
 		return blen;
@@ -114,41 +112,39 @@ public static int getVLQLongLength(long x) {
 	/**
 	 * Gets the length in bytes of VLQ count encoding for the given long value
 	 * @param x Long value to encode
-	 * @return Length of VLQ encoding
+	 * @return Length of VLQ encoding, or 0 if value is negative (overflow case)
 	 */
 	public static int getVLQCountLength(long x) {
-		if (x<0) throw new IllegalArgumentException("Negative Count");
-		if (x < 128) {
-			return 1;
-		}
+		if (x<0) return 0;
+		if (x<128) return 1;
 		int bitLength = Utils.bitLength(x)-1; // high zero not required
 		int blen = (bitLength + 6) / 7;
 		return blen;
 	}
 
 	/**
-	 * Puts a VLQ encoded long into the specified bytebuffer (with no tag)
+	 * Puts a VLQ encoded count into the specified bytebuffer (with no tag)
 	 * 
 	 * Format: 
 	 * <ul>
 	 * <li>MSB of each byte 0=last octet, 1=more octets</li>
 	 * <li>Following MSB, 7 bits of integer representation for each octet</li>
-	 * <li>Second highest bit of first byte is interpreted as the sign</li> 
 	 * </ul>
 	 * @param bb ByteBuffer to write to
 	 * @param x Value to VLQ encode
 	 * @return Updated ByteBuffer
 	 */
-	public static ByteBuffer writeVLQLong(ByteBuffer bb, long x) {
-		if ((x < 64) && (x >= -64)) {
-			// single byte, cleared high bit
-			byte single = (byte) (x & 0x7F);
+	public static ByteBuffer writeVLQCount(ByteBuffer bb, long x) {
+		if (x<128) {
+			if (x<0) throw new IllegalArgumentException("Negative count!");
+			// single byte
+			byte single = (byte) (x);
 			return bb.put(single);
 		}
-		int bitLength = Utils.bitLength(x);
-		int blen = (bitLength + 6) / 7;
+		int bitLength = 64-Bits.leadingZeros(x);
+		int blen = (bitLength + 6) / 7; // 8 bits overflows to 2 bytes etc.
 		for (int i = blen - 1; i >= 1; i--) {
-			byte single = (byte) (0x80 | (x >> (7 * i))); // 7 bits with high bit set
+			byte single = (byte) (0x80 | (x >>> (7 * i))); // 7 bits
 			bb = bb.put(single);
 		}
 		byte end = (byte) (x & 0x7F); // last 7 bits of long, high bit zero
@@ -310,7 +306,7 @@ public static long readVLQCount(AArrayBlob blob, int pos) throws BadFormatExcept
 
 	/**
 	 * Peeks for a VLQ encoded message length at the start of a ByteBuffer, which
-	 * must contain at least 1 byte, maximum 2.
+	 * must contain at least 1 byte
 	 * 
 	 * Does not move the buffer position.
 	 * 
@@ -323,7 +319,7 @@ public static int peekMessageLength(ByteBuffer bb) throws BadFormatException {
 		int remaining=bb.limit();
 		if (remaining==0) return -1;
 
-		int len = bb.get(0);
+		long len = bb.get(0);
 
 		// Quick check for 1 byte message length
 		if ((len & 0x80) == 0) {
@@ -333,25 +329,27 @@ public static int peekMessageLength(ByteBuffer bb) throws BadFormatException {
 						"Format.peekMessageLength: Zero message length:" + Utils.readBufferData(bb));
 			}
 
-			// 1 byte header (without high bit set)
-			return len & 0x3F;
+			// 1 byte length (without high bit set)
+			return (int)(len & 0x7F);
 		}
 
-		// Clear high bit
+		// Clear high bits
 		len &=0x7f;
 
 		if (len==0) throw new BadFormatException("Format.peekMessageLength: Excess leading zeros");
 
 		for (int i=1; i<Format.MAX_VLQ_COUNT_LENGTH; i++) {
 			if (i>=remaining) return -1; // we are expecting more bytes, but none available yet....
 			int lsb = bb.get(i);
-			len = (len << 7) + (lsb&0x7f);
+			len = (len << 7) | (lsb&0x7f);
 			if ((lsb & 0x80) == 0) {
-				return len;
+				break;
 			}
 		}
 
-		throw new BadFormatException("Format.peekMessageLength: Too many bytes in length encoding");
+		int result=(int)len;
+		if (result!=len) throw new BadFormatException("Format.peekMessageLength: Too many bytes in length encoding");
+		return result;
 	}
 
 	/**
@@ -362,7 +360,7 @@ public static int peekMessageLength(ByteBuffer bb) throws BadFormatException {
 	 * @return The ByteBuffer after writing the message length
 	 */
 	public static ByteBuffer writeMessageLength(ByteBuffer bb, int len) {
-		return writeVLQLong(bb, len);
+		return writeVLQCount(bb, len);
 	}
 
 	/**
@@ -448,11 +446,6 @@ public static AString readUTF8String(Blob blob, int pos, int len) throws BadForm
 		AString s = Strings.create(blob.slice(pos,pos+len));
 		return s;
 	}
-
-	public static ByteBuffer writeLength(ByteBuffer bb, int i) {
-		bb = writeVLQLong(bb, i);
-		return bb;
-	}
 
 	/**
 	 * Reads a Ref or embedded Cell value from a Blob

convex-core/src/main/java/convex/core/data/MapLeaf.java

@@ -340,7 +340,7 @@ public int estimatedEncodingSize() {
 	 * @throws BadFormatException If encoding is invalid
 	 */
 	public static <K extends ACell, V extends ACell> MapLeaf<K, V> read(Blob b, int pos, long count) throws BadFormatException {
-		int epos=pos+2; // Note: Tag byte plus VLC Count length which is always 1
+		int epos=pos+2; // Note: Tag byte plus VLQ Count length which is always 1
 
 		@SuppressWarnings("unchecked")
 		MapEntry<K, V>[] items = (MapEntry<K, V>[]) new MapEntry[(int) count];

convex-core/src/main/java/convex/core/data/Maps.java

@@ -139,7 +139,10 @@ public static <K extends ACell, V extends ACell, R extends AMap<K, V>> R coerce(
 	 * @throws BadFormatException If encoding is invalid
 	 */
 	public static <K extends ACell, V extends ACell> AHashMap<K, V> read(Blob b, int pos) throws BadFormatException {
+		// A hashmap always starts with a VLQ count after the tag
+		// We use this to distinguish the type of Map cell
 		long count = Format.readVLQCount(b,pos+1);
+		
 		if (count==0) return empty();
 		if (count <= MapLeaf.MAX_ENTRIES) {
 			if (count < 0) throw new BadFormatException("Overflowed count of map elements!");

convex-core/src/main/java/convex/core/data/SetLeaf.java

@@ -220,7 +220,7 @@ public int estimatedEncodingSize() {
 	 */
 
 	public static <V extends ACell> SetLeaf<V> read(Blob b, int pos, long count) throws BadFormatException {
-		int headerLen=1+1; // tag plus VLC Count length which is always 1
+		int headerLen=1+1; // tag plus VLQ Count length which is always 1
 
 		int epos=pos+headerLen;
 		if (count == 0) return Sets.empty();

convex-core/src/main/java/convex/core/util/Bits.java

@@ -42,7 +42,7 @@ public static int positionForDigit(int digit, short mask) {
 	 * @return Number of leading zeros (0-32)
 	 */
 	public static int leadingZeros(int x) {
-		if (x == 0) return 32;
+		if (x == 0) return 32; // fast path if all zeros
 		int result = 0;
 		if ((x & 0xFFFF0000) == 0) {
 			result += 16;
@@ -66,9 +66,7 @@ public static int leadingZeros(int x) {
 		}
 		if ((x & 0x2) == 0) {
 			result += 1;
-		} else {
-			x >>>= 1;
-		}
+		} 
 		return result;
 	}
 
@@ -78,10 +76,9 @@ public static int leadingZeros(int x) {
 	 * @return Number of leading zeros (0-64)
 	 */
 	public static int leadingZeros(long x) {
-		int highWord = (int) (x >>> 32); // high 4 bytes, unsigned
-		if (highWord != 0) return leadingZeros(highWord);
-		int lowWord = (int) (x);
-		return 32 + leadingZeros(lowWord);
+		int z=leadingZeros((int) (x >>> 32)); // get leading zeros from high word
+		if (z<32) return z;
+		return z + leadingZeros((int) x); // add leading zeros from low word
 	}
 
 	/**
@@ -113,7 +110,7 @@ public static int leadingOnes(long value) {
 	 * A long salt value used for internal hashing. 
 	 * 
 	 * We use a local, secure random number to minimise chance of attacker engineering hash collisions
-	 * also minimise risk of multiple peers suffering such attacks at the same time
+	 * also minimise risk of multiple peers suffering hashing collisions at the same time
 	 */
 	private static final long SALT=new SecureRandom().nextLong();
 

convex-core/src/test/java/convex/core/data/EncodingTest.java

@@ -56,7 +56,8 @@ public class EncodingTest {
 		assertEquals(2,Format.getVLQCountLength(0x80)); // roll over at 128
 		assertEquals(Format.MAX_VLQ_COUNT_LENGTH,Format.getVLQCountLength(Long.MAX_VALUE));
 
-		assertThrows(IllegalArgumentException.class, ()->Format.getVLQCountLength(-10));
+		// technically an overflow
+		assertEquals(0,Format.getVLQCountLength(-10));
 	}
 
 	@Test public void testVLCCount() throws BadFormatException {

convex-core/src/test/java/convex/core/data/FormatTest.java

@@ -4,6 +4,8 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.fail;
 
+import java.nio.ByteBuffer;
+
 import org.junit.jupiter.api.Test;
 
 import convex.core.exceptions.BadFormatException;
@@ -36,6 +38,18 @@ private void assertBadVLCEncoding(String hex) {
 			if (Format.getVLQLongLength(val)!=b.count()) throw new BadFormatException("Wrong length");
 		});
 	}
+	
+	@Test public void testBigCount() throws BadFormatException {
+		int c = Integer.MAX_VALUE;
+		int n=Format.getVLQCountLength(c);
+		assertEquals(5,n);
+		ByteBuffer bb=ByteBuffer.allocate(n);
+		Format.writeVLQCount(bb, c);
+		bb.flip();
+		assertEquals(c,Format.peekMessageLength(bb));
+		Blob b=Blob.fromByteBuffer(bb);
+		assertEquals(n,b.count());
+	}
 
 	private void checkVLCEncoding(String hex, long a) {
 		byte[] bs=new byte[12];

convex-core/src/test/java/convex/core/data/GenTestMessages.java

@@ -15,10 +15,12 @@
 public class GenTestMessages {
 
 	@Property
-	public void messageLengthVLC(Long a) throws BadFormatException {
-		ByteBuffer bb = ByteBuffer.allocate(Format.MAX_VLQ_LONG_LENGTH);
-		Format.writeVLQLong(bb, a);
+	public void messageLengthVLQ(Integer a) throws BadFormatException {
+		if (a<1) return;
+		ByteBuffer bb = ByteBuffer.allocate(5); // sufficient for 32 bits
+		Format.writeVLQCount(bb, a);
 		bb.flip();
-		assertEquals(bb.remaining(), Format.getVLQLongLength(a));
+		assertEquals(a,Format.peekMessageLength(bb));
+		assertEquals(bb.remaining(), Format.getVLQCountLength(a));
 	}
 }

convex-peer/src/main/java/convex/net/MessageReceiver.java

@@ -97,7 +97,7 @@ public synchronized int receiveFromChannel(ReadableByteChannel chan) throws BadF
 			int len = Format.peekMessageLength(buffer);
 			if (len<0) return numRead; // Not enough bytes for a message length yet
 
-			int lengthLength = Format.getVLQLongLength(len);
+			int lengthLength = Format.getVLQCountLength(len);
 			int totalFrameSize=lengthLength + len;
 
 			if (totalFrameSize>buffer.capacity()) {