Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECR-1624 Messages payload length fix #285

Merged
merged 4 commits into from
Jun 8, 2018
Merged

ECR-1624 Messages payload length fix #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16db46f
ECR-1624 Messages payload length fix
Jun 7, 2018
492fbbf
ECR-1624 Checkstyle fix
Jun 8, 2018
5bf2e5f
Some improvements to MessageReader:
dmitry-timofeev Jun 8, 2018
c75cee0
Merge branch 'master' into ECR-1624
dmitry-timofeev Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions exonum-java-binding-core/src/main/java/com/exonum/binding/messages/BinaryMessageBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package com.exonum.binding.messages;

import static com.exonum.binding.messages.Message.BODY_LENGTH_OFFSET;
import static com.exonum.binding.messages.Message.BODY_OFFSET;
import static com.exonum.binding.messages.Message.MESSAGE_TYPE_OFFSET;
import static com.exonum.binding.messages.Message.NET_ID_OFFSET;
import static com.exonum.binding.messages.Message.PAYLOAD_LENGTH_OFFSET;
import static com.exonum.binding.messages.Message.SERVICE_ID_OFFSET;
import static com.exonum.binding.messages.Message.VERSION_OFFSET;
import static com.google.common.base.Preconditions.checkNotNull;
Expand Down Expand Up @@ -62,7 +62,7 @@ private void putHeader(ByteBuffer buffer) {
.put(VERSION_OFFSET, message.getVersion())
.putShort(SERVICE_ID_OFFSET, message.getServiceId())
.putShort(MESSAGE_TYPE_OFFSET, message.getMessageType())
.putInt(BODY_LENGTH_OFFSET, message.getBody().remaining());
.putInt(PAYLOAD_LENGTH_OFFSET, message.size());
}

private void putBody(ByteBuffer buffer) {
Expand Down
2 changes: 1 addition & 1 deletion exonum-java-binding-core/src/main/java/com/exonum/binding/messages/Message.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public interface Message {
int VERSION_OFFSET = 1;
int MESSAGE_TYPE_OFFSET = 2;
int SERVICE_ID_OFFSET = 4;
int BODY_LENGTH_OFFSET = 6;
int PAYLOAD_LENGTH_OFFSET = 6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little misleading name in the specification is one of the causes of this bug, I propose to use something more specific: MESSAGE_SIZE_OFFSET.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I agree that this naming is ambiguous, but on the other hand it is consistent with our documentation - https://exonum.com/doc/architecture/serialization/#message-serialization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, there will be surprises/inconsistencies. OK, let's keep it as is, but request the core to use clearer names:

https://jira.bf.local/browse/ECR-1647

int HEADER_SIZE = 10;
int BODY_OFFSET = HEADER_SIZE;
int SIGNATURE_SIZE = 64;
Expand Down
12 changes: 7 additions & 5 deletions exonum-java-binding-core/src/main/java/com/exonum/binding/messages/MessageReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ public static MessageReader wrap(ByteBuffer buffer) {

int bufferSize = buffer.limit();
checkArgument(MIN_MESSAGE_BUFFER_SIZE <= bufferSize,
"The buffer size (%s) is less than the minimal possible (%s)",
"The buffer size (%s) is less than the minimal possible message size (%s)",
bufferSize, MIN_MESSAGE_BUFFER_SIZE);
int expectedSize = Message.messageSize(reader.bodySize());
// Check the 'payload_length' field of the message matches the actual buffer size.
int expectedSize = reader.size();
checkArgument(bufferSize == expectedSize,
"The size of the buffer (%s) does not match expected (%s)", bufferSize, expectedSize);
"The size of the buffer (%s) does not match the expected size "
+ "specified in the message header (%s)", bufferSize, expectedSize);
return reader;
}

Expand Down Expand Up @@ -84,7 +86,7 @@ public ByteBuffer getBody() {
}

private int bodySize() {
return message.getInt(BODY_LENGTH_OFFSET);
return size() - HEADER_SIZE - SIGNATURE_SIZE;
}

/**
Expand All @@ -101,7 +103,7 @@ public ByteBuffer getSignature() {

@Override
public int size() {
return message.limit();
return message.getInt(PAYLOAD_LENGTH_OFFSET);
}

/**
Expand Down
74 changes: 54 additions & 20 deletions exonum-java-binding-core/src/test/java/com/exonum/binding/messages/MessageReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package com.exonum.binding.messages;

import static com.exonum.binding.messages.ByteBufferAllocator.allocateBuffer;
import static com.exonum.binding.messages.Message.BODY_LENGTH_OFFSET;
import static com.exonum.binding.messages.Message.BODY_OFFSET;
import static com.exonum.binding.messages.Message.MESSAGE_TYPE_OFFSET;
import static com.exonum.binding.messages.Message.NET_ID_OFFSET;
import static com.exonum.binding.messages.Message.PAYLOAD_LENGTH_OFFSET;
import static com.exonum.binding.messages.Message.SERVICE_ID_OFFSET;
import static com.exonum.binding.messages.Message.SIGNATURE_SIZE;
import static com.exonum.binding.messages.Message.VERSION_OFFSET;
Expand All @@ -29,7 +29,8 @@ public class MessageReaderTest {
public void wrapThrowsIfTooSmall() throws Exception {
ByteBuffer buf = allocateBuffer(2);

expectedException.expectMessage("The buffer size (2) is less than the minimal possible (74)");
expectedException.expectMessage("The buffer size (2) is less than the minimal possible "
+ "message size (74)");
expectedException.expect(IllegalArgumentException.class);
MessageReader.wrap(buf);
}
Expand All @@ -38,14 +39,25 @@ public void wrapThrowsIfTooSmall() throws Exception {
public void wrapThrowsIfTooSmall2() throws Exception {
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE - 1);

expectedException.expectMessage("The buffer size (73) is less than the minimal possible (74)");
expectedException.expectMessage("The buffer size (73) is less than the minimal possible "
+ "message size (74)");
expectedException.expect(IllegalArgumentException.class);
MessageReader.wrap(buf);
}

@Test
public void wrapThrowsIfTooSmallWithSetMessageSize() throws Exception {
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE - 1);

expectedException.expectMessage("The buffer size (73) is less than the minimal possible "
+ "message size (74)");
expectedException.expect(IllegalArgumentException.class);
MessageReader.wrap(buf);
}

@Test
public void wrapsMinimalMessage() throws Exception {
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE);
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE);

MessageReader m = MessageReader.wrap(buf);

Expand All @@ -54,7 +66,8 @@ public void wrapsMinimalMessage() throws Exception {

@Test
public void wrapsWhenLimitNotEqualCapacity() throws Exception {
ByteBuffer buf = allocateBuffer(2 * MIN_MESSAGE_BUFFER_SIZE);
ByteBuffer buf = allocateBuffer(2 * MIN_MESSAGE_BUFFER_SIZE)
.putInt(PAYLOAD_LENGTH_OFFSET, MIN_MESSAGE_BUFFER_SIZE);
buf.limit(MIN_MESSAGE_BUFFER_SIZE);

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -63,23 +76,37 @@ public void wrapsWhenLimitNotEqualCapacity() throws Exception {
}

@Test
public void wrapThrowsIfBodySizeFieldDoesNotMatchActual() throws Exception {
public void wrapThrowsIfMessageSizeFieldGreaterThanActual() throws Exception {
int bufferSize = MIN_MESSAGE_BUFFER_SIZE;
ByteBuffer buf = allocateBuffer(bufferSize);
int bodySize = 2048;
buf.putInt(BODY_LENGTH_OFFSET, bodySize);
int messageSize = 2048;
buf.putInt(PAYLOAD_LENGTH_OFFSET, messageSize);

int expectedBufferSize = Message.messageSize(bodySize);
expectedException.expectMessage("The size of the buffer (" + bufferSize
+ ") does not match expected (" + expectedBufferSize + ")");
+ ") does not match the expected size specified in the message header (" + messageSize
+ ")");
expectedException.expect(IllegalArgumentException.class);
MessageReader.wrap(buf);
}

@Test
public void wrapThrowsIfMessageSizeFieldLessThanActual() throws Exception {
int bufferSize = 2 * MIN_MESSAGE_BUFFER_SIZE;
ByteBuffer buf = allocateBuffer(bufferSize);
int messageSize = bufferSize - 1;
buf.putInt(PAYLOAD_LENGTH_OFFSET, messageSize);

expectedException.expectMessage("The size of the buffer (" + bufferSize
+ ") does not match the expected size specified in the message header (" + messageSize
+ ")");
expectedException.expect(IllegalArgumentException.class);
MessageReader.wrap(buf);
}

@Test
public void getNetworkId() throws Exception {
byte netId = 0x01;
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE)
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE)
.put(NET_ID_OFFSET, netId);

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -90,7 +117,7 @@ public void getNetworkId() throws Exception {
@Test
public void getVersion() throws Exception {
byte version = 0x02;
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE)
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE)
.put(VERSION_OFFSET, version);

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -101,7 +128,7 @@ public void getVersion() throws Exception {
@Test
public void getServiceId() throws Exception {
short serviceId = 0x0BCD;
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE)
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE)
.putShort(SERVICE_ID_OFFSET, serviceId);

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -112,7 +139,7 @@ public void getServiceId() throws Exception {
@Test
public void getMessageType() throws Exception {
short messageType = 0x0BCD;
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE)
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE)
.putShort(MESSAGE_TYPE_OFFSET, messageType);

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -122,7 +149,7 @@ public void getMessageType() throws Exception {

@Test
public void getBody_Empty() throws Exception {
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE);
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE);
boolean directBuffer = buf.isDirect();

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -136,8 +163,7 @@ public void getBody_Empty() throws Exception {
public void getBody_4Bytes() throws Exception {
int bodySize = Integer.BYTES;
int bodyValue = 0x12345678;
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE + bodySize)
.putInt(BODY_LENGTH_OFFSET, bodySize)
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE + bodySize)
.putInt(BODY_OFFSET, bodyValue);

MessageReader m = MessageReader.wrap(buf);
Expand All @@ -151,7 +177,7 @@ public void getBody_4Bytes() throws Exception {
@Test
public void getSignature() throws Exception {
byte[] signature = createPrefixed(bytes("Signature bytes"), SIGNATURE_SIZE);
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE);
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE);
buf.position(MIN_MESSAGE_BUFFER_SIZE - SIGNATURE_SIZE);
buf.put(signature);
buf.flip();
Expand All @@ -163,7 +189,7 @@ public void getSignature() throws Exception {

@Test
public void getMessage() throws Exception {
ByteBuffer buf = allocateBuffer(MIN_MESSAGE_BUFFER_SIZE)
ByteBuffer buf = allocateMessageBuffer(MIN_MESSAGE_BUFFER_SIZE)
.put(NET_ID_OFFSET, (byte) 0x02)
.put(VERSION_OFFSET, (byte) 0x01)
.putShort(MESSAGE_TYPE_OFFSET, (short) 0x0ABC);
Expand All @@ -177,10 +203,18 @@ public void getMessage() throws Exception {
@Test
public void size() throws Exception {
int bufferSize = MIN_MESSAGE_BUFFER_SIZE;
ByteBuffer buf = allocateBuffer(bufferSize);
ByteBuffer buf = allocateMessageBuffer(bufferSize);

MessageReader m = MessageReader.wrap(buf);

assertThat(m.size(), equalTo(bufferSize));
}

/**
* Allocates a byte buffer of the given size and sets its "payload_length" field.
*/
private static ByteBuffer allocateMessageBuffer(int bufferSize) {
return allocateBuffer(bufferSize)
.putInt(PAYLOAD_LENGTH_OFFSET, bufferSize);
}
}