http chunked response with buffer slices served over TLS corrupts input data

[conet]

Version

4.3.4

Context

While working on a streaming http server I encountered data corruption if TLS was used, this did not happen if TLS was not present. As far as I can tell buffers sent as response should be considered read only so I consider this alteration of buffer contents as a bug. If this was not true it means that the data delivered on every response must be copied which is a huge performance issue.

Do you have a reproducer?

The reproducer contains all the details.

https://github.com/conet/vertx-tls-reproducer

Steps to reproduce

1. Run the test with TLS enabled
2. Compare the input file with the output file
3. The actual result is that the files are different
4. The files should be identical
5. All diff instances involve http chunked encoding protocol data 0D 0A 66 61 30 0D 0A -> CR LF size CR LF which means that the chunked encoding protocol corrupts the input buffer (emitted in slices - read only references pointing to the same underlying buffer)
6. This does not happen when TLS is disabled

Extra

Might be caused by something like this

[conet]

I found out that if I transform the source buffers into a read only buffers like this:

.map(buffer -> Buffer.buffer(buffer.getByteBuf().asReadOnly()))

The corruption does not happen anymore. This is a solution that we can work with because sharing a read only buffer stream is really what we were looking for. Although I still think that there is a problem that the input buffer is modified in the write function.

[vietj]

vertx buffers can be shared only if they are not modified, usually sending buffer on a response should not modify them unless there is a bug (e.g like you pointed out there was in netty).

Can you try using a specific implementation of netty ByteBuf that logs when the buffer is actually (with a stacktrace) modified, instead of being read-only ?

[conet]

I tried it but it didn't work, looks like the write does not go through the implementation that calls ensureWritable or maybe I'm missing something.

[conet]

After some debugging and I think that the problem is here data is being accumulated to the vert.x buffer (the slice in this case) see the screenshot

The workaround has an effect because the intermediate ReadOnlyByteBuf that wraps the data has this implementation:

    public int ensureWritable(int minWritableBytes, boolean force) {
        return 1;
    }

which prevents SSLHandler from appending bytes to the buffer. If this is the case then all normal buffers sent as a response are appended with bytes, in our case it became a corruption because we were sending slices.

As you can see the cumulation member is the ReadOnlyByteBuf (when the workaround is applied)

[conet]

Based on my previous comment it looks like this exactly an effect of netty/netty#11792

[vietj]

thanks for your investigation work @conet , can we reproduce this with a smaller reproducer (i.e with a minimum amount of data) ?

[vietj]

what is not clear to me with respect to the netty issue that is referenced is that

• in the netty bug, the byte buf were corrupted because they are shared
• in this case buffers are not shared since they originate from the file system

so I do not understand yet how the bug happens if the byte buf originating from the file system can be expanded

[vietj]

ah I think I understand what I was missing : it comes from the slice operation that is performed in the concatMap that shares the same underlying bigger byte buf.

[vietj]

so here is what happen:

• the concatMap will slice vertx buffers that create netty slices, normally those buffers cannot be appended to
• when such buffers are written to the HttpServerResponse, they are duplicated (so the netty slice is duplicated) and the duplicate does not respect the slice nature of the buffer, making the buffer appendable

e.g let's take this vertx code

    Buffer hello_world = Buffer.buffer("Hello World");
    Buffer hello = hello_world.slice(0, 5); // create a Netty slice
    hello.getByteBuf().writeByte('_'); // getByteBuf duplicates the bytebuf which is  a slice, it is duplicated as a regular byte buf and we get Hello_World which should not be possible
    ((BufferImpl)hello).byteBuf().writeByte(' '); // bytebuf() returns the original buffer, throws an exception because we are using a slice

[conet]

I updated the project to make it a self contained test without external files. Yes the slice operation is the key cause of this but I think that the code should handle it without the corruption.

[vietj]

wondering first whether the netty slice duplicate should not itself duplicate into a slice as well, or vertx should ensure the correctness.

the javadoc of duplicate says:

A buffer whose readable content is equivalent to the buffer returned by slice(). However this buffer will share the capacity of the underlying buffer, and therefore allows access to all of the underlying content if necessary.

[conet]

That is a good question, if you think that vert.x uses netty correctly then the problem is in netty otherwise it's a problem vert.x needs to fix. My feeling is that duplicate should preserve attributes such as readOnly.

[vietj]

I don't think read only is involved here, a slice is writable, however it cannot be expanded which cause the issue with SSL handler.

[conet]

OK, I'm not that familiar with all netty buffer attributes, I was thinking read only because making the underlying buffer (the one that is sliced) read only fixes the problem.

[vietj]

the main difference now I can find is:

    System.out.println(hello.getByteBuf().isWritable(1)); // true
    System.out.println(((BufferImpl)hello).byteBuf().isWritable(1)); // false

so we could use this when we duplicate and determine whether we need to perform extra work, i.e in the case the buffer is not writable then duplicate would do

buffer.unwrap().slice(buffer.readerIndex(), buffer.writeIndex())

[conet]

Yes, I think that would work, that is also the check used by SSLHandler.

[vietj]

that sound more complex than at first sight, I'm having a look but in reality a sliced buffer has an adjustment field (offset) and it is not possible to know this adjustment field when we use a slice, which means that a vertx sliced buffer should retain the original information when a slice operation happens in vertx I think.

[vietj]

I'll provide a patch soon and check against the reproducer you provided @conet

[vietj]

created this issue to check whether that's considered as a ByteBuf defect : netty/netty#12919

[vietj]

it is not considered as a defect, I believe one way we can do it, is simply by replacing the duplicate by a slice, so vertx buffers will always send slices through the pipeline which will always prevent the buffers to be appended to.

[vietj]

fixed by #4517