Repository: netty/netty
Issue: A number of toString() methods on classes that implement ByteBufHolder can throw IllegalReferenceCountException
Labels: defect

Two examples that I've found include `WebSocketFrame.toString()` and `DefaultByteBufHolder.toString()`.  `toString()` implementations should always try to avoid throwing exceptions, since they're used for things like creating exception messages and arbitrary logging of inputs/outputs.  Consequently, if `toString()` does throw an exception, this will often hide another real problem, a typical example would be:

``` java
TextWebSocketFrame frame = ...
try {
  String text = frame.text();
  ReferenceCountUtil.release(frame);
  doSomeProcessing(text);
} catch (Exception e) {
  log.error("Error processing frame: " + frame, e);
}
```

In this example, if `doSomeProcessing` throws an exception, the exception will be caught, but not logged, instead, when `frame.toString()` is invoked, that will throw an `IllegalReferenceCountException`.

I don't think there's any need to guard access to `ByteBuf.toString()`, since it likewise shouldn't throw an exception for the same reasons as above, so `ByteBufHolder` `toString()` implementations should just invoke that directly without going through any reference count checks.


Issue comments:
@jroper sounds like a bug... let me fix it.

Fixed by https://github.com/netty/netty/pull/4341


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java b/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java
index c029accedf6..ef39045dcdd 100644
--- a/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java
+++ b/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java
@@ -90,8 +90,16 @@ public boolean release(int decrement) {
         return data.release(decrement);
     }
 
+    /**
+     * Return {@link ByteBuf#toString()} without checking the reference count first. This is useful to implemement
+     * {@link #toString()}.
+     */
+    protected final String contentToString() {
+        return data.toString();
+    }
+
     @Override
     public String toString() {
-        return StringUtil.simpleClassName(this) + '(' + content().toString() + ')';
+        return StringUtil.simpleClassName(this) + '(' + contentToString() + ')';
     }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketFrame.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketFrame.java
index c46eec82bdd..37606ab535f 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketFrame.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketFrame.java
@@ -68,7 +68,7 @@ public int rsv() {
 
     @Override
     public String toString() {
-        return StringUtil.simpleClassName(this) + "(data: " + content() + ')';
+        return StringUtil.simpleClassName(this) + "(data: " + contentToString() + ')';
     }
 
     @Override
diff --git a/transport-sctp/src/main/java/io/netty/channel/sctp/SctpMessage.java b/transport-sctp/src/main/java/io/netty/channel/sctp/SctpMessage.java
index d602ee50e14..f809999d5d9 100644
--- a/transport-sctp/src/main/java/io/netty/channel/sctp/SctpMessage.java
+++ b/transport-sctp/src/main/java/io/netty/channel/sctp/SctpMessage.java
@@ -17,7 +17,6 @@
 
 import com.sun.nio.sctp.MessageInfo;
 import io.netty.buffer.ByteBuf;
-import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.DefaultByteBufHolder;
 
 /**
@@ -191,15 +190,9 @@ public SctpMessage touch(Object hint) {
 
     @Override
     public String toString() {
-        if (refCnt() == 0) {
-            return "SctpFrame{" +
-                    "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier +
-                    ", unordered=" + unordered +
-                    ", data=(FREED)}";
-        }
         return "SctpFrame{" +
-                "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier +
-                ", unordered=" + unordered +
-                ", data=" + ByteBufUtil.hexDump(content()) + '}';
+               "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier +
+               ", unordered=" + unordered +
+               ", data=" + contentToString() + '}';
     }
 }


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java b/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java
new file mode 100644
index 00000000000..0b462c4f541
--- /dev/null
+++ b/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2015 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.buffer;
+
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class DefaultByteBufHolderTest {
+
+    @Test
+    public void testToString() {
+        ByteBufHolder holder = new DefaultByteBufHolder(Unpooled.buffer());
+        assertEquals(1, holder.refCnt());
+        assertNotNull(holder.toString());
+        assertTrue(holder.release());
+        assertNotNull(holder.toString());
+    }
+}