Extend ExtractUVarInt to support customizable kMaxVarintLen64

[ChinmayaSharma-hue]

ExtractUVarInt in Binary Decoder currently uses a definite value for kMaxVarintLen64, which can be a problem while parsing the variable encoding in protocols such as MQTT. which has the maximum limit set as 4 bytes. This PR makes a slight modification to support this functionality.

[ddelnano]

@ChinmayaSharma-hue thanks for opening this up. After thinking about this a second time, the UVarInt encoding is self delimiting. Meaning that the kMaxVarintLen64 is there to prevent erroneous, larger values. However, that condition isn't relevant for decoding smaller ones.

Rather than changing the implementation of this, I think it would be better if we added constants for the maximum value of a X bit encoded int to detect if we read too much data for the integer size we are interested in (since decoding could consume 10 bytes). If the MQTT parser consumed 5 to 10 bytes, it would indicate the MQTT packet was malformed or wasn't MQTT data at all.

Therefore, I'm thinking that we should define the following constants and that we can have it live within the binary_decoder.h file. We could probably do that all in the #1756 or we can update this to have that constant.

// This is the constant that the MQTT parser would check the UVarInt decoded value against.
// MQTT uses all 7 bits of each of the 4 bytes it encodes, which results in a total of 28 bits
uint64_t kMaxVarint28 = 268435455 // 2^28 - 1;


// mqtt/parser.cc code

// Need to verify length is at least 4 prior to this because too few bytes could be kNeedsMoreData
// (if buf.size() < 4) but would be kInvalid if the UVarInt was actually 5-10 bytes (bogus value)

PX_ASSIGN_OR(auto var_length_header,  binary_decoder->ExtractUVarInt(), return kInvalid);
if (var_length_header > kMaxVarint28) {
  return kInvalid; // MQTT variable length header too large. Frame is malformed or not MQTT
}

Sorry for going back and forth on this, but I had to refresh my memory of the encoding details. I still went through and reviewed the code since I figured those comments would be applicable for your upcoming code changes.

[ChinmayaSharma-hue]

This means that I can close this PR and make all the changes in #1756 right? The implementation of ExtractUVarInt remains the same and I should just add a check in the parser to make sure ExtractUVarInt did not consume more than 4 bytes. Am I getting this right?

[ddelnano]

@ChinmayaSharma-hue yep.

[ddelnano]

k prefixed variables with camelCase are constants. Since this is a function parameter, it should be snake case and not have a prefix.

We largely follow Google's C++ style guide as mentioned here.

[ddelnano]

uVarIntsWithLimits should be snake case according to our style guide.

[ddelnano]

Same casing change as the earlier comment.

[ddelnano]

Sorry I thought I submitted those comments with my initial comment!