From a7987585b824117704edf4d0fa2324bf18835129 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 28 May 2013 17:17:05 +0200 Subject: [PATCH] Catch out-of-bound reads and rethrow as ProtocolExceptions. --- .../java/com/google/bitcoin/core/Block.java | 16 ++- .../java/com/google/bitcoin/core/Message.java | 124 +++++++++++------- .../com/google/bitcoin/core/PeerAddress.java | 2 +- .../java/com/google/bitcoin/core/Ping.java | 2 +- .../bitcoin/core/ProtocolException.java | 2 +- .../google/bitcoin/core/TransactionInput.java | 2 +- .../bitcoin/core/TransactionOutput.java | 2 +- .../bitcoin/core/VerificationException.java | 4 + 8 files changed, 97 insertions(+), 57 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/core/Block.java b/core/src/main/java/com/google/bitcoin/core/Block.java index 896db5ba..24f6b957 100644 --- a/core/src/main/java/com/google/bitcoin/core/Block.java +++ b/core/src/main/java/com/google/bitcoin/core/Block.java @@ -148,7 +148,7 @@ public class Block extends Message { hash = null; } - private void parseHeader() { + private void parseHeader() throws ProtocolException { if (headerParsed) return; @@ -230,7 +230,7 @@ public class Block extends Message { /* * Block uses some special handling for lazy parsing and retention of cached bytes. Parsing and serializing the * block header and the transaction list are both non-trivial so there are good efficiency gains to be had by - * separating them. There are many cases where a user may need access to access or change one or the other but not both. + * separating them. There are many cases where a user may need to access or change one or the other but not both. * * With this in mind we ignore the inherited checkParse() and unCache() methods and implement a separate version * of them for both header and transactions. @@ -242,9 +242,15 @@ public class Block extends Message { private void maybeParseHeader() { if (headerParsed || bytes == null) return; - parseHeader(); - if (!(headerBytesValid || transactionBytesValid)) - bytes = null; + try { + parseHeader(); + if (!(headerBytesValid || transactionBytesValid)) + bytes = null; + } catch (ProtocolException e) { + throw new LazyParseException( + "ProtocolException caught during lazy parse. For safe access to fields call ensureParsed before attempting read or write access", + e); + } } private void maybeParseTransactions() { diff --git a/core/src/main/java/com/google/bitcoin/core/Message.java b/core/src/main/java/com/google/bitcoin/core/Message.java index 6d5f0185..cc88cfb9 100644 --- a/core/src/main/java/com/google/bitcoin/core/Message.java +++ b/core/src/main/java/com/google/bitcoin/core/Message.java @@ -402,74 +402,104 @@ public abstract class Message implements Serializable { return length; } - long readUint32() { - long u = Utils.readUint32(bytes, cursor); - cursor += 4; - return u; + long readUint32() throws ProtocolException { + try { + long u = Utils.readUint32(bytes, cursor); + cursor += 4; + return u; + } catch (ArrayIndexOutOfBoundsException e) { + throw new ProtocolException(e); + } } - Sha256Hash readHash() { - byte[] hash = new byte[32]; - System.arraycopy(bytes, cursor, hash, 0, 32); - // We have to flip it around, as it's been read off the wire in little endian. - // Not the most efficient way to do this but the clearest. - hash = Utils.reverseBytes(hash); - cursor += 32; - return new Sha256Hash(hash); + Sha256Hash readHash() throws ProtocolException { + try { + byte[] hash = new byte[32]; + System.arraycopy(bytes, cursor, hash, 0, 32); + // We have to flip it around, as it's been read off the wire in little endian. + // Not the most efficient way to do this but the clearest. + hash = Utils.reverseBytes(hash); + cursor += 32; + return new Sha256Hash(hash); + } catch (IndexOutOfBoundsException e) { + throw new ProtocolException(e); + } } - long readInt64() { - long u = Utils.readInt64(bytes, cursor); - cursor += 8; - return u; + long readInt64() throws ProtocolException { + try { + long u = Utils.readInt64(bytes, cursor); + cursor += 8; + return u; + } catch (ArrayIndexOutOfBoundsException e) { + throw new ProtocolException(e); + } } - BigInteger readUint64() { - // Java does not have an unsigned 64 bit type. So scrape it off the wire then flip. - byte[] valbytes = new byte[8]; - System.arraycopy(bytes, cursor, valbytes, 0, 8); - valbytes = Utils.reverseBytes(valbytes); - cursor += valbytes.length; - return new BigInteger(valbytes); + BigInteger readUint64() throws ProtocolException { + try { + // Java does not have an unsigned 64 bit type. So scrape it off the wire then flip. + byte[] valbytes = new byte[8]; + System.arraycopy(bytes, cursor, valbytes, 0, 8); + valbytes = Utils.reverseBytes(valbytes); + cursor += valbytes.length; + return new BigInteger(valbytes); + } catch (IndexOutOfBoundsException e) { + throw new ProtocolException(e); + } } - long readVarInt() { + long readVarInt() throws ProtocolException { return readVarInt(0); } - long readVarInt(int offset) { - VarInt varint = new VarInt(bytes, cursor + offset); - cursor += offset + varint.getOriginalSizeInBytes(); - return varint.value; + long readVarInt(int offset) throws ProtocolException { + try { + VarInt varint = new VarInt(bytes, cursor + offset); + cursor += offset + varint.getOriginalSizeInBytes(); + return varint.value; + } catch (ArrayIndexOutOfBoundsException e) { + throw new ProtocolException(e); + } } - byte[] readBytes(int length) { - byte[] b = new byte[length]; - System.arraycopy(bytes, cursor, b, 0, length); - cursor += length; - return b; + byte[] readBytes(int length) throws ProtocolException { + try { + byte[] b = new byte[length]; + System.arraycopy(bytes, cursor, b, 0, length); + cursor += length; + return b; + } catch (IndexOutOfBoundsException e) { + throw new ProtocolException(e); + } } - byte[] readByteArray() { + byte[] readByteArray() throws ProtocolException { long len = readVarInt(); return readBytes((int)len); } - String readStr() { - VarInt varInt = new VarInt(bytes, cursor); - if (varInt.value == 0) { - cursor += 1; - return ""; - } - cursor += varInt.getOriginalSizeInBytes(); - byte[] characters = new byte[(int) varInt.value]; - System.arraycopy(bytes, cursor, characters, 0, characters.length); - cursor += characters.length; + String readStr() throws ProtocolException { try { - return new String(characters, "UTF-8"); - } catch (UnsupportedEncodingException e) { - throw new RuntimeException(e); // Cannot happen, UTF-8 is always supported. + VarInt varInt = new VarInt(bytes, cursor); + if (varInt.value == 0) { + cursor += 1; + return ""; + } + cursor += varInt.getOriginalSizeInBytes(); + byte[] characters = new byte[(int) varInt.value]; + System.arraycopy(bytes, cursor, characters, 0, characters.length); + cursor += characters.length; + try { + return new String(characters, "UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); // Cannot happen, UTF-8 is always supported. + } + } catch (ArrayIndexOutOfBoundsException e) { + throw new ProtocolException(e); + } catch (IndexOutOfBoundsException e) { + throw new ProtocolException(e); } } diff --git a/core/src/main/java/com/google/bitcoin/core/PeerAddress.java b/core/src/main/java/com/google/bitcoin/core/PeerAddress.java index 66cce7ca..a0e43d5f 100644 --- a/core/src/main/java/com/google/bitcoin/core/PeerAddress.java +++ b/core/src/main/java/com/google/bitcoin/core/PeerAddress.java @@ -129,7 +129,7 @@ public class PeerAddress extends ChildMessage { } @Override - protected void parse() { + protected void parse() throws ProtocolException { // Format of a serialized address: // uint32 timestamp // uint64 services (flags determining what the node can do) diff --git a/core/src/main/java/com/google/bitcoin/core/Ping.java b/core/src/main/java/com/google/bitcoin/core/Ping.java index 87d3b24e..29761a0b 100644 --- a/core/src/main/java/com/google/bitcoin/core/Ping.java +++ b/core/src/main/java/com/google/bitcoin/core/Ping.java @@ -54,7 +54,7 @@ public class Ping extends Message { try { nonce = readInt64(); hasNonce = true; - } catch(ArrayIndexOutOfBoundsException e) { + } catch(ProtocolException e) { hasNonce = false; } length = hasNonce ? 8 : 0; diff --git a/core/src/main/java/com/google/bitcoin/core/ProtocolException.java b/core/src/main/java/com/google/bitcoin/core/ProtocolException.java index 498dd268..c5526e89 100644 --- a/core/src/main/java/com/google/bitcoin/core/ProtocolException.java +++ b/core/src/main/java/com/google/bitcoin/core/ProtocolException.java @@ -17,7 +17,7 @@ package com.google.bitcoin.core; @SuppressWarnings("serial") -public class ProtocolException extends Exception { +public class ProtocolException extends VerificationException { public ProtocolException(String msg) { super(msg); diff --git a/core/src/main/java/com/google/bitcoin/core/TransactionInput.java b/core/src/main/java/com/google/bitcoin/core/TransactionInput.java index ee1d3f9c..ac40ed9f 100644 --- a/core/src/main/java/com/google/bitcoin/core/TransactionInput.java +++ b/core/src/main/java/com/google/bitcoin/core/TransactionInput.java @@ -121,7 +121,7 @@ public class TransactionInput extends ChildMessage implements Serializable { this.parentTransaction = parentTransaction; } - protected void parseLite() { + protected void parseLite() throws ProtocolException { int curs = cursor; int scriptLen = (int) readVarInt(36); length = cursor - offset + scriptLen + 4; diff --git a/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java b/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java index 9d8bfe78..558a0bbd 100644 --- a/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java +++ b/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java @@ -121,7 +121,7 @@ public class TransactionOutput extends ChildMessage implements Serializable { return scriptPubKey; } - protected void parseLite() { + protected void parseLite() throws ProtocolException { // TODO: There is no reason to use BigInteger for values, they are always smaller than 21 million * COIN // The only reason to use BigInteger would be to properly read values from the reference implementation, however // the reference implementation uses signed 64-bit integers for its values as well (though it probably shouldn't) diff --git a/core/src/main/java/com/google/bitcoin/core/VerificationException.java b/core/src/main/java/com/google/bitcoin/core/VerificationException.java index 2697151a..967edfec 100644 --- a/core/src/main/java/com/google/bitcoin/core/VerificationException.java +++ b/core/src/main/java/com/google/bitcoin/core/VerificationException.java @@ -22,6 +22,10 @@ public class VerificationException extends Exception { super(msg); } + public VerificationException(Exception e) { + super(e); + } + public VerificationException(String msg, Throwable t) { super(msg, t); }