From 026f1902a72652a5590d294c075d6187110d97f3 Mon Sep 17 00:00:00 2001 From: Ilan Olkies Date: Wed, 22 Nov 2023 11:54:44 -0300 Subject: [PATCH] Make byteArrayToLong negative safe --- .../main/java/org/ethereum/util/ByteUtil.java | 22 +++++------ .../main/java/org/ethereum/vm/GasCost.java | 9 ++--- .../java/org/ethereum/util/ByteUtilTest.java | 18 ++++++++- .../java/org/ethereum/vm/GasCostTest.java | 37 ++++++++++++++++++- .../StateTests/stSystemOperationsTest.json | 2 +- 5 files changed, 66 insertions(+), 22 deletions(-) diff --git a/rskj-core/src/main/java/org/ethereum/util/ByteUtil.java b/rskj-core/src/main/java/org/ethereum/util/ByteUtil.java index f90b24ebe4d..ad232c6867b 100644 --- a/rskj-core/src/main/java/org/ethereum/util/ByteUtil.java +++ b/rskj-core/src/main/java/org/ethereum/util/ByteUtil.java @@ -337,7 +337,7 @@ public static int byteArrayToInt(byte[] b) { /** * Cast from byte[] to long * Limited to Long.MAX_VALUE: 2^63-1 (8 bytes) - * Will throw IlegalArgumentException if you pass + * Will throw RuntimeException if you pass * a byte array with more than 8 bytes. * * @param b array contains the values @@ -345,21 +345,19 @@ public static int byteArrayToInt(byte[] b) { */ public static long byteArrayToLong(byte[] b) { if (b == null || b.length == 0) { - return 0; - } - // avoids overflows in the result - if (b.length > 8) { - throw new IllegalArgumentException("byte array can't have more than 8 bytes if it is to be cast to long"); + return 0; } - long result = 0; - for (int i = 0; i < b.length; i++) { - result <<= 8; - result |= (b[i] & 0xFF); + + BigInteger bigInteger = new BigInteger(1, b); + + if (bigInteger.compareTo(BigInteger.valueOf(Long.MAX_VALUE)) > 0) { + throw new RuntimeException("Cannot represent Integer " + bigInteger.toString(16)); } - return result; - } + return bigInteger.longValue(); + } + /** * Turn nibbles to a pretty looking output string * diff --git a/rskj-core/src/main/java/org/ethereum/vm/GasCost.java b/rskj-core/src/main/java/org/ethereum/vm/GasCost.java index 7501cf2cac2..b4d3b53923b 100644 --- a/rskj-core/src/main/java/org/ethereum/vm/GasCost.java +++ b/rskj-core/src/main/java/org/ethereum/vm/GasCost.java @@ -135,14 +135,11 @@ private GasCost() { } * is negative. */ public static long toGas(byte[] bytes) throws InvalidGasException { - if (bytes.length > 8) { + if (bytes.length > 8 || (bytes.length == 8 && (bytes[0] & 0xff) >= 0x80)) { return Long.MAX_VALUE; } - long result = ByteUtil.byteArrayToLong(bytes); - if (result < 0) { - throw new InvalidGasException(bytes); - } - return result; + + return ByteUtil.byteArrayToLong(bytes); } /** diff --git a/rskj-core/src/test/java/org/ethereum/util/ByteUtilTest.java b/rskj-core/src/test/java/org/ethereum/util/ByteUtilTest.java index af8bcfb6e3d..9d83858fa53 100644 --- a/rskj-core/src/test/java/org/ethereum/util/ByteUtilTest.java +++ b/rskj-core/src/test/java/org/ethereum/util/ByteUtilTest.java @@ -97,13 +97,29 @@ void testByteArrayToLong() { @Test void testByteArrayToLongThrowsWhenOverflow() { - Assertions.assertThrows(IllegalArgumentException.class, () -> ByteUtil.byteArrayToLong(new byte[]{ + Assertions.assertThrows(RuntimeException.class, () -> ByteUtil.byteArrayToLong(new byte[]{ (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)123, } )); } + @Test + void testByteArrayToLongNegative() { + Assertions.assertThrows(RuntimeException.class, () -> ByteUtil.byteArrayToLong(new byte[]{ + (byte)-1, (byte)255, (byte)255, (byte)255, + (byte)255, (byte)255, (byte)255, (byte)255 + })); + } + + @Test + void testByteArrayToLongNegative2() { + Assertions.assertThrows(RuntimeException.class, () -> ByteUtil.byteArrayToLong(new byte[]{ + (byte) 0xa4, (byte) 0xbe, (byte) 0xfa, (byte) 0xd1, + (byte) 0x41, (byte) 0xd5, (byte) 0x1c, (byte) 0x4f + })); + } + @Test void testByteArrayToInt() { Assertions.assertEquals(0, ByteUtil.byteArrayToInt(null)); diff --git a/rskj-core/src/test/java/org/ethereum/vm/GasCostTest.java b/rskj-core/src/test/java/org/ethereum/vm/GasCostTest.java index afd5d9228fa..920d5e1d9e2 100644 --- a/rskj-core/src/test/java/org/ethereum/vm/GasCostTest.java +++ b/rskj-core/src/test/java/org/ethereum/vm/GasCostTest.java @@ -53,14 +53,47 @@ void toGasOverflowsSlightly() { Assertions.assertEquals(Long.MAX_VALUE, GasCost.toGas(bytes)); } + @Test + void toGasInRange() throws GasCost.InvalidGasException { + Assertions.assertEquals(0L, GasCost.toGas(new byte[]{ + (byte)0 + })); + + + Assertions.assertEquals(0x000000000a141e28L, GasCost.toGas(new byte[]{ + (byte)10, (byte)20, (byte)30, (byte)40, + })); + + Assertions.assertEquals(0x7ffffffffffffffeL, GasCost.toGas(new byte[]{ + (byte)0x7f, (byte)255, (byte)255, (byte)255, + (byte)255, (byte)255, (byte)255, (byte)254, + })); + + Assertions.assertEquals(Long.MAX_VALUE, GasCost.toGas(new byte[]{ + (byte)0x7f, (byte)255, (byte)255, (byte)255, + (byte)255, (byte)255, (byte)255, (byte)255, + })); + } + + @Test + void toGasIsAlwaysPositive() throws GasCost.InvalidGasException { + Assertions.assertEquals(255, GasCost.toGas(new byte[]{ + (byte) (-1 & 0xff), + })); + + Assertions.assertEquals(Long.MAX_VALUE, GasCost.toGas(new byte[]{ + (byte) (-1 & 0xff), (byte)255, (byte)255, (byte)255, + (byte)255, (byte)255, (byte)255, (byte)255, + })); + } @Test - void toGasGivesNegativeValue() throws GasCost.InvalidGasException { + void toGasGivesMaxIntegerOnOverflow() throws GasCost.InvalidGasException { byte[] negativeBytes = new byte[]{ (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, (byte)255, }; - Assertions.assertThrows(GasCost.InvalidGasException.class, () -> GasCost.toGas(negativeBytes)); + Assertions.assertEquals(Long.MAX_VALUE, GasCost.toGas(negativeBytes)); } @Test diff --git a/rskj-core/src/test/resources/json/StateTests/stSystemOperationsTest.json b/rskj-core/src/test/resources/json/StateTests/stSystemOperationsTest.json index 51ab9780e31..a4a116f2f7c 100644 --- a/rskj-core/src/test/resources/json/StateTests/stSystemOperationsTest.json +++ b/rskj-core/src/test/resources/json/StateTests/stSystemOperationsTest.json @@ -7874,7 +7874,7 @@ "currentDifficulty" : "0x7fb7d889155ce8c6", "currentGasLimit" : "0x58272e28", "currentNumber" : "0xa7d8c0", - "currentTimestamp" : "0xa4befad141d51c4f", + "currentTimestamp" : "0x00", "previousHash" : "d30f77155de00f207ad60109897e790f73e9f3431be25717bf3651d91949f041" }, "logs" : [