-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Handle overflow in rd_buf_write_remains #4689
base: master
Are you sure you want to change the base?
Conversation
a1622dd
to
66fa00b
Compare
Hi @emasab, Could you please review and approve this PR? It's critical for us as it addresses a blocking issue with the confluent-kafka-dotnet library update. Your approval is eagerly awaited. |
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.
Thanks Anchit, the source of the problem comes from a bug that happens previously, check the comment.
There also a problem in My proposed solution is to add a |
Makes sense. I'll create a separate PR to handle that. |
@anchitj better to do it here, as it's basically the same thing that is missing |
e5886a9
to
4071f8b
Compare
5bb225f
to
daab8b9
Compare
daab8b9
to
396f7cc
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.
Thanks Anchit, some changes are needed
rd_buf_destroy_segment(rbuf, seg); | ||
rbuf->rbuf_erased -= toerase; |
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 needs to be the same as below, and before the destroy
rbuf->rbuf_erased -= toerase; | |
rbuf->rbuf_erased -= seg->seg_erased; |
@@ -710,6 +713,7 @@ int rd_buf_write_seek(rd_buf_t *rbuf, size_t absof) { | |||
rd_segment_t *this = next; | |||
next = TAILQ_PREV(this, rd_segment_head, seg_link); | |||
rd_buf_destroy_segment(rbuf, this); | |||
rbuf->rbuf_erased -= seg->seg_erased; |
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.
rbuf->rbuf_erased -= seg->seg_erased; | |
rbuf->rbuf_erased -= this->seg_erased; |
Put it before the rd_buf_destroy_segment
call too
const int client_id_length = 239; | ||
char *client_id; |
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 simplify it with a memset, see below. In C I only saw it happening from 270 on.
char client_id[271];
client_id = malloc((client_id_length + 1) * sizeof(char)); | ||
for (i = 0; i < client_id_length; i++) { | ||
client_id[i] = 'c'; | ||
} | ||
client_id[client_id_length] = '\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.
client_id = malloc((client_id_length + 1) * sizeof(char)); | |
for (i = 0; i < client_id_length; i++) { | |
client_id[i] = 'c'; | |
} | |
client_id[client_id_length] = '\0'; | |
/* A long client id must not cause a segmentation fault | |
* because of an erased segment when using flexver. | |
* See: https://github.com/confluentinc/confluent-kafka-dotnet/issues/2084 */ | |
memset(client_id, 'c', sizeof(client_id) - 1); | |
client_id[sizeof(client_id) - 1] = '\0'; |
free(client_id); | ||
|
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.
Remove this
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.
Please add the CHANGELOG entry too
After the implementation of KIP-320, TopicCnt was sent as a varint in the metadata request. This change caused an overflow issue due to the use of
rd_buf_erase
to erase the remaining bytes. The overflow occurred in the calculationrbuf->rbuf_size - (rbuf->rbuf_len + rbuf->rbuf_erased)
becauserbuf->rbuf_erased
was non-zero when the buffer was almost full, leading to an overflow condition.As a result, for certain configurations that caused the buffer to be nearly full, the client was unable to send metadata requests, and it also caused crashes in the .NET client.