Skip to content

Handle overflow in rd_buf_write_remains#4689

Merged
Emanuele Sabellico (emasab) merged 7 commits intomasterfrom
dev_rd_buf_write_remains
Jun 11, 2024
Merged

Handle overflow in rd_buf_write_remains#4689
Emanuele Sabellico (emasab) merged 7 commits intomasterfrom
dev_rd_buf_write_remains

Conversation

@anchitj
Copy link
Copy Markdown
Contributor

@anchitj Anchit Jain (anchitj) commented Apr 16, 2024 •
edited
Loading

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 calculation rbuf->rbuf_size - (rbuf->rbuf_len + rbuf->rbuf_erased) because rbuf->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.

@anchitj Anchit Jain (anchitj) mentioned this pull request Apr 16, 2024
Closed
8 tasks
@anchitj Anchit Jain (anchitj) changed the title Handle underflow in rd_buf_write_remains Handle overflow in rd_buf_write_remains Apr 16, 2024
@anchitj Anchit Jain (anchitj) marked this pull request as ready for review April 16, 2024 12:17
@korchak-aleksandr
Copy link
Copy Markdown

Aleksandr Korchak (korchak-aleksandr) commented Apr 25, 2024

Hi Emanuele Sabellico (@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.

emasab
Emanuele Sabellico (emasab) requested changes May 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@emasab Emanuele Sabellico (emasab) left a comment •
edited
Loading

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.

Comment thread tests/0011-produce_batch.c Outdated
Comment thread src/rdbuf.h Outdated
Comment thread tests/0011-produce_batch.c Outdated
Comment thread tests/0011-produce_batch.c Outdated
@emasab
Copy link
Copy Markdown
Contributor

Emanuele Sabellico (emasab) commented May 13, 2024 •
edited
Loading

There also a problem in rd_buf_write_seek because of rd_buf_destroy_segment.

My proposed solution is to add a seg_erased to rd_segment_s and increase it when part of the segment is erased, then subtract seg_erased from rbuf_erased when a segment is removed in rd_buf_write_seek

@anchitj
Copy link
Copy Markdown
Contributor Author

Anchit Jain (anchitj) commented May 15, 2024

There also a problem in rd_buf_write_seek because of rd_buf_destroy_segment.

My proposed solution is to add a seg_erased to rd_segment_s and increase it when part of the segment is erased, then subtract seg_erased from rbuf_erased when a segment is removed in rd_buf_write_seek

Makes sense. I'll create a separate PR to handle that.

@emasab
Copy link
Copy Markdown
Contributor

Emanuele Sabellico (emasab) commented May 17, 2024

Anchit Jain (@anchitj) better to do it here, as it's basically the same thing that is missing

@anchitj Anchit Jain (anchitj) requested a review from a team as a code owner June 4, 2024 05:46
emasab
Emanuele Sabellico (emasab) requested changes Jun 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@emasab Emanuele Sabellico (emasab) left a 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

Comment thread src/rdbuf.c Outdated
Comment thread src/rdbuf.c Outdated
Comment thread tests/0011-produce_batch.c Outdated
Comment thread tests/0011-produce_batch.c Outdated
Comment thread tests/0011-produce_batch.c Outdated
@anchitj Anchit Jain (anchitj) requested a review from a team as a code owner June 10, 2024 10:02
emasab
Emanuele Sabellico (emasab) reviewed Jun 10, 2024
View reviewed changes
Comment thread CHANGELOG.md Outdated
emasab
Emanuele Sabellico (emasab) approved these changes Jun 10, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@emasab Emanuele Sabellico (emasab) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Anchit! Let's wait for the CI and then we can merge this.

@emasab Emanuele Sabellico (emasab) deleted the dev_rd_buf_write_remains branch June 11, 2024 09:02
@nkostoulas Nikos Kostoulas (nkostoulas) mentioned this pull request Aug 8, 2024
Open
8 tasks
@nkostoulas
Copy link
Copy Markdown

Nikos Kostoulas (nkostoulas) commented Aug 8, 2024 •
edited
Loading

Could this affect the confluent-kafka-go AdminClient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emasab Emanuele Sabellico (emasab) emasab approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anchitj @korchak-aleksandr @emasab @nkostoulas