Unpooled and Wrapped Buffer Leak

Motivation:
There are a few buffer leaks related to how Unpooled.wrapped and Base64.encode is used.

Modifications:
- Fix usages of Bas64.encode to correct leaks
- Clarify interface of Unpooled.wrapped* to ensure reference count ownership is clearly defined.

Result:
Reference count code is more clearly defined and less leaks are possible.
This commit is contained in:
Scott Mitchell 2015-12-29 10:04:34 -08:00
parent e7af3ee970
commit e578134b57
4 changed files with 106 additions and 18 deletions

View File

@ -211,11 +211,15 @@ public final class Unpooled {
* Creates a new buffer which wraps the specified buffer's readable bytes. * Creates a new buffer which wraps the specified buffer's readable bytes.
* A modification on the specified buffer's content will be visible to the * A modification on the specified buffer's content will be visible to the
* returned buffer. * returned buffer.
* @param buffer The buffer to wrap. Reference count ownership of this variable is transfered to this method.
* @return The readable portion of the {@code buffer}, or an empty buffer if there is no readable portion.
* The caller is responsible for releasing this buffer.
*/ */
public static ByteBuf wrappedBuffer(ByteBuf buffer) { public static ByteBuf wrappedBuffer(ByteBuf buffer) {
if (buffer.isReadable()) { if (buffer.isReadable()) {
return buffer.slice(); return buffer.slice();
} else { } else {
buffer.release();
return EMPTY_BUFFER; return EMPTY_BUFFER;
} }
} }
@ -233,6 +237,8 @@ public final class Unpooled {
* Creates a new big-endian composite buffer which wraps the readable bytes of the * Creates a new big-endian composite buffer which wraps the readable bytes of the
* specified buffers without copying them. A modification on the content * specified buffers without copying them. A modification on the content
* of the specified buffers will be visible to the returned buffer. * of the specified buffers will be visible to the returned buffer.
* @param buffers The buffers to wrap. Reference count ownership of all variables is transfered to this method.
* @return The readable portion of the {@code buffers}. The caller is responsible for releasing this buffer.
*/ */
public static ByteBuf wrappedBuffer(ByteBuf... buffers) { public static ByteBuf wrappedBuffer(ByteBuf... buffers) {
return wrappedBuffer(16, buffers); return wrappedBuffer(16, buffers);
@ -285,20 +291,29 @@ public final class Unpooled {
* Creates a new big-endian composite buffer which wraps the readable bytes of the * Creates a new big-endian composite buffer which wraps the readable bytes of the
* specified buffers without copying them. A modification on the content * specified buffers without copying them. A modification on the content
* of the specified buffers will be visible to the returned buffer. * of the specified buffers will be visible to the returned buffer.
* @param maxNumComponents Advisement as to how many independent buffers are allowed to exist before
* consolidation occurs.
* @param buffers The buffers to wrap. Reference count ownership of all variables is transfered to this method.
* @return The readable portion of the {@code buffers}. The caller is responsible for releasing this buffer.
*/ */
public static ByteBuf wrappedBuffer(int maxNumComponents, ByteBuf... buffers) { public static ByteBuf wrappedBuffer(int maxNumComponents, ByteBuf... buffers) {
switch (buffers.length) { switch (buffers.length) {
case 0: case 0:
break; break;
case 1: case 1:
if (buffers[0].isReadable()) { ByteBuf buffer = buffers[0];
return wrappedBuffer(buffers[0].order(BIG_ENDIAN)); if (buffer.isReadable()) {
return wrappedBuffer(buffer.order(BIG_ENDIAN));
} else {
buffer.release();
} }
break; break;
default: default:
for (ByteBuf b: buffers) { for (ByteBuf b: buffers) {
if (b.isReadable()) { if (b.isReadable()) {
return new CompositeByteBuf(ALLOC, false, maxNumComponents, buffers); return new CompositeByteBuf(ALLOC, false, maxNumComponents, buffers);
} else {
b.release();
} }
} }
} }

View File

@ -283,6 +283,42 @@ public class UnpooledTest {
ByteBuffer.wrap(new byte[] { 3 })))); ByteBuffer.wrap(new byte[] { 3 }))));
} }
@Test
public void testSingleWrappedByteBufReleased() {
ByteBuf buf = buffer(12).writeByte(0);
ByteBuf wrapped = wrappedBuffer(buf);
assertTrue(wrapped.release());
assertEquals(0, buf.refCnt());
}
@Test
public void testSingleUnReadableWrappedByteBufReleased() {
ByteBuf buf = buffer(12);
ByteBuf wrapped = wrappedBuffer(buf);
assertFalse(wrapped.release()); // EMPTY_BUFFER cannot be released
assertEquals(0, buf.refCnt());
}
@Test
public void testMultiByteBufReleased() {
ByteBuf buf1 = buffer(12).writeByte(0);
ByteBuf buf2 = buffer(12).writeByte(0);
ByteBuf wrapped = wrappedBuffer(16, buf1, buf2);
assertTrue(wrapped.release());
assertEquals(0, buf1.refCnt());
assertEquals(0, buf2.refCnt());
}
@Test
public void testMultiUnReadableByteBufReleased() {
ByteBuf buf1 = buffer(12);
ByteBuf buf2 = buffer(12);
ByteBuf wrapped = wrappedBuffer(16, buf1, buf2);
assertFalse(wrapped.release()); // EMPTY_BUFFER cannot be released
assertEquals(0, buf1.refCnt());
assertEquals(0, buf2.refCnt());
}
@Test @Test
public void testCopiedBuffer() { public void testCopiedBuffer() {
assertEquals(16, copiedBuffer(ByteBuffer.allocateDirect(16)).capacity()); assertEquals(16, copiedBuffer(ByteBuffer.allocateDirect(16)).capacity());

View File

@ -486,9 +486,18 @@ public abstract class OpenSslContext extends SslContext {
ByteBuf buffer = Unpooled.directBuffer(); ByteBuf buffer = Unpooled.directBuffer();
try { try {
buffer.writeBytes(BEGIN_PRIVATE_KEY); buffer.writeBytes(BEGIN_PRIVATE_KEY);
ByteBuf encoded = Base64.encode(Unpooled.wrappedBuffer(key.getEncoded()), true); ByteBuf wrappedBuf = Unpooled.wrappedBuffer(key.getEncoded());
buffer.writeBytes(encoded); final ByteBuf encodedBuf;
encoded.release(); try {
encodedBuf = Base64.encode(wrappedBuf, true);
try {
buffer.writeBytes(encodedBuf);
} finally {
encodedBuf.release();
}
} finally {
wrappedBuf.release();
}
buffer.writeBytes(END_PRIVATE_KEY); buffer.writeBytes(END_PRIVATE_KEY);
return newBIO(buffer); return newBIO(buffer);
} finally { } finally {
@ -508,9 +517,17 @@ public abstract class OpenSslContext extends SslContext {
try { try {
for (X509Certificate cert: certChain) { for (X509Certificate cert: certChain) {
buffer.writeBytes(BEGIN_CERT); buffer.writeBytes(BEGIN_CERT);
ByteBuf encoded = Base64.encode(Unpooled.wrappedBuffer(cert.getEncoded()), true); ByteBuf wrappedBuf = Unpooled.wrappedBuffer(cert.getEncoded());
buffer.writeBytes(encoded); try {
encoded.release(); ByteBuf encodedBuf = Base64.encode(wrappedBuf, true);
try {
buffer.writeBytes(encodedBuf);
} finally {
encodedBuf.release();
}
} finally {
wrappedBuf.release();
}
buffer.writeBytes(END_CERT); buffer.writeBytes(END_CERT);
} }
return newBIO(buffer); return newBIO(buffer);

View File

@ -219,10 +219,21 @@ public final class SelfSignedCertificate {
static String[] newSelfSignedCertificate( static String[] newSelfSignedCertificate(
String fqdn, PrivateKey key, X509Certificate cert) throws IOException, CertificateEncodingException { String fqdn, PrivateKey key, X509Certificate cert) throws IOException, CertificateEncodingException {
// Encode the private key into a file. // Encode the private key into a file.
ByteBuf enc = Base64.encode(Unpooled.wrappedBuffer(key.getEncoded()), true); ByteBuf wrappedBuf = Unpooled.wrappedBuffer(key.getEncoded());
String keyText = "-----BEGIN PRIVATE KEY-----\n" + enc.toString(CharsetUtil.US_ASCII) + ByteBuf encodedBuf;
"\n-----END PRIVATE KEY-----\n"; final String keyText;
enc.release(); try {
encodedBuf = Base64.encode(wrappedBuf, true);
try {
keyText = "-----BEGIN PRIVATE KEY-----\n" +
encodedBuf.toString(CharsetUtil.US_ASCII) +
"\n-----END PRIVATE KEY-----\n";
} finally {
encodedBuf.release();
}
} finally {
wrappedBuf.release();
}
File keyFile = File.createTempFile("keyutil_" + fqdn + '_', ".key"); File keyFile = File.createTempFile("keyutil_" + fqdn + '_', ".key");
keyFile.deleteOnExit(); keyFile.deleteOnExit();
@ -239,12 +250,21 @@ public final class SelfSignedCertificate {
} }
} }
ByteBuf encoded = Base64.encode(Unpooled.wrappedBuffer(cert.getEncoded()), true); wrappedBuf = Unpooled.wrappedBuffer(cert.getEncoded());
// Encode the certificate into a CRT file. final String certText;
String certText = "-----BEGIN CERTIFICATE-----\n" + try {
encoded.toString(CharsetUtil.US_ASCII) + encodedBuf = Base64.encode(wrappedBuf, true);
"\n-----END CERTIFICATE-----\n"; try {
encoded.release(); // Encode the certificate into a CRT file.
certText = "-----BEGIN CERTIFICATE-----\n" +
encodedBuf.toString(CharsetUtil.US_ASCII) +
"\n-----END CERTIFICATE-----\n";
} finally {
encodedBuf.release();
}
} finally {
wrappedBuf.release();
}
File certFile = File.createTempFile("keyutil_" + fqdn + '_', ".crt"); File certFile = File.createTempFile("keyutil_" + fqdn + '_', ".crt");
certFile.deleteOnExit(); certFile.deleteOnExit();