Skip to content

Leak detection combined with composite buffers results in incorrectly handled writerIndex #8152

@nitsanw

Description

@nitsanw

This is a nasty bug, which only manifests with leak detection, so would be very confusing in the wild. It was discovered in a test suite which validated with a range of buffer types and resulted in a flaky test. Fortunately composite buffers are not in common use, and leak detection is not permanently on, so the bug should not effect most people most of the time. This also made it harder to hunt down.

In some places the writerIndex field of AbstractByteBuf is manipulated directly under the assumption that subclasses of AbstractByteBuf are safe to manipulate, while subclasses of WrappedByteBuf are to be unwrapped until, hopefully, an AbstractByteBuf will emerge.

E.g.:

    public static int writeAscii(ByteBuf buf, CharSequence seq) {
        // ASCII uses 1 byte per char
        final int len = seq.length();
        if (seq instanceof AsciiString) {
            AsciiString asciiString = (AsciiString) seq;
            buf.writeBytes(asciiString.array(), asciiString.arrayOffset(), len);
        } else {
            for (;;) {
                if (buf instanceof AbstractByteBuf) {
                    AbstractByteBuf byteBuf = (AbstractByteBuf) buf;
                    byteBuf.ensureWritable(len);
                    int written = writeAscii(byteBuf, byteBuf.writerIndex, seq, len);
                    byteBuf.writerIndex += written;
                    return written;
                } else if (buf instanceof WrappedByteBuf) {
                    // Unwrap as the wrapped buffer may be an AbstractByteBuf and so we can use fast-path.
                    buf = buf.unwrap();
                } else {
                    buf.writeBytes(seq.toString().getBytes(CharsetUtil.US_ASCII));
                }
            }
        }
        return len;
    }

The assumption breaks with WrappedCompositeByteBuf as it is a subclass of AbstractByteBuf via CompositeByteBuf, but should have perhaps been a subclass of WrappedByteBuf. The code ends up writing the data to the 0 offset, overwriting whatever it finds there.

I've hit the resulting bug on writeUtf and can see it would also manifest for writeAscii, and perhaps elsewhere. Leak detection for composite buffers extends WrappedCompositeByteBuf. The bug can be fixed by using the accessor methods instead of the field.

Steps to reproduce

Run a writeAscii/Utf test with paranoid leak detection to reproduce.

Minimal yet complete reproducer code (or URL to code)

Netty version

4.1, perhaps earlier too, I did not check.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions