Improve compression support: #3419
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,51 +120,94 @@ buffersBegin(BufferSequence const& bufs) | |
| bufs); | ||
| } | ||
|
|
||
| /** Parse a message header | ||
| * @return a seated optional if the message header was successfully | ||
| * parsed. An unseated optional otherwise, in which case | ||
| * @param ec contains more information: | ||
| * - set to `errc::success` if not enough bytes were present | ||
| * - set to `errc::no_message` if a valid header was not present | ||
| */ | ||
| template <class BufferSequence> | ||
| boost::optional<MessageHeader> | ||
|
There was a problem hiding this comment. Can we change this to There was a problem hiding this comment. We can. Shouldn't be a problem. |
||
| parseMessageHeader( | ||
|
There was a problem hiding this comment. What do you think of putting the bitwise comparison inside of a named lambda, where the name describes what is being checked? I don't really like bitwise operations in a conditional directly, because it doesn't read that easily. It always takes me a second to figure out whats going on, and I think sometimes subtle bugs can occur. Remember this: #3417? At the least, I think we should add a comment saying exactly what is being checked. There was a problem hiding this comment. Actually, after reading through the whole function, I think we should abstract away these bitwise operations to a class, or a bunch of helper functions. Bitwise operations are pretty low level, and I think it's hard to read the code and easy to mess up. We could compromise on more judicious use of comments (I would put a clear comment before every bitwise operation), but I'd really prefer named functions; it's pretty easy for a developer to change the code and forget to update the comment, but it's harder to change the function implementation and forget to update the function name. There was a problem hiding this comment. I hear what you're saying, but I think that adding additional lambas would only add confusion. Better comments would probably help; I'll try and follow-up. |
||
| boost::system::error_code& ec, | ||
| BufferSequence const& bufs, | ||
| std::size_t size) | ||
| { | ||
| using namespace ripple::compression; | ||
|
|
||
| MessageHeader hdr; | ||
| auto iter = buffersBegin(bufs); | ||
|
|
||
| // Check valid header | ||
| if (*iter & 0x80) | ||
nbougalis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| hdr.header_size = headerBytesCompressed; | ||
|
|
||
| // not enough bytes to parse the header | ||
| if (size < hdr.header_size) | ||
| { | ||
|
There was a problem hiding this comment. I prefer a style that checks for errors and returns early as opposed for checking for success. That style tends to reduce nesting indentation levels. Most of the code in this function is written this way, but this block doesn't do that. It's a minor nit, so no change needed, but since I don't have many comments in this PR I thought I'd point it out. |
||
| ec = make_error_code(boost::system::errc::success); | ||
| return boost::none; | ||
| } | ||
|
|
||
| if (*iter & 0x0C) | ||
| { | ||
| ec = make_error_code(boost::system::errc::protocol_error); | ||
| return boost::none; | ||
| } | ||
|
|
||
| hdr.algorithm = static_cast<compression::Algorithm>(*iter); | ||
|
|
||
| if (hdr.algorithm != compression::Algorithm::LZ4) | ||
| { | ||
| ec = make_error_code(boost::system::errc::protocol_error); | ||
| return boost::none; | ||
| } | ||
|
|
||
| for (int i = 0; i != 4; ++i) | ||
| hdr.payload_wire_size = (hdr.payload_wire_size << 8) + *iter++; | ||
|
|
||
| // clear the top four bits (the compression bits). | ||
| hdr.payload_wire_size &= 0x0FFFFFFF; | ||
|
|
||
| hdr.total_wire_size = hdr.header_size + hdr.payload_wire_size; | ||
|
|
||
| for (int i = 0; i != 2; ++i) | ||
| hdr.message_type = (hdr.message_type << 8) + *iter++; | ||
|
|
||
| for (int i = 0; i != 4; ++i) | ||
| hdr.uncompressed_size = (hdr.uncompressed_size << 8) + *iter++; | ||
|
|
||
| return hdr; | ||
| } | ||
|
|
||
| if ((*iter & 0xFC) == 0) | ||
| { | ||
| hdr.header_size = headerBytes; | ||
|
|
||
| if (size < hdr.header_size) | ||
| { | ||
| ec = make_error_code(boost::system::errc::success); | ||
| return boost::none; | ||
| } | ||
|
|
||
| hdr.algorithm = Algorithm::None; | ||
|
|
||
| for (int i = 0; i != 4; ++i) | ||
| hdr.payload_wire_size = (hdr.payload_wire_size << 8) + *iter++; | ||
|
|
||
| hdr.uncompressed_size = hdr.payload_wire_size; | ||
| hdr.total_wire_size = hdr.header_size + hdr.payload_wire_size; | ||
|
|
||
| for (int i = 0; i != 2; ++i) | ||
| hdr.message_type = (hdr.message_type << 8) + *iter++; | ||
|
|
||
| return hdr; | ||
| } | ||
|
|
||
| ec = make_error_code(boost::system::errc::no_message); | ||
| return boost::none; | ||
| } | ||
|
|
||
| template < | ||
|
|
@@ -186,7 +229,7 @@ invoke(MessageHeader const& header, Buffers const& buffers, Handler& handler) | |
| std::vector<std::uint8_t> payload; | ||
| payload.resize(header.uncompressed_size); | ||
|
|
||
| auto const payloadSize = ripple::compression::decompress( | ||
| stream, | ||
| header.payload_wire_size, | ||
| payload.data(), | ||
|
|
@@ -226,23 +269,35 @@ invokeProtocolMessage(Buffers const& buffers, Handler& handler) | |
| if (size == 0) | ||
| return result; | ||
|
|
||
| auto header = detail::parseMessageHeader(result.second, buffers, size); | ||
|
|
||
| // If we can't parse the header then it may be that we don't have enough | ||
| // bytes yet, or because the message was cut off (if error_code is success). | ||
| // Otherwise we failed to match the header's marker (error_code is set to | ||
| // no_message) or the compression algorithm is invalid (error_code is | ||
| // protocol_error) and signal an error. | ||
| if (!header) | ||
| return result; | ||
|
|
||
| // We implement a maximum size for protocol messages. Sending a message | ||
| // whose size exceeds this may result in the connection being dropped. A | ||
| // larger message size may be supported in the future or negotiated as | ||
| // part of a protocol upgrade. | ||
| if (header->payload_wire_size > megabytes(64) || | ||
| header->uncompressed_size > megabytes(64)) | ||
| { | ||
| result.second = make_error_code(boost::system::errc::message_size); | ||
| return result; | ||
| } | ||
|
|
||
| // We requested uncompressed messages from the peer but received compressed. | ||
| if (!handler.compressionEnabled() && | ||
| header->algorithm != compression::Algorithm::None) | ||
| { | ||
| result.second = make_error_code(boost::system::errc::protocol_error); | ||
| return result; | ||
| } | ||
|
|
||
| // We don't have the whole message yet. This isn't an error but we have | ||
| // nothing to do. | ||
| if (header->total_wire_size > size) | ||
|
|
||
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 the change from
0x01to0x90?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.
The new framing for compressed messages alters the first byte of the header, and specifically the first four bits: If the high bit is set, it means the message is compressed. The remaining three bits specify the compression algorithm used.
0x90is1001000in binary: high bit set (compression enabled) , low bit of high nibble set (LZ4).While reading your comment, it occurred to me to check when we set this value, and I noticed that the code there is wrong. It does:
*h |= 0x80 | (static_cast<uint8_t>(comprAlgorithm) << 4);Obviously not the right thing, this should now be:
*h |= static_cast<std::uint8_t>(comprAlgorithm);instead. For the record, this was a bug that I introduced into this PR and not @gregtatcam.I'll push a [FOLD] commit to address this.
Thanks @MarkusTeufelberger! Sharp eyes!