grpc
/
grpc-java
mirror of https://github.com/grpc/grpc-java.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

services: fix binary logging empty metadata NPE, add null checks (#4213) 

Fix NPE and refactor tests to make it more obvious whether tests
assume empty or non empty metadata.

Add null checks to applicable places in binary log.
This commit is contained in:
zpencer 2018-03-14 11:29:49 -07:00 committed by GitHub
parent 2a778f126a
commit 9224d2ab8f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 48 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
services/src/main/java/io/grpc/services/BinaryLog.java
View File

@ -81,6 +81,9 @@ final class BinaryLog {
*/ */
public void logSendInitialMetadata( public void logSendInitialMetadata(
Metadata metadata, boolean isServer, byte[] callId, SocketAddress peerSocket) { Metadata metadata, boolean isServer, byte[] callId, SocketAddress peerSocket) {
Preconditions.checkNotNull(metadata);
// TODO(zpencer): peerSocket may go away, because at this time we have not selected a peer yet
Preconditions.checkNotNull(peerSocket);
GrpcLogEntry entry = GrpcLogEntry GrpcLogEntry entry = GrpcLogEntry
.newBuilder() .newBuilder()
.setType(Type.SEND_INITIAL_METADATA) .setType(Type.SEND_INITIAL_METADATA)
@ -98,6 +101,8 @@ final class BinaryLog {
*/ */
public void logRecvInitialMetadata( public void logRecvInitialMetadata(
Metadata metadata, boolean isServer, byte[] callId, SocketAddress peerSocket) { Metadata metadata, boolean isServer, byte[] callId, SocketAddress peerSocket) {
Preconditions.checkNotNull(metadata);
Preconditions.checkNotNull(peerSocket);
GrpcLogEntry entry = GrpcLogEntry GrpcLogEntry entry = GrpcLogEntry
.newBuilder() .newBuilder()
.setType(Type.RECV_INITIAL_METADATA) .setType(Type.RECV_INITIAL_METADATA)
@ -114,6 +119,7 @@ final class BinaryLog {
* as determined by the binary logging configuration. * as determined by the binary logging configuration.
*/ */
public void logTrailingMetadata(Metadata metadata, boolean isServer, byte[] callId) { public void logTrailingMetadata(Metadata metadata, boolean isServer, byte[] callId) {
Preconditions.checkNotNull(metadata);
GrpcLogEntry entry = GrpcLogEntry GrpcLogEntry entry = GrpcLogEntry
.newBuilder() .newBuilder()
.setType(isServer ? Type.SEND_TRAILING_METADATA : Type.RECV_TRAILING_METADATA) .setType(isServer ? Type.SEND_TRAILING_METADATA : Type.RECV_TRAILING_METADATA)
@ -131,6 +137,8 @@ final class BinaryLog {
*/ */
public void logOutboundMessage( public void logOutboundMessage(
ByteBuffer message, boolean compressed, boolean isServer, byte[] callId) { ByteBuffer message, boolean compressed, boolean isServer, byte[] callId) {
Preconditions.checkNotNull(message);
Preconditions.checkNotNull(callId);
GrpcLogEntry entry = GrpcLogEntry GrpcLogEntry entry = GrpcLogEntry
.newBuilder() .newBuilder()
.setType(Type.SEND_MESSAGE) .setType(Type.SEND_MESSAGE)
@ -148,6 +156,8 @@ final class BinaryLog {
*/ */
public void logInboundMessage( public void logInboundMessage(
ByteBuffer message, boolean compressed, boolean isServer, byte[] callId) { ByteBuffer message, boolean compressed, boolean isServer, byte[] callId) {
Preconditions.checkNotNull(message);
Preconditions.checkNotNull(callId);
GrpcLogEntry entry = GrpcLogEntry GrpcLogEntry entry = GrpcLogEntry
.newBuilder() .newBuilder()
.setType(Type.RECV_MESSAGE) .setType(Type.RECV_MESSAGE)
@ -370,6 +380,7 @@ final class BinaryLog {
*/ */
// TODO(zpencer): verify int64 representation with other gRPC languages // TODO(zpencer): verify int64 representation with other gRPC languages
static Uint128 callIdToProto(byte[] bytes) { static Uint128 callIdToProto(byte[] bytes) {
Preconditions.checkNotNull(bytes);
Preconditions.checkArgument( Preconditions.checkArgument(
bytes.length == 16, bytes.length == 16,
String.format("can only convert from 16 byte input, actual length = %d", bytes.length)); String.format("can only convert from 16 byte input, actual length = %d", bytes.length));
@ -382,6 +393,7 @@ final class BinaryLog {
@VisibleForTesting @VisibleForTesting
// TODO(zpencer): the binlog design does not specify how to actually express the peer bytes // TODO(zpencer): the binlog design does not specify how to actually express the peer bytes
static Peer socketToProto(SocketAddress address) { static Peer socketToProto(SocketAddress address) {
Preconditions.checkNotNull(address);
PeerType peerType = PeerType.UNKNOWN_PEERTYPE; PeerType peerType = PeerType.UNKNOWN_PEERTYPE;
byte[] peerAddress = null; byte[] peerAddress = null;
@ -414,12 +426,13 @@ final class BinaryLog {
@VisibleForTesting @VisibleForTesting
static io.grpc.binarylog.Metadata metadataToProto(Metadata metadata, int maxHeaderBytes) { static io.grpc.binarylog.Metadata metadataToProto(Metadata metadata, int maxHeaderBytes) {
Preconditions.checkNotNull(metadata);
Preconditions.checkState(maxHeaderBytes >= 0); Preconditions.checkState(maxHeaderBytes >= 0);
Builder builder = io.grpc.binarylog.Metadata.newBuilder(); Builder builder = io.grpc.binarylog.Metadata.newBuilder();
if (maxHeaderBytes > 0) {
byte[][] serialized = InternalMetadata.serialize(metadata);
int written = 0;
// This code is tightly coupled with Metadata's implementation // This code is tightly coupled with Metadata's implementation
byte[][] serialized;
if (maxHeaderBytes > 0 && (serialized = InternalMetadata.serialize(metadata)) != null) {
int written = 0;
for (int i = 0; i < serialized.length && written < maxHeaderBytes; i += 2) { for (int i = 0; i < serialized.length && written < maxHeaderBytes; i += 2) {
byte[] key = serialized[i]; byte[] key = serialized[i];
byte[] value = serialized[i + 1]; byte[] value = serialized[i + 1];
@ -440,6 +453,7 @@ final class BinaryLog {
@VisibleForTesting @VisibleForTesting
static Message messageToProto(ByteBuffer message, boolean compressed, int maxMessageBytes) { static Message messageToProto(ByteBuffer message, boolean compressed, int maxMessageBytes) {
Preconditions.checkNotNull(message);
int messageSize = message.remaining(); int messageSize = message.remaining();
Message.Builder builder = Message Message.Builder builder = Message
.newBuilder() .newBuilder()

55
services/src/test/java/io/grpc/services/BinaryLogTest.java
View File

@ -95,16 +95,16 @@ public final class BinaryLogTest {
private static final int HEADER_LIMIT = 10; private static final int HEADER_LIMIT = 10;
private static final int MESSAGE_LIMIT = Integer.MAX_VALUE; private static final int MESSAGE_LIMIT = Integer.MAX_VALUE;
private final Metadata metadata = new Metadata(); private final Metadata nonEmptyMetadata = new Metadata();
private final BinaryLogSink sink = mock(BinaryLogSink.class); private final BinaryLogSink sink = mock(BinaryLogSink.class);
private final BinaryLog binaryLog = new BinaryLog(sink, HEADER_LIMIT, MESSAGE_LIMIT); private final BinaryLog binaryLog = new BinaryLog(sink, HEADER_LIMIT, MESSAGE_LIMIT);
private final ByteBuffer message = ByteBuffer.wrap(new byte[100]); private final ByteBuffer message = ByteBuffer.wrap(new byte[100]);
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
metadata.put(KEY_A, DATA_A); nonEmptyMetadata.put(KEY_A, DATA_A);
metadata.put(KEY_B, DATA_B); nonEmptyMetadata.put(KEY_B, DATA_B);
metadata.put(KEY_C, DATA_C); nonEmptyMetadata.put(KEY_C, DATA_C);
} }
@Test @Test
@ -371,6 +371,13 @@ public final class BinaryLogTest {
BinaryLog.socketToProto(unknownSocket)); BinaryLog.socketToProto(unknownSocket));
} }
@Test
public void metadataToProto_empty() throws Exception {
assertEquals(
io.grpc.binarylog.Metadata.getDefaultInstance(),
BinaryLog.metadataToProto(new Metadata(), Integer.MAX_VALUE));
}
@Test @Test
public void metadataToProto() throws Exception { public void metadataToProto() throws Exception {
assertEquals( assertEquals(
@ -380,7 +387,7 @@ public final class BinaryLogTest {
.addEntry(ENTRY_B) .addEntry(ENTRY_B)
.addEntry(ENTRY_C) .addEntry(ENTRY_C)
.build(), .build(),
BinaryLog.metadataToProto(metadata, Integer.MAX_VALUE)); BinaryLog.metadataToProto(nonEmptyMetadata, Integer.MAX_VALUE));
} }
@Test @Test
@ -388,39 +395,39 @@ public final class BinaryLogTest {
// 0 byte limit not enough for any metadata // 0 byte limit not enough for any metadata
assertEquals( assertEquals(
io.grpc.binarylog.Metadata.getDefaultInstance(), io.grpc.binarylog.Metadata.getDefaultInstance(),
BinaryLog.metadataToProto(metadata, 0)); BinaryLog.metadataToProto(nonEmptyMetadata, 0));
// not enough bytes for first key value // not enough bytes for first key value
assertEquals( assertEquals(
io.grpc.binarylog.Metadata.getDefaultInstance(), io.grpc.binarylog.Metadata.getDefaultInstance(),
BinaryLog.metadataToProto(metadata, 9)); BinaryLog.metadataToProto(nonEmptyMetadata, 9));
// enough for first key value // enough for first key value
assertEquals( assertEquals(
io.grpc.binarylog.Metadata io.grpc.binarylog.Metadata
.newBuilder() .newBuilder()
.addEntry(ENTRY_A) .addEntry(ENTRY_A)
.build(), .build(),
BinaryLog.metadataToProto(metadata, 10)); BinaryLog.metadataToProto(nonEmptyMetadata, 10));
// Test edge cases for >= 2 key values // Test edge cases for >= 2 key values
assertEquals( assertEquals(
io.grpc.binarylog.Metadata io.grpc.binarylog.Metadata
.newBuilder() .newBuilder()
.addEntry(ENTRY_A) .addEntry(ENTRY_A)
.build(), .build(),
BinaryLog.metadataToProto(metadata, 19)); BinaryLog.metadataToProto(nonEmptyMetadata, 19));
assertEquals( assertEquals(
io.grpc.binarylog.Metadata io.grpc.binarylog.Metadata
.newBuilder() .newBuilder()
.addEntry(ENTRY_A) .addEntry(ENTRY_A)
.addEntry(ENTRY_B) .addEntry(ENTRY_B)
.build(), .build(),
BinaryLog.metadataToProto(metadata, 20)); BinaryLog.metadataToProto(nonEmptyMetadata, 20));
assertEquals( assertEquals(
io.grpc.binarylog.Metadata io.grpc.binarylog.Metadata
.newBuilder() .newBuilder()
.addEntry(ENTRY_A) .addEntry(ENTRY_A)
.addEntry(ENTRY_B) .addEntry(ENTRY_B)
.build(), .build(),
BinaryLog.metadataToProto(metadata, 29)); BinaryLog.metadataToProto(nonEmptyMetadata, 29));
assertEquals( assertEquals(
io.grpc.binarylog.Metadata io.grpc.binarylog.Metadata
.newBuilder() .newBuilder()
@ -428,7 +435,7 @@ public final class BinaryLogTest {
.addEntry(ENTRY_B) .addEntry(ENTRY_B)
.addEntry(ENTRY_C) .addEntry(ENTRY_C)
.build(), .build(),
BinaryLog.metadataToProto(metadata, 30)); BinaryLog.metadataToProto(nonEmptyMetadata, 30));
} }
@Test @Test
@ -504,7 +511,7 @@ public final class BinaryLogTest {
InetAddress address = InetAddress.getByName("127.0.0.1"); InetAddress address = InetAddress.getByName("127.0.0.1");
int port = 12345; int port = 12345;
InetSocketAddress socketAddress = new InetSocketAddress(address, port); InetSocketAddress socketAddress = new InetSocketAddress(address, port);
binaryLog.logSendInitialMetadata(metadata, IS_SERVER, CALL_ID, socketAddress); binaryLog.logSendInitialMetadata(nonEmptyMetadata, IS_SERVER, CALL_ID, socketAddress);
verify(sink).write( verify(sink).write(
GrpcLogEntry GrpcLogEntry
.newBuilder() .newBuilder()
@ -512,7 +519,7 @@ public final class BinaryLogTest {
.setLogger(GrpcLogEntry.Logger.SERVER) .setLogger(GrpcLogEntry.Logger.SERVER)
.setCallId(BinaryLog.callIdToProto(CALL_ID)) .setCallId(BinaryLog.callIdToProto(CALL_ID))
.setPeer(BinaryLog.socketToProto(socketAddress)) .setPeer(BinaryLog.socketToProto(socketAddress))
.setMetadata(BinaryLog.metadataToProto(metadata, 10)) .setMetadata(BinaryLog.metadataToProto(nonEmptyMetadata, 10))
.build()); .build());
} }
@ -521,7 +528,7 @@ public final class BinaryLogTest {
InetAddress address = InetAddress.getByName("127.0.0.1"); InetAddress address = InetAddress.getByName("127.0.0.1");
int port = 12345; int port = 12345;
InetSocketAddress socketAddress = new InetSocketAddress(address, port); InetSocketAddress socketAddress = new InetSocketAddress(address, port);
binaryLog.logSendInitialMetadata(metadata, IS_CLIENT, CALL_ID, socketAddress); binaryLog.logSendInitialMetadata(nonEmptyMetadata, IS_CLIENT, CALL_ID, socketAddress);
verify(sink).write( verify(sink).write(
GrpcLogEntry GrpcLogEntry
.newBuilder() .newBuilder()
@ -529,7 +536,7 @@ public final class BinaryLogTest {
.setLogger(GrpcLogEntry.Logger.CLIENT) .setLogger(GrpcLogEntry.Logger.CLIENT)
.setCallId(BinaryLog.callIdToProto(CALL_ID)) .setCallId(BinaryLog.callIdToProto(CALL_ID))
.setPeer(BinaryLog.socketToProto(socketAddress)) .setPeer(BinaryLog.socketToProto(socketAddress))
.setMetadata(BinaryLog.metadataToProto(metadata, 10)) .setMetadata(BinaryLog.metadataToProto(nonEmptyMetadata, 10))
.build()); .build());
} }
@ -538,7 +545,7 @@ public final class BinaryLogTest {
InetAddress address = InetAddress.getByName("127.0.0.1"); InetAddress address = InetAddress.getByName("127.0.0.1");
int port = 12345; int port = 12345;
InetSocketAddress socketAddress = new InetSocketAddress(address, port); InetSocketAddress socketAddress = new InetSocketAddress(address, port);
binaryLog.logRecvInitialMetadata(metadata, IS_SERVER, CALL_ID, socketAddress); binaryLog.logRecvInitialMetadata(nonEmptyMetadata, IS_SERVER, CALL_ID, socketAddress);
verify(sink).write( verify(sink).write(
GrpcLogEntry GrpcLogEntry
.newBuilder() .newBuilder()
@ -546,7 +553,7 @@ public final class BinaryLogTest {
.setLogger(GrpcLogEntry.Logger.SERVER) .setLogger(GrpcLogEntry.Logger.SERVER)
.setCallId(BinaryLog.callIdToProto(CALL_ID)) .setCallId(BinaryLog.callIdToProto(CALL_ID))
.setPeer(BinaryLog.socketToProto(socketAddress)) .setPeer(BinaryLog.socketToProto(socketAddress))
.setMetadata(BinaryLog.metadataToProto(metadata, 10)) .setMetadata(BinaryLog.metadataToProto(nonEmptyMetadata, 10))
.build()); .build());
} }
@ -555,7 +562,7 @@ public final class BinaryLogTest {
InetAddress address = InetAddress.getByName("127.0.0.1"); InetAddress address = InetAddress.getByName("127.0.0.1");
int port = 12345; int port = 12345;
InetSocketAddress socketAddress = new InetSocketAddress(address, port); InetSocketAddress socketAddress = new InetSocketAddress(address, port);
binaryLog.logRecvInitialMetadata(metadata, IS_CLIENT, CALL_ID, socketAddress); binaryLog.logRecvInitialMetadata(nonEmptyMetadata, IS_CLIENT, CALL_ID, socketAddress);
verify(sink).write( verify(sink).write(
GrpcLogEntry GrpcLogEntry
.newBuilder() .newBuilder()
@ -563,33 +570,33 @@ public final class BinaryLogTest {
.setLogger(GrpcLogEntry.Logger.CLIENT) .setLogger(GrpcLogEntry.Logger.CLIENT)
.setCallId(BinaryLog.callIdToProto(CALL_ID)) .setCallId(BinaryLog.callIdToProto(CALL_ID))
.setPeer(BinaryLog.socketToProto(socketAddress)) .setPeer(BinaryLog.socketToProto(socketAddress))
.setMetadata(BinaryLog.metadataToProto(metadata, 10)) .setMetadata(BinaryLog.metadataToProto(nonEmptyMetadata, 10))
.build()); .build());
} }
@Test @Test
public void logTrailingMetadata_server() throws Exception { public void logTrailingMetadata_server() throws Exception {
binaryLog.logTrailingMetadata(metadata, IS_SERVER, CALL_ID); binaryLog.logTrailingMetadata(nonEmptyMetadata, IS_SERVER, CALL_ID);
verify(sink).write( verify(sink).write(
GrpcLogEntry GrpcLogEntry
.newBuilder() .newBuilder()
.setType(GrpcLogEntry.Type.SEND_TRAILING_METADATA) .setType(GrpcLogEntry.Type.SEND_TRAILING_METADATA)
.setLogger(GrpcLogEntry.Logger.SERVER) .setLogger(GrpcLogEntry.Logger.SERVER)
.setCallId(BinaryLog.callIdToProto(CALL_ID)) .setCallId(BinaryLog.callIdToProto(CALL_ID))
.setMetadata(BinaryLog.metadataToProto(metadata, 10)) .setMetadata(BinaryLog.metadataToProto(nonEmptyMetadata, 10))
.build()); .build());
} }
@Test @Test
public void logTrailingMetadata_client() throws Exception { public void logTrailingMetadata_client() throws Exception {
binaryLog.logTrailingMetadata(metadata, IS_CLIENT, CALL_ID); binaryLog.logTrailingMetadata(nonEmptyMetadata, IS_CLIENT, CALL_ID);
verify(sink).write( verify(sink).write(
GrpcLogEntry GrpcLogEntry
.newBuilder() .newBuilder()
.setType(GrpcLogEntry.Type.RECV_TRAILING_METADATA) .setType(GrpcLogEntry.Type.RECV_TRAILING_METADATA)
.setLogger(GrpcLogEntry.Logger.CLIENT) .setLogger(GrpcLogEntry.Logger.CLIENT)
.setCallId(BinaryLog.callIdToProto(CALL_ID)) .setCallId(BinaryLog.callIdToProto(CALL_ID))
.setMetadata(BinaryLog.metadataToProto(metadata, 10)) .setMetadata(BinaryLog.metadataToProto(nonEmptyMetadata, 10))
.build()); .build());
} }