fix(kafka): bound telemetry and Produce response arrays by shoemoney · Pull Request #5580 · confluentinc/librdkafka

Jeremy Schoemaker (shoemoney) · 2026-08-25T20:53:37Z

Malformed GetTelemetrySubscriptions array counts can reach allocation as negative or unbounded sizes. The Produce v8+ RecordErrors count also allowed any positive value, even when it exceeded the number of records sent.

This bounds accepted compression types at 1000, rejects nullable counts for both telemetry arrays, and bounds Produce record errors by the request batch size. Telemetry allocation ownership is established before reading array elements so a truncated response is cleaned up correctly.

Validation on macOS arm64:

Produce v8 and v9 parser tests cover counts -1, 0, the request batch size, and batch size + 1. The over-limit case returned success before the upper bound and now returns BAD_MSG.
All 25 built-in unit suites pass, including the telemetry count regressions.
Build and clang-format 18 checks pass after rebasing onto current master.

Configured with SSL, SASL and curl disabled. No live Kafka cluster tests were run.

confluent-cla-assistant · 2026-08-25T20:53:49Z

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

Copilot

Pull request overview

This PR hardens rd_kafka_handle_GetTelemetrySubscriptions() response parsing to prevent a pre-auth SIGSEGV by bounding the decoded array count, rejecting nullable/negative array counts, and handling allocation failures during parsing.

Changes:

Cap AcceptedCompressionTypes array count parsing at 1000 (aligning with other capped array parsing in the file).
Reject negative (nullable) array counts for AcceptedCompressionTypes via rd_kafka_buf_parse_fail().
Add an allocation failure check around rd_calloc() for accepted_compression_types.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

                rk->rk_telemetry.accepted_compression_types_cnt = arraycnt;
                rk->rk_telemetry.accepted_compression_types =
                    rd_calloc(arraycnt, sizeof(rd_kafka_compression_t));
+                if (!rk->rk_telemetry.accepted_compression_types)
+                        rd_kafka_buf_parse_fail(


+        rd_kafka_buf_read_arraycnt(rkbuf, &arraycnt, 1000);
+        if (arraycnt < 0)
+                rd_kafka_buf_parse_fail(rkbuf,
+                                        "ApiArrayCnt %" PRId32 " out of range",
+                                        arraycnt);


Jeremy Schoemaker (shoemoney) · 2026-08-27T20:43:16Z

Addressed in 5214a83: allocation-failure path no longer leaves accepted_compression_types_cnt set with a NULL pointer (temp pointer assigned after alloc succeeds, same for the arraycnt==0 branch), and added unit tests for arraycnt -1 and >1000 asserting rkbuf_err is set to RD_KAFKA_RESP_ERR__BAD_MSG with no crash.

Kaushik Raina (k-raina)

Thanks for PR Jeremy Schoemaker (@shoemoney)
I have left few comments

Kaushik Raina (k-raina) · 2026-09-11T14:07:27Z


        rd_kafka_buf_read_arraycnt(rkbuf, &arraycnt, 1000);

        if (arraycnt) {


Do we need to check for negative error count here?
Similiar to https://github.com/confluentinc/librdkafka/pull/5580/changes#diff-81d5315dc871bcfe090e0b28790b8e6ff1b929209cd1d9b3bf03d3d96d7e89faR6831-R6832

Kaushik Raina (k-raina) · 2026-09-11T14:10:59Z

                }
+
+                rk->rk_telemetry.accepted_compression_types_cnt = arraycnt;
+                rk->rk_telemetry.accepted_compression_types =


Should this memory be freed in case of https://github.com/confluentinc/librdkafka/pull/5580/changes#diff-81d5315dc871bcfe090e0b28790b8e6ff1b929209cd1d9b3bf03d3d96d7e89faR6845 fails?

@k-raina

…raycnt Two findings from @k-raina's review on confluentinc#5580. The temp pointer introduced last round leaks. rd_kafka_buf_read_i8 in the fill loop expands to rd_kafka_buf_parse_fail on a short buffer, which does goto err_parse, and that label has no free; the array only reached rk_telemetry after the loop, so the sole owner went out of scope. Assigning the pointer and count immediately after a successful rd_calloc puts it back under rd_kafka_telemetry_clear()'s free while still never pairing a non-zero _cnt with a NULL pointer, which is what the temp pointer was for. The RequestedMetrics arraycnt had no negative guard. rd_kafka_buf_read_arraycnt only rejects < -1 and > maxval, so a nullable -1 survives, and -1 is truthy, so the following if (arraycnt) reached rd_calloc(-1, sizeof(char *)). Same shape as the AcceptedCompressionTypes count this PR already guards, same fix.

Jeremy Schoemaker (shoemoney) · 2026-09-11T14:49:12Z

All four are right. Three are fixed in 3432fbd6 and dce1ca7b; the third is a question I can answer from the source.

Should this memory be freed if the read at :6845 fails? Yes, and it was a leak I introduced last round. rd_kafka_buf_read_i8 expands to rd_kafka_buf_parse_fail on a short buffer, which is goto err_parse, and that label has no rd_free. Because the temp pointer only reached rk_telemetry after the loop, the sole owner went out of scope on that path. Fixed by assigning the pointer and count immediately after a successful rd_calloc, so rd_kafka_telemetry_clear() owns it during the loop. That still satisfies what the temp pointer was for, since the assignment happens only after the NULL check, so _cnt is never set alongside a NULL pointer.

Do we need a negative check at :6888? Yes, same bug. rd_kafka_buf_read_arraycnt only rejects < -1 and > maxval, so a nullable -1 survives it, and -1 is truthy, so if (arraycnt) then reaches rd_calloc(-1, sizeof(char *)). Guard added.

Should we make the same change at :3988? Yes, and that call passes -1 for maxval so nothing bounds it at all. dce1ca7b guards RecordErrorsCnt the same way. I kept it as its own commit because the Produce v8+ response is a different path from the GetTelemetrySubscriptions work this PR is titled for, so drop it or split it out if you would rather keep the PR to telemetry. Happy to retitle if you want it included.

Will rd_calloc abort on allocation failure? Only where asserts are live. rd_calloc is rd.h:139 and calls rd_assert(p), which on POSIX is rdposix.h:159, #define rd_assert(EXPR) assert(EXPR). Standard assert compiles to nothing under NDEBUG, so in such a build rd_calloc returns calloc's NULL rather than aborting. librdkafka's own build does not define NDEBUG, but as a library the flags often come from the consumer, and CMake's Release configuration adds -DNDEBUG by default. So the NULL check is unreachable in a debug build and live in a release one, which is why I left it in. If you would rather these paths rely on the allocator wrapper's documented premise in rd.h:134-138 that a failed small allocation is unrecoverable, say so and I will drop both checks and the temp pointer with them.

Jeremy Schoemaker (shoemoney) · 2026-09-11T15:21:26Z

Since your :3988 pointer was "are there others", I swept the rest rather than leave it to guesswork. Across all 82 rd_kafka_buf_read_arraycnt call sites in src/*.c, looking for a count that reaches rd_calloc/rd_malloc as a length without a negative guard first: zero remain.

The sweep is only worth quoting because it detects the known positives. Run against HEAD~2, before the two guards in this PR, it reports exactly the two sites you raised and nothing else:

line 3988  var RecordErrorsCnt  maxval -1
line 6886  var arraycnt         maxval 1000

Against the current head it reports none.

Two limits worth stating: it only looks 8 lines past the read, and only for rd_calloc/rd_malloc with the count as an argument, so a count that is stashed in a struct and used as a length further away would not be caught. The 5 sites that pass -1 for maxval are also uncapped by design, and I have not touched the other 4, since capping them is a policy question rather than a crash fix.

Kaushik Raina (k-raina)

Thanks Jeremy Schoemaker (@shoemoney) for addressing comments. Overall PR looks good,
Provided one comment.

Kaushik Raina (k-raina) · 2026-09-15T13:26:57Z

@@ -3986,6 +3986,10 @@ rd_kafka_handle_Produce_parse(rd_kafka_broker_t *rkb,
                int32_t RecordErrorsCnt;
                rd_kafkap_str_t ErrorMessage;
                rd_kafka_buf_read_arraycnt(rkbuf, &RecordErrorsCnt, -1);


Should we update &RecordErrorsCnt also to upper limit?

Yes. 4a0bd30c caps RecordErrorsCnt at the number of records in the request batch, so the bound follows the actual request instead of imposing a fixed limit on valid batches. The negative-count guard remains.

Added Produce v8/v9 parser tests for counts -1, 0, the batch size and batch size + 1. The over-limit case failed before the fix and passes now. Rebased onto current master; the build, all 25 built-in unit suites and clang-format 18 checks pass locally (SSL/SASL/curl disabled; no live Kafka cluster tests).

Kaushik Raina (k-raina) · 2026-09-15T13:31:18Z

/sem-approve

@k-raina

…raycnt Two findings from @k-raina's review on confluentinc#5580. The temp pointer introduced last round leaks. rd_kafka_buf_read_i8 in the fill loop expands to rd_kafka_buf_parse_fail on a short buffer, which does goto err_parse, and that label has no free; the array only reached rk_telemetry after the loop, so the sole owner went out of scope. Assigning the pointer and count immediately after a successful rd_calloc puts it back under rd_kafka_telemetry_clear()'s free while still never pairing a non-zero _cnt with a NULL pointer, which is what the temp pointer was for. The RequestedMetrics arraycnt had no negative guard. rd_kafka_buf_read_arraycnt only rejects < -1 and > maxval, so a nullable -1 survives, and -1 is truthy, so the following if (arraycnt) reached rd_calloc(-1, sizeof(char *)). Same shape as the AcceptedCompressionTypes count this PR already guards, same fix.

Kaushik Raina (k-raina) · 2026-09-17T08:57:02Z

/sem-approve

Jeremy Schoemaker (shoemoney) · 2026-09-19T04:09:31Z

The CLA is signed and fully executed on Confluent's side as of 2026-09-19 02:02 UTC — cla-status-check is now green, alongside ci/semaphoreci/pr: build-test-release and r2-agents/approval-gate. Apologies for the delay on that; it was the only thing standing between your two /sem-approve runs and a merge. Nothing in the diff has changed since your review. Ready when you are.

Kaushik Raina (k-raina)

Jeremy Schoemaker (@shoemoney) Thanks for changes.

Left one comment on checks for requested_metrics & record_errors.

Kaushik Raina (k-raina) · 2026-09-21T12:00:53Z

Should we have change for NULL-check the alloc and Temp-pointer ownership-timing fix to requested_metrics & record_errors too, if applicable?

Jeremy Schoemaker (shoemoney) · 2026-09-21T12:08:07Z

Good catch. Applied to both, though they needed slightly different treatment.

requested_metrics had the same bug accepted_compression_types had: the count was set before the allocation, so a failed alloc would leave a non-zero requested_metrics_cnt beside a NULL pointer. Switched it to the temp-pointer, NULL-check, assign-count-last pattern.

record_errors already assigned the count after the alloc, so that particular ordering failure was not present, but it had no NULL check at all. Added the same temp-pointer pattern for consistency. Worth noting that rd_calloc asserts internally on OOM rather than returning NULL, so these checks are defensive in the same way the existing accepted_compression_types check is.

Ran the full in-process unit suite after the change, with the request and telemetry suites specifically exercising both parsers. All green, and clang-format 18 is clean.

Kaushik Raina (k-raina) · 2026-09-22T06:58:00Z

/sem-approve

Kaushik Raina (k-raina) · 2026-09-22T07:03:31Z

+  memory (#5580, @shoemoney).
+
+* Guard the Produce response `record_errors` allocation, and the
+  GetTelemetrySubscriptions `requested_metrics` allocation, against


nit: we should mention accepted_compression_types also.

Added accepted_compression_types to the changelog entry in 6921423f. Squashed and rebased onto c58a8dc4 as CONTRIBUTING requests.

Both clean-base and updated builds and all 25 built-in unit suites passed locally; clang-format 18 passed. SSL/SASL/curl were disabled; broker integration suites were not run. The new head has no code-test CI result yet.

Validate telemetry array counts and Produce record-error counts before allocation. Guard allocation failures and establish cleanup ownership before parsing array elements. Add parser regression coverage.

Kaushik Raina (k-raina) · 2026-09-22T10:22:17Z

/sem-approve

Kaushik Raina (k-raina)

Thanks Jeremy Schoemaker (@shoemoney) for PR and congratulations on your first contribution to this repo.
We will wait for second reviewer to approve, before merging PR.

Ankith L (Ankith-Confluent)

Thanks for the fix Jeremy Schoemaker (@shoemoney)!
One thing on the CHANGELOG, the entries are under v2.15.1, which has already been released. Could you move them to a new v2.16.1 section at the top of the file.

Validate telemetry array counts and Produce record-error counts before allocation. Guard allocation failures and establish cleanup ownership before parsing array elements. Add parser regression coverage.

Jeremy Schoemaker (shoemoney) · 2026-09-29T07:06:54Z

Moved both entries unchanged to a new v2.16.1 section at the top in b7b915d0, addressing your review. Squashed and rebased onto current master as CONTRIBUTING requires; the source patch is unchanged.

Both clean master and the updated PR build and pass all 25 built-in unit suites. Changelog placement/text assertions, git diff --check, and clang-format 18 also pass. SSL/SASL/curl were disabled; no live Kafka broker integration suites were run.

Ankith L (Ankith-Confluent)

Thanks for the PR Jeremy Schoemaker (@shoemoney)
LGTM!

Validate telemetry array counts and Produce record-error counts before allocation. Guard allocation failures and establish cleanup ownership before parsing array elements. Add parser regression coverage.

Kaushik Raina (k-raina) · 2026-10-06T17:07:23Z

/sem-approve

Jeremy Schoemaker (shoemoney) requested a review from a team as a code owner August 25, 2026 20:53

Copilot AI lite review requested due to automatic review settings August 25, 2026 20:53

Copilot started reviewing on behalf of Jeremy Schoemaker (shoemoney) August 25, 2026 20:54 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Kaushik Raina (k-raina) reviewed Sep 11, 2026

View reviewed changes

Kaushik Raina (k-raina) reviewed Sep 15, 2026

View reviewed changes

Jeremy Schoemaker (shoemoney) force-pushed the fix/librdkafka-telemetry-array branch from dce1ca7 to 4a0bd30 Compare September 16, 2026 22:47

Jeremy Schoemaker (shoemoney) changed the title ~~fix(kafka): cap GetTelemetrySubscriptions array and check alloc~~ fix(kafka): bound telemetry and Produce response arrays Sep 16, 2026

Kaushik Raina (k-raina) reviewed Sep 21, 2026

View reviewed changes

Kaushik Raina (k-raina) reviewed Sep 22, 2026

View reviewed changes

Jeremy Schoemaker (shoemoney) force-pushed the fix/librdkafka-telemetry-array branch from f80ed91 to 6921423 Compare September 22, 2026 07:48

Kaushik Raina (k-raina) approved these changes Sep 23, 2026

View reviewed changes

Kaushik Raina (k-raina) requested a review from Ankith L (Ankith-Confluent) September 23, 2026 05:09

Ankith L (Ankith-Confluent) requested changes Sep 29, 2026

View reviewed changes

Jeremy Schoemaker (shoemoney) force-pushed the fix/librdkafka-telemetry-array branch from 6921423 to b7b915d Compare September 29, 2026 07:06

Ankith L (Ankith-Confluent) approved these changes Oct 1, 2026

View reviewed changes

Ojasva Jain (ojasvajain) reviewed Oct 1, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

request: guard response array counts and allocations (confluentinc#5580)

70b6542

Validate telemetry array counts and Produce record-error counts before allocation. Guard allocation failures and establish cleanup ownership before parsing array elements. Add parser regression coverage.

Jeremy Schoemaker (shoemoney) force-pushed the fix/librdkafka-telemetry-array branch from b7b915d to 70b6542 Compare October 1, 2026 11:06


		rd_kafka_buf_read_arraycnt(rkbuf, &arraycnt, 1000);

		if (arraycnt) {

Conversation

Jeremy Schoemaker (shoemoney) commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

confluent-cla-assistant Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Jeremy Schoemaker (shoemoney) commented Aug 27, 2026

Uh oh!

Kaushik Raina (k-raina) left a comment

Choose a reason for hiding this comment

Uh oh!

Kaushik Raina (k-raina) Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Kaushik Raina (k-raina) Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Jeremy Schoemaker (shoemoney) commented Sep 11, 2026

Uh oh!

Jeremy Schoemaker (shoemoney) commented Sep 11, 2026

Uh oh!

Kaushik Raina (k-raina) left a comment

Choose a reason for hiding this comment

Uh oh!

Kaushik Raina (k-raina) Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Jeremy Schoemaker (shoemoney) Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Kaushik Raina (k-raina) commented Sep 15, 2026

Uh oh!

Kaushik Raina (k-raina) commented Sep 17, 2026

Uh oh!

Jeremy Schoemaker (shoemoney) commented Sep 19, 2026

Uh oh!

Kaushik Raina (k-raina) left a comment

Choose a reason for hiding this comment

Uh oh!

Kaushik Raina (k-raina) Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

Jeremy Schoemaker (shoemoney) commented Sep 21, 2026

Uh oh!

Kaushik Raina (k-raina) commented Sep 22, 2026

Uh oh!

Kaushik Raina (k-raina) Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Jeremy Schoemaker (shoemoney) Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Kaushik Raina (k-raina) commented Sep 22, 2026

Uh oh!

Kaushik Raina (k-raina) left a comment

Choose a reason for hiding this comment

Uh oh!

Ankith L (Ankith-Confluent) left a comment

Choose a reason for hiding this comment

Uh oh!

Jeremy Schoemaker (shoemoney) commented Sep 29, 2026

Uh oh!

Ankith L (Ankith-Confluent) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kaushik Raina (k-raina) commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Jeremy Schoemaker (shoemoney) commented Aug 25, 2026 •

edited

Loading

confluent-cla-assistant Bot commented Aug 25, 2026 •

edited

Loading