confluentinc / confluentinc/librdkafka

Use-after-free when rd_kafka_new returns RD_KAFKA_RESP_ERR__CRIT_SYS_RESOURCE

Open
#4,100 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
1k
Forks
3.3k
Avg merge
5d 12h
Merged PRs (30d)
6

Description

We're hitting [this codepath](https://github.com/edenhill/librdkafka/blob/3b50e1e/src/rdkafka.c#L2512-L2537) in `rd_kafka_new`. This is in a pathologically large integration test where (1) all Kafka brokers are unreachable by design, and (2) we're running lots of VMs (7x 16-core VMs all simulated on the same single 34-core physical machine) such that "`the OS is not scheduling the background threads`" is actually a highly likely scenario for us.

```
if (rd_kafka_init_wait(rk, 60 * 1000) != 0) {
/* This should never happen unless there is a bug
* or the OS is not scheduling the background threads.
* Either case there is no point in handling this gracefully
* in the current state since the thread joins are likely
* to hang as well. */
mtx_lock(&rk->rk_init_lock);
rd_kafka_log(rk, LOG_CRIT, "INIT",
"Failed to initialize %s: "
"%d background thread(s) did not initialize "
"within 60 seconds",
rk->rk_name, rk->rk_init_wait_cnt);
if (errstr)
rd_snprintf(errstr, errstr_size,
"Timed out waiting for "
"%d background thread(s) to initialize",
rk->rk_init_wait_cnt);
mtx_unlock(&rk->rk_init_lock);

rd_kafka_set_last_error(RD_KAFKA_RESP_ERR__CRIT_SYS_RESOURCE,
EDEADLK);
return NULL;
}
```

Looking at this code, I see that it calls `rd_kafka_log(rk, ...)` and then returns null without destroying the `rk` at all. This seems like a memory leak, right? That's bad, but not fatal.
What's _fatal_ is that this codepath leads to a use-after-free for us, because we assume that when `rd_kafka_new` fails, we should clean up the `rk_conf` that we passed to it. In other words, we do exactly what's documented in [INTRODUCTION.md](https://github.com/edenhill/librdkafka/blob/3b50e1e/INTRODUCTION.md?plain=1#L857-L863):

```
rk = rd_kafka_new(RD_KAFKA_PRODUCER, conf, errstr, sizeof(errstr));
if (!rk) {
rd_kafka_conf_destroy(rk); // [sic]; actually this should say `conf` and I'll submit a PR for that in a minute
fail("Failed to create producer: %s\n", errstr);
}
/* Note: librdkafka takes ownership of the conf object on success */
```

But along this specific codepath, `rd_kafka_new` fails and returns NULL _and yet there are still background threads running,_ and those background threads are going to try to access the function-pointer callbacks registered in that `conf` object. If we `rd_kafka_conf_destroy(conf)` on failure, then we have a use-after-free situation on those function pointers, and the symptom is generally a segfault.

#2820 might be related; it mentions this same codepath.

My ideal outcome here would be a simple rule like "When `rd_kafka_new` fails by returning NULL, it _always_ relinquishes ownership of the `rk_conf`, so you can feel free to destroy it at your leisure," or "Whenever `rd_kafka_new` is called, it _always_ takes ownership of the `rk_conf`, so you should actually _never_ destroy the `rk_conf` after that point because now it belongs to the library." (The former is what I always _thought_ the rule was. The latter would be awesome, but probably can't be achieved in practice because it would cause double-free bugs for all existing code.)

The second-best outcome would be if you could give us a simple distinguishing rule for when we should destroy the `rk_conf` and when we shouldn't. For example, "When `rd_kafka_new` fails by returning NULL, it relinquishes ownership of the `rk_conf` if and only if `rd_kafka_last_error() != RD_KAFKA_RESP_ERR__CRIT_SYS_RESOURCE`. Client code should check for that situation and avoid destroying the `rk_conf` if `rd_kafka_last_error() == RD_KAFKA_RESP_ERR__CRIT_SYS_RESOURCE`." (However, notice that that rule is also wrong, because of [this codepath](https://github.com/edenhill/librdkafka/blob/3b50e1e/src/rdkafka.c#L2466-L2485).)

Checklist
=========

Please provide the following information:

- [x] librdkafka version (release number or git tag): `v1.8.2`, `v1.9.2`
- [x] Apache Kafka version: `2.6.3`, I think
- [x] librdkafka client configuration: Various, but one example is a CONSUMER with the following non-default properties: {group.id => "redacted", metadata.broker.list => "172.31.1.6:9093", enable.partition.eof => "true", security.protocol => "ssl", ssl.key.location => "/var/redacted1.key", ssl.certificate.location => "/var/redacted2.pem", ssl.ca.location => "/var/redacted3.pem"}
- [x] Operating system: RHEL 7, also probably any OS

Contributor guide

Open the contributing guide

Research direction

Start in src/rdkafka.c at the rd_kafka_new failure paths around the linked lines, then compare the ownership wording in INTRODUCTION.md around lines 857-863 and the related discussion in #2820. Trace the timeout path and background-thread access to conf callbacks; done means the failure behavior is safe and the documented conf ownership rule is unambiguous.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend-api-design
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.