Skip to content
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

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Conversation

anchitj
Copy link
Member

@anchitj anchitj commented Apr 16, 2024

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 anchitj changed the title Handle underflow in rd_buf_write_remains Handle overflow in rd_buf_write_remains Apr 16, 2024
@anchitj anchitj force-pushed the dev_rd_buf_write_remains branch from a1622dd to 66fa00b Compare April 16, 2024 11:26
@anchitj anchitj marked this pull request as ready for review April 16, 2024 12:17
@anchitj anchitj requested a review from emasab April 16, 2024 12:17
@korchak-aleksandr
Copy link

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.

Copy link
Contributor

@emasab 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, the source of the problem comes from a bug that happens previously, check the comment.

tests/0011-produce_batch.c Outdated Show resolved Hide resolved
src/rdbuf.h Outdated Show resolved Hide resolved
tests/0011-produce_batch.c Outdated Show resolved Hide resolved
tests/0011-produce_batch.c Outdated Show resolved Hide resolved
@emasab
Copy link
Contributor

emasab commented May 13, 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

@anchitj
Copy link
Member Author

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
Contributor

emasab commented May 17, 2024

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

@anchitj anchitj requested a review from a team as a code owner June 4, 2024 05:46
@anchitj anchitj force-pushed the dev_rd_buf_write_remains branch from e5886a9 to 4071f8b Compare June 4, 2024 05:49
@anchitj anchitj requested a review from emasab June 4, 2024 05:56
@anchitj anchitj force-pushed the dev_rd_buf_write_remains branch from 5bb225f to daab8b9 Compare June 4, 2024 05:58
@anchitj anchitj force-pushed the dev_rd_buf_write_remains branch from daab8b9 to 396f7cc Compare June 4, 2024 06:03
Copy link
Contributor

@emasab 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

src/rdbuf.c Outdated Show resolved Hide resolved
src/rdbuf.c Outdated Show resolved Hide resolved
tests/0011-produce_batch.c Outdated Show resolved Hide resolved
tests/0011-produce_batch.c Outdated Show resolved Hide resolved
tests/0011-produce_batch.c Outdated Show resolved Hide resolved
@anchitj anchitj requested a review from a team as a code owner June 10, 2024 10:02
@anchitj anchitj force-pushed the dev_rd_buf_write_remains branch from 4c0d964 to 125846c Compare June 10, 2024 10:24
@anchitj anchitj force-pushed the dev_rd_buf_write_remains branch from 125846c to 4842503 Compare June 10, 2024 10:27
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@emasab 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 emasab merged commit 8242cc9 into master Jun 11, 2024
2 checks passed
@emasab emasab deleted the dev_rd_buf_write_remains branch June 11, 2024 09:02
@nkostoulas
Copy link

nkostoulas commented Aug 8, 2024

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants