Skip to content

[KIP-714] Fix: Metric name from the protocol is read outside boundaries #5105

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

Merged
merged 2 commits into from
Jun 9, 2025

Conversation

emasab
Copy link
Contributor

@emasab emasab commented Jun 4, 2025

Closes #5102

Avoid copy outside boundaries when reading metric names in telemetry subscription. It can cause that some metrics aren't matched.

@Copilot Copilot AI review requested due to automatic review settings June 4, 2025 17:37
@emasab emasab requested a review from a team as a code owner June 4, 2025 17:37
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures metric names in telemetry subscriptions are duplicated within their defined length boundaries and adds a unit test for the related handler.

  • Replace unbounded rd_strdup with RD_KAFKAP_STR_DUP to avoid out-of-bounds copies of metric names.
  • Introduce unittest_handle_GetTelemetrySubscriptions to validate subscription parsing.
  • Update CHANGELOG.md with the new fix under v2.11.0.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/rdkafka_request.c Swap rd_strdup for RD_KAFKAP_STR_DUP and add a unit test case.
CHANGELOG.md Add v2.11.0 entry describing the boundary-copy fix.
Comments suppressed due to low confidence (3)

CHANGELOG.md:5

  • Replace the placeholder "(#)" with the actual PR or issue number to create a proper link in the changelog.
* Avoid copy outside boundaries when reading metric names in telemetry

src/rdkafka_request.c:7001

  • The assertion message uses "compression type" in singular for a count of two; consider changing it to "compression types" for clarity.
RD_UT_ASSERT(rk->rk_telemetry.accepted_compression_types_cnt == 2,

src/rdkafka_request.c:6987

  • Consider adding a test case where a metric name exceeds its declared length to verify RD_KAFKAP_STR_DUP properly truncates or rejects out-of-bounds input.
rd_kafka_buf_write_arraycnt(rkbuf, 2); /* #RequestedMetrics */

@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_kip714_fix_metric_name_outside_boundaries branch 2 times, most recently from e195d04 to 37bf3c5 Compare June 4, 2025 17:42
@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_kip714_fix_metric_name_outside_boundaries branch from 37bf3c5 to dd5b1df Compare June 9, 2025 13:57
@emasab emasab merged commit e3ae6e6 into master Jun 9, 2025
2 checks passed
@emasab emasab deleted the dev_kip714_fix_metric_name_outside_boundaries branch June 9, 2025 16:42
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.

[KIP-714] Metric name from the protocol is read outside boundaries
2 participants