From 4f6d05365a284edf2f692e11b573e971c258a1e7 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Sun, 10 Feb 2013 14:22:14 +0900 Subject: [PATCH] Fix a race condition in reference counter implementation / Reference count never goes below 0 --- .../java/io/netty/buffer/AbstractByteBuf.java | 2 +- .../buffer/AbstractReferenceCounted.java | 20 ++++++++++++------- .../AbstractReferenceCountedByteBuf.java | 20 ++++++++++++------- .../codec/spdy/DefaultSpdyDataFrame.java | 2 +- .../io/netty/channel/sctp/SctpMessage.java | 2 +- .../netty/channel/socket/DatagramPacket.java | 2 +- .../channel/socket/aio/AioSocketChannel.java | 4 ++-- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java index d3ccaea9f0..ae7a0422c7 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java @@ -1032,7 +1032,7 @@ public abstract class AbstractByteBuf implements ByteBuf { } protected final void ensureAccessible() { - if (refCnt() <= 0) { + if (refCnt() == 0) { throw new IllegalBufferAccessException(); } } diff --git a/buffer/src/main/java/io/netty/buffer/AbstractReferenceCounted.java b/buffer/src/main/java/io/netty/buffer/AbstractReferenceCounted.java index e9b89936f7..411802fe77 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractReferenceCounted.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractReferenceCounted.java @@ -32,15 +32,18 @@ public abstract class AbstractReferenceCounted implements ReferenceCounted { @Override public final void retain() { - do { + for (;;) { int refCnt = this.refCnt; - if (refCnt <= 0) { + if (refCnt == 0) { throw new IllegalBufferAccessException(); } if (refCnt == Integer.MAX_VALUE) { throw new IllegalBufferAccessException("refCnt overflow"); } - } while (!refCntUpdater.compareAndSet(this, refCnt, refCnt + 1)); + if (refCntUpdater.compareAndSet(this, refCnt, refCnt + 1)) { + break; + } + } } @Override @@ -49,22 +52,25 @@ public abstract class AbstractReferenceCounted implements ReferenceCounted { throw new IllegalArgumentException("increment: " + increment + " (expected: > 0)"); } - do { + for (;;) { int refCnt = this.refCnt; - if (refCnt <= 0) { + if (refCnt == 0) { throw new IllegalBufferAccessException(); } if (refCnt > Integer.MAX_VALUE - increment) { throw new IllegalBufferAccessException("refCnt overflow"); } - } while (!refCntUpdater.compareAndSet(this, refCnt, refCnt + increment)); + if (refCntUpdater.compareAndSet(this, refCnt, refCnt + increment)) { + break; + } + } } @Override public final boolean release() { for (;;) { int refCnt = this.refCnt; - if (refCnt <= 0) { + if (refCnt == 0) { throw new IllegalBufferAccessException(); } diff --git a/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java index fdd66f3d36..8321795e56 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java @@ -37,15 +37,18 @@ public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf { @Override public final void retain() { - do { + for (;;) { int refCnt = this.refCnt; - if (refCnt <= 0) { + if (refCnt == 0) { throw new IllegalBufferAccessException(); } if (refCnt == Integer.MAX_VALUE) { throw new IllegalBufferAccessException("refCnt overflow"); } - } while (!refCntUpdater.compareAndSet(this, refCnt, refCnt + 1)); + if (refCntUpdater.compareAndSet(this, refCnt, refCnt + 1)) { + break; + } + } } @Override @@ -54,22 +57,25 @@ public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf { throw new IllegalArgumentException("increment: " + increment + " (expected: > 0)"); } - do { + for (;;) { int refCnt = this.refCnt; - if (refCnt <= 0) { + if (refCnt == 0) { throw new IllegalBufferAccessException(); } if (refCnt > Integer.MAX_VALUE - increment) { throw new IllegalBufferAccessException("refCnt overflow"); } - } while (!refCntUpdater.compareAndSet(this, refCnt, refCnt + increment)); + if (refCntUpdater.compareAndSet(this, refCnt, refCnt + increment)) { + break; + } + } } @Override public final boolean release() { for (;;) { int refCnt = this.refCnt; - if (refCnt <= 0) { + if (refCnt == 0) { throw new IllegalBufferAccessException(); } diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/DefaultSpdyDataFrame.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/DefaultSpdyDataFrame.java index 4edbf1bb4d..6702613ef0 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/spdy/DefaultSpdyDataFrame.java +++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/DefaultSpdyDataFrame.java @@ -101,7 +101,7 @@ public class DefaultSpdyDataFrame extends DefaultByteBufHolder implements SpdyDa buf.append(streamId); buf.append(StringUtil.NEWLINE); buf.append("--> Size = "); - if (refCnt() <= 0) { + if (refCnt() == 0) { buf.append("(freed)"); } else { buf.append(data().readableBytes()); 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 117ad9a545..f2f93b3ea2 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 @@ -137,7 +137,7 @@ public final class SctpMessage extends DefaultByteBufHolder { @Override public String toString() { - if (refCnt() <= 0) { + if (refCnt() == 0) { return "SctpFrame{" + "streamIdentifier=" + streamIdentifier + ", protocolIdentifier=" + protocolIdentifier + ", data=(FREED)}"; diff --git a/transport/src/main/java/io/netty/channel/socket/DatagramPacket.java b/transport/src/main/java/io/netty/channel/socket/DatagramPacket.java index dcfe373a50..d8a7c44cbd 100644 --- a/transport/src/main/java/io/netty/channel/socket/DatagramPacket.java +++ b/transport/src/main/java/io/netty/channel/socket/DatagramPacket.java @@ -57,7 +57,7 @@ public final class DatagramPacket extends DefaultByteBufHolder { @Override public String toString() { - if (refCnt() <= 0) { + if (refCnt() == 0) { return "DatagramPacket{remoteAddress=" + remoteAddress().toString() + ", data=(FREED)}"; } diff --git a/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java b/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java index 14e6051ee0..a706190423 100755 --- a/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java +++ b/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java @@ -254,7 +254,7 @@ public class AioSocketChannel extends AbstractAioChannel implements SocketChanne try { if (buf.isReadable()) { for (;;) { - if (buf.refCnt() <= 0) { + if (buf.refCnt() == 0) { break; } // Ensure the readerIndex of the buffer is 0 before beginning an async write. @@ -370,7 +370,7 @@ public class AioSocketChannel extends AbstractAioChannel implements SocketChanne channel.writeInProgress = false; ByteBuf buf = channel.unsafe().directOutboundContext().outboundByteBuffer(); - if (buf.refCnt() <= 0) { + if (buf.refCnt() == 0) { return; }