From 7db9f454fe34c9f0136ef2d77185264de4154d06 Mon Sep 17 00:00:00 2001
From: Jongyeol Choi <jongyeol.choi@gmail.com>
Date: Sat, 30 Apr 2016 17:36:24 +0900
Subject: [PATCH] Fix decoding null bulk string of RedisDecoder

Motivation:

- `RedisBulkStringAggregator` raises errors for multiple null bulk strings.
- Null or empty bulk string has no content, but current `RedisDecoder` generates header and contents.

Modifications:

- Fix decoding null bulk string of `RedisDecoder` for `RedisBulkStringAggregator`.

Result:

- Fixes #5184.
---
 .../codec/redis/BulkStringHeaderRedisMessage.java |  3 +++
 .../codec/redis/RedisBulkStringAggregator.java    |  9 +--------
 .../netty/handler/codec/redis/RedisDecoder.java   |  8 ++++----
 .../handler/codec/redis/RedisDecoderTest.java     | 15 ++++++++++++---
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/codec-redis/src/main/java/io/netty/handler/codec/redis/BulkStringHeaderRedisMessage.java b/codec-redis/src/main/java/io/netty/handler/codec/redis/BulkStringHeaderRedisMessage.java
index 7bc46812a6..ebdda37bee 100644
--- a/codec-redis/src/main/java/io/netty/handler/codec/redis/BulkStringHeaderRedisMessage.java
+++ b/codec-redis/src/main/java/io/netty/handler/codec/redis/BulkStringHeaderRedisMessage.java
@@ -28,6 +28,9 @@ public class BulkStringHeaderRedisMessage implements RedisMessage {
      * @param bulkStringLength follow content length.
      */
     public BulkStringHeaderRedisMessage(int bulkStringLength) {
+        if (bulkStringLength <= 0) {
+            throw new RedisCodecException("bulkStringLength: " + bulkStringLength + " (expected: > 0)");
+        }
         this.bulkStringLength = bulkStringLength;
     }
 
diff --git a/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisBulkStringAggregator.java b/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisBulkStringAggregator.java
index 02ec0a4f9b..3b979c4259 100644
--- a/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisBulkStringAggregator.java
+++ b/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisBulkStringAggregator.java
@@ -93,13 +93,6 @@ public final class RedisBulkStringAggregator extends MessageAggregator<RedisMess
     @Override
     protected FullBulkStringRedisMessage beginAggregation(BulkStringHeaderRedisMessage start, ByteBuf content)
             throws Exception {
-        switch (start.bulkStringLength()) {
-        case RedisConstants.NULL_VALUE:
-            return FullBulkStringRedisMessage.NULL_INSTANCE;
-        case 0:
-            return FullBulkStringRedisMessage.EMPTY_INSTANCE;
-        default:
-            return new FullBulkStringRedisMessage(content);
-        }
+        return new FullBulkStringRedisMessage(content);
     }
 }
diff --git a/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisDecoder.java b/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisDecoder.java
index fe3f6c1ec8..54891a11c4 100644
--- a/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisDecoder.java
+++ b/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisDecoder.java
@@ -163,15 +163,14 @@ public final class RedisDecoder extends ByteToMessageDecoder {
                                               RedisConstants.REDIS_MESSAGE_MAX_LENGTH + ")");
             }
             remainingBulkLength = (int) length; // range(int) is already checked.
-            out.add(new BulkStringHeaderRedisMessage(remainingBulkLength));
-            return decodeBulkString(remainingBulkLength, in, out);
+            return decodeBulkString(in, out);
         default:
             throw new RedisCodecException("bad type: " + type);
         }
     }
 
-    private boolean decodeBulkString(int length, ByteBuf in, List<Object> out) throws Exception {
-        switch (length) {
+    private boolean decodeBulkString(ByteBuf in, List<Object> out) throws Exception {
+        switch (remainingBulkLength) {
         case RedisConstants.NULL_VALUE: // $-1\r\n
             out.add(FullBulkStringRedisMessage.NULL_INSTANCE);
             resetDecoder();
@@ -180,6 +179,7 @@ public final class RedisDecoder extends ByteToMessageDecoder {
             state = State.DECODE_BULK_STRING_EOL;
             return decodeBulkStringEndOfLine(in, out);
         default: // expectedBulkLength is always positive.
+            out.add(new BulkStringHeaderRedisMessage(remainingBulkLength));
             state = State.DECODE_BULK_STRING_CONTENT;
             return decodeBulkStringContent(in, out);
         }
diff --git a/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java b/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java
index 7eafd8215f..d7e7a40858 100644
--- a/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java
+++ b/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java
@@ -153,11 +153,20 @@ public class RedisDecoderTest {
         assertFalse(channel.writeInbound(byteBufOf(Integer.toString(-1))));
         assertTrue(channel.writeInbound(byteBufOf("\r\n")));
 
-        FullBulkStringRedisMessage msg = channel.readInbound();
+        assertTrue(channel.writeInbound(byteBufOf("$")));
+        assertTrue(channel.writeInbound(byteBufOf(Integer.toString(-1))));
+        assertTrue(channel.writeInbound(byteBufOf("\r\n")));
 
-        assertThat(msg.isNull(), is(true));
+        FullBulkStringRedisMessage msg1 = channel.readInbound();
+        assertThat(msg1.isNull(), is(true));
+        ReferenceCountUtil.release(msg1);
 
-        ReferenceCountUtil.release(msg);
+        FullBulkStringRedisMessage msg2 = channel.readInbound();
+        assertThat(msg2.isNull(), is(true));
+        ReferenceCountUtil.release(msg2);
+
+        FullBulkStringRedisMessage msg3 = channel.readInbound();
+        assertThat(msg3, is(nullValue()));
     }
 
     @Test