k/protocol: Checking for tag value to be 0 #11443
Conversation
Tag fields that are primitives should not be encoded if their value is equal to 0. Fixes: redpanda-data#11358 Signed-off-by: Michael Boquard <[email protected]>
michael-redpanda
requested review from
graphcareful,
dotnwat,
NyaliaLui,
BenPope and
twmb
and removed request for
graphcareful
|
Slight addendum -- they should not be encoded if they are equal to their default -- frequently, the default is 0, but there can be a non-zero default. |
|
Is there a unit test we can add that would reliably reproduce it before this fix? |
There already is a check for that (if default is set and is equal to default, then don't encode). |
Yeah, maybe something simple where I create two of these messages, one with error code = 1 and one with error code = 0. The error code = 0 one should be 4 bytes smaller. It's not a 'smart' test but it's quick. |
We shouldn't crash if we parse an unknown error code. Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
added |
| @@ -196,7 +196,7 @@ std::string_view error_code_to_str(error_code error) { | |||
| case error_code::transactional_id_not_found: | |||
| return "transactional_id_not_found"; | |||
| default: | |||
| std::terminate(); // make gcc happy | |||
| return "unknown_error_code"; | |||
There was a problem hiding this comment.
I think the logic here is we should crash because if we say we support a version of a certain kafka API and we observe an unknown error code, it means there's a bug in redpanda and the handler is implemented incorrectly.
There was a problem hiding this comment.
But I guess that is debatable though, realistically perhaps crashing is too harsh.
There was a problem hiding this comment.
But I guess that is debatable though, realistically perhaps crashing is too harsh.
I feel like this is an easy DoS to run against RP
|
/backport v23.1.x |
|
/backport v22.3.x |
|
/backport v22.2.x |
Tag fields that are primitives, and don't have assigned default values, should not be encoded if their value is equal to 0.
Fixes: #11358
Backports Required
Release Notes
Improvements