-
Notifications
You must be signed in to change notification settings - Fork 652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add better guards again re-entrance in ByteToMessageDecoder #370
Add better guards again re-entrance in ByteToMessageDecoder #370
Conversation
Sources/NIOHTTP1/HTTPDecoder.swift
Outdated
fileprivate var state = HTTPParserState() | ||
|
||
fileprivate init(type: HTTPMessageT.Type) { | ||
/* this is a private init, the public versions only allow HTTPClientResponsePart and HTTPServerRequestPart */ | ||
assert(HTTPMessageT.self == HTTPClientResponsePart.self || HTTPMessageT.self == HTTPServerRequestPart.self) | ||
} | ||
|
||
deinit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a deinit { }
for it as decoderRemoved
may be called while still in the decode(...)
and so I can not modify the parser
.
Sources/NIO/Codec.swift
Outdated
/// Decode in a loop until there is nothing more to decode. | ||
private func decodeLoop(ctx: ChannelHandlerContext, decodeFunc: (ChannelHandlerContext, inout ByteBuffer) throws -> DecodingState) { | ||
assert(self.cumulationBuffer != nil) | ||
ctx.withThrowingToFireErrorAndClose { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to hoist this up to the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean let the error bubble up ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let decodeLoop
throw, place the ctx.thenThrowingToFireErrorAndClose
in the function that calls decodeLoop
.
Sources/NIO/Codec.swift
Outdated
let writerIndex = buffer.writerIndex | ||
let result = try decodeFunc(ctx, &buffer) | ||
if self.cumulationBuffer != nil { | ||
self.cumulationBuffer!.moveReaderIndex(forwardBy: readable - buffer.readableBytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? We’re passing a local copy of the buffer, but then mutating the cumulation buffer. Why not just preserve the buffer returned from the call to decode, especially as we’re bothering to pass it inout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s right but I am all ears for better ideas. Can you show me some code with your suggestion as I don’t understand what exactly you suggest here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I'm just saying why not just do self.cumulationBuffer = buffer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I know why: the cumulationBuffer
may have been mutated. That's awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I'm inclined to say that we should probably pass &buffer
as a slice instead, and then come up with some custom logic to reconcile the changes from that slice with the parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it’s because of the reason you stated... a slice sounds ok, that said I am not sure if it really buys us anything
Sources/NIO/Codec.swift
Outdated
buffer.discardReadBytes() | ||
if self.cumulationBuffer != nil { | ||
if self.cumulationBuffer!.readableBytes > 0 { | ||
if self.shouldReclaimBytes(buffer: self.cumulationBuffer!) && self.cumulationBuffer!.discardReadBytes() && self.cumulationBuffer!.readableBytes == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s make this clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let me break up the if statement and add some comments
@swift-nio-bot test this please |
Also tests pass in docker locally... we may need to update timeouts |
Sources/NIOHTTP1/HTTPDecoder.swift
Outdated
parser.data = UnsafeMutableRawPointer(bitPattern: 0x0000deadbeef0000) | ||
|
||
// Set the callbacks to nil as we dont need these anymore | ||
settings.on_body = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should have the same effect:
settings = http_parser_settings()
@swift-nio-bot test this please |
5a9c7f6
to
5af4c27
Compare
Let me update the PR to only include the |
5af4c27
to
a7d4ff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test for this change?
4a0a64a
to
58ff97f
Compare
@Lukasa haha you were faster then me (and I did not refresh yet). Yep will add a few tests. |
58ff97f
to
d1f08f5
Compare
@normanmaurer awesome!
Please edit the docker compose files and lower the limits once again. Just add the changes to this PR |
Sources/NIO/Codec.swift
Outdated
if self.shouldReclaimBytes(buffer: buffer) { | ||
buffer.discardReadBytes() | ||
// Discard the cumulationBuffer or discard read bytes if needed. | ||
guard self.cumulationBuffer != nil else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use guard let
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we want to act on the self.cumulationBuffer
on the following lines and not on a different copy ?
Sources/NIO/Codec.swift
Outdated
} | ||
|
||
if self.shouldReclaimBytes(buffer: self.cumulationBuffer!) && self.cumulationBuffer!.discardReadBytes() { | ||
if self.cumulationBuffer!.readableBytes == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to just hoist this check up? It won’t be changed by reclaiming bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa you are right this can be removed.
@@ -96,64 +106,95 @@ private extension ChannelHandlerContext { | |||
|
|||
extension ByteToMessageDecoder { | |||
|
|||
/// Decode in a loop until there is nothing more to decode. | |||
private func decodeLoop(ctx: ChannelHandlerContext, decodeFunc: (ChannelHandlerContext, inout ByteBuffer) throws -> DecodingState) throws { | |||
while var slice = self.cumulationBuffer?.slice(), slice.readableBytes > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weissi @Lukasa so I started to write some unit tests and notice we can not do this.. The problem here is that it is possible that decodeLast
will be triggered from within decodeFunc
(due a close(...)
call for example). In this case decodeLast
will by default call decode(...)
which will then see the same bytes again even if decode(...)
before increased the readerIndex
as we had no chance to replicate these changes to the cumulationBuffer
yet :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's no way to entirely prevent re-entrancy without removing decodeLast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... so I wonder what we should do here in the case... any suggestions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I'm inclined to sit on it and wait until 2.0, when decodeLast is removed. In the meantime we can recommend users override decodeLast or channelInactive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa when you say sit on it you suggest just not pass in a slice but self.cumulationBuffer
or pass in the slice (as I did here) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it doesn't matter what you do, the issue is still the same: decodeLast
is not safe from avoid reentrancy. So you may as well keep using the slice.
d01f35c
to
8abacf9
Compare
Sources/NIO/Codec.swift
Outdated
} | ||
|
||
guard slice.writerIndex == sliceWriterIndex else { | ||
fatalError("Writing to the buffer is not allowed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, but I tend to prefer these to be preconditionFailure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure why not... I dont care and I think at the end it makes no difference as long as it crashes :)
Tests/NIOTests/CodecTest.swift
Outdated
typealias InboundOut = ByteBuffer | ||
|
||
var cumulationBuffer: ByteBuffer? | ||
var triggeredReentrace: Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "reentrance"
Tests/NIOTests/CodecTest.swift
Outdated
} | ||
|
||
if !self.triggeredReentrace { | ||
self.triggeredReentrace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than set this here, it might be better to let the re-entrant call set it (where you currently have an XCTAssertTrue
) and then check it at the end of the test. Otherwise this test could pass if you never re-entered at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'm basically happy. @weissi?
Sources/NIO/Codec.swift
Outdated
/// 1. Moving the reader index forward persists across calls. When your method returns, if the reader index has advanced, those bytes are considered "consumed" and will not be available in future calls to `decode`. | ||
/// Please note, however, that the numerical value of the `readerIndex` itself is not preserved, and may not be the same from one call to the next. Please do not rely on this numerical value: if you need | ||
/// to recall where a byte is relative to the `readerIndex`, use an offset rather than an absolute value. | ||
/// 2. Mutating the bytes in the buffer will cause a `fatalError` and so is not allowed. You are only allowed to move the readerIndex forward or consume from the buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we say 'mutating the bytes or the readerIndex will cause undefined behaviour and likely crash your program' or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly nits and can you lower the allocation limits?
Sources/NIO/Codec.swift
Outdated
break | ||
} | ||
|
||
guard slice.writerIndex == sliceWriterIndex else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just precondition(slice.writeIndex == sliceWriterIndex, "...")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice one, ta
Motivation: When the ByteToMessageDecoder re-entranced in its decode(...) / decodeLast(...) methods it could be possible that the ordering of processing and so the seen bytes are mixed. Modifications: - Refresh the buffer (and take a slice) on each loop iteration and update the cumulationBuffers indicies. Result: More robust code.
832de2f
to
35783b7
Compare
Motivation:
When the ByteToMessageDecoder re-entranced in its decode(...) / decodeLast(...) methods it could be possible that the ordering of processing and so the seen bytes are mixed.
Modifications:
Result:
More robust code.