Fix segfault with describe_topics and flaky connection #1692

lpsinger · 2023-12-15T22:46:12Z

If you call describe_topics on a flaky connection, sometimes the admin client reply has the host set to a null pointer. When this occurs, instead of segfaulting, report the host as None.

cla-assistant · 2023-12-15T22:46:26Z

All committers have signed the CLA.

cla-assistant · 2023-12-15T22:46:27Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

pranavrth · 2024-03-08T08:49:46Z

Thanks for the PR.

Even though the changes seems correct, I think we should not change the base implementation which is being used in alot of APIs. Some of the values of other APIs must not be NULL. I think its better to handle the scenario in this way - https://github.com/confluentinc/confluent-kafka-python/blob/master/src/confluent_kafka/src/Admin.c#L3198-L3201.

I am also checking if it should be possible to get the host information as NULL or should we handle it as an error instead. But in the meantime you can rebase the branch and do the change mentioned above if you have time. Otherwise, you can wait for my revert.

lpsinger · 2024-03-20T17:33:28Z

But in the meantime you can rebase the branch and do the change mentioned above if you have time.

Done.

pranavrth

Provided minor comments.

Please update the CHANGELOG.md as well with the entry for the fix.

src/confluent_kafka/src/confluent_kafka.c

pranavrth · 2024-04-24T09:09:07Z

src/confluent_kafka/src/Admin.c

+        if (rd_kafka_AclBinding_host(c_acl_binding)) {
+                cfl_PyDict_SetString(kwargs, "host",
+                                        rd_kafka_AclBinding_host(c_acl_binding));
+        } else {
+                cfl_PyObject_SetItemString(kwargs, "host", Py_None);
+        }


Please revert this change:

This change is not related to describe_topic

We can't reproduce this

It shouldn't be null as per the protocol

Not nullable too in DescribeAclsResponse and DeleteAclsResponse

https://github.com/apache/kafka/blob/d65d8867983e54a36178c3a1227b68c8a468d415/clients/src/main/resources/common/message/DescribeAclsResponse.json#L45
https://github.com/apache/kafka/blob/d65d8867983e54a36178c3a1227b68c8a468d415/clients/src/main/resources/common/message/DeleteAclsResponse.json#L49

pranavrth · 2024-04-24T09:09:28Z

src/confluent_kafka/src/Admin.c

+        if (rd_kafka_MemberDescription_host(c_member)) {
+                cfl_PyDict_SetString(kwargs,
+                                "host",
+                                rd_kafka_MemberDescription_host(c_member));
+        } else {
+                cfl_PyObject_SetItemString(kwargs, "host", Py_None);
+        }


Please revert this change:

This change is not related to describe_topic

We can't reproduce this

It shouldn't be null as per the protocol

+1 : In this case it's the member host and is not nullable by the protocol

https://github.com/apache/kafka/blob/18f2589c1e087654c0cdd46d2473246f37f0b66a/clients/src/main/resources/common/message/DescribeGroupsResponse.json#L57

pranavrth · 2024-04-24T09:10:15Z

src/confluent_kafka/src/Metadata.c

+        if (c_broker.host) {
+                if (cfl_PyObject_SetString(broker, "host",
+                                                c_broker.host) == -1) {
+                        Py_DECREF(broker);
+                        return NULL;
+                }
+        } else {
+                if (cfl_PyObject_SetItemString(broker, "host",
+                                                Py_None) == -1) {
+                        Py_DECREF(broker);
+                        return NULL;
+                }


Please revert this change:

This function is used in deprecated API and will be removed in future versions

This change is not related to describe_topic

We can't reproduce this

It shouldn't be null as per the protocol

Are you sure about that? Ultimately this function is called by list_topics in Metadata.c which sounds like it may be related to the segfaults I reported.

Were you able to isolate those segfaults to the one bit of code that you do want to change, in confluent_kafka.c?

The host comes from the Metadata response in list_topics and is not nullable there
https://github.com/apache/kafka/blob/e8cd661becb08279702f1670bb8f1d816537adea/clients/src/main/resources/common/message/MetadataResponse.json#L54

otherwise it can come from list_group (deprecated) that, in librdkafka only, does a list + describe and in that case it's taken from the host of the coordinator that returned the DescribeGroup response here:
https://github.com/confluentinc/librdkafka/blob/c95d2ee950bda779c63843d376aab1d90ed8dfa9/src/rdkafka.c#L4739

pranavrth · 2024-04-24T09:16:39Z

/sem-approve

emasab · 2024-04-25T14:29:25Z

src/confluent_kafka/src/confluent_kafka.c

@@ -1620,7 +1620,10 @@ PyObject *c_Node_to_py(const rd_kafka_Node_t *c_node) {

        cfl_PyDict_SetInt(kwargs, "id", rd_kafka_Node_id(c_node));
        cfl_PyDict_SetInt(kwargs, "port", rd_kafka_Node_port(c_node));
-        cfl_PyDict_SetString(kwargs, "host", rd_kafka_Node_host(c_node));
+        if (rd_kafka_Node_host(c_node))


Now I have a doubt that the fix should be here in CKPy, because this uses the Metadata response too for the replicas and isrs and it uses the Metadata host here (not nullable)
https://github.com/apache/kafka/blob/e8cd661becb08279702f1670bb8f1d816537adea/clients/src/main/resources/common/message/MetadataResponse.json#L54
And broker metadata must be present for all the indexed brokers. Checking librdkafka code

OK, here it can be None because it's possible not all the brokers are returned from the Metadata call. I'm not sure if this expected from the broker because the documentation says "Each broker in the response". And in that case LK just sets the broker id here
https://github.com/confluentinc/librdkafka/blob/c95d2ee950bda779c63843d376aab1d90ed8dfa9/src/rdkafka_aux.c#L281

In the list_topics API, instead, only the broker id list is returned in the replicas and isrs fields so the broker list in ClusterMetadata only contains complete BrokerMetadata objects.

It seems it's the expected behavior. I've created a PR for changing the documentation here.
An update: in Go and .NET it doesn't happen because it either uses C.GoString or PtrToStringUTF8 that returns nil or null.

lpsinger · 2024-05-02T16:46:03Z

@emasab, what else do you want me to do with this?

emasab · 2024-05-03T07:50:43Z

@lpsinger after reviewing all the cases only in c_Node_to_py it's needed to check for NULL values because it takes the nodes from the Metadata request and some of then can be absent, so without host, when the broker is down and not listed there.

Could you leave only that change?

lpsinger · 2024-05-03T20:32:39Z

Could you leave only that change?

Done. Would you like me to squash the changes?

pranavrth · 2024-05-06T08:26:25Z

/sem-approve

src/confluent_kafka/src/confluent_kafka.c

pranavrth · 2024-05-07T08:44:09Z

/sem-approve

If you call describe_topics on a flaky connection, sometimes the admin client reply has the host set to a null pointer. When this occurs, instead of segfaulting, report the host as None.

This reverts commit 2627ecc.

Co-authored-by: Pranav Rathi <4427674+pranavrth@users.noreply.github.com>

pranavrth · 2024-05-07T11:01:45Z

/sem-approve

pranavrth

Thanks for the contribution. LGTM!

lpsinger · 2024-05-07T12:15:17Z

You're very welcome!

lpsinger requested a review from a team as a code owner December 15, 2023 22:46

lpsinger force-pushed the fix-segfault-on-admin-client-flaky-connection branch 3 times, most recently from f1d8c82 to c1c65e0 Compare December 15, 2023 23:05

lpsinger force-pushed the fix-segfault-on-admin-client-flaky-connection branch from c1c65e0 to 2627ecc Compare March 20, 2024 17:22

pranavrth requested changes Apr 24, 2024

View reviewed changes

emasab reviewed Apr 25, 2024

View reviewed changes

lpsinger requested a review from emasab May 3, 2024 20:32

pranavrth reviewed May 6, 2024

View reviewed changes

src/confluent_kafka/src/confluent_kafka.c Outdated Show resolved Hide resolved

lpsinger force-pushed the fix-segfault-on-admin-client-flaky-connection branch from e2d6f3e to cfd227c Compare May 6, 2024 13:57

lpsinger requested a review from pranavrth May 6, 2024 13:57

lpsinger and others added 5 commits May 7, 2024 16:31

Fix segfault with describe_topics and flaky connection

811167d

If you call describe_topics on a flaky connection, sometimes the admin client reply has the host set to a null pointer. When this occurs, instead of segfaulting, report the host as None.

Revert "Fix segfault with describe_topics and flaky connection"

01d91f0

This reverts commit 2627ecc.

Fix segfault with describe_topics and flaky connection

8b5e60d

Update src/confluent_kafka/src/confluent_kafka.c

d1ab445

Co-authored-by: Pranav Rathi <4427674+pranavrth@users.noreply.github.com>

Undo all changes except for the one in c_Node_to_py

6e48741

pranavrth force-pushed the fix-segfault-on-admin-client-flaky-connection branch from cfd227c to 6e48741 Compare May 7, 2024 11:01

pranavrth approved these changes May 7, 2024

View reviewed changes

emasab approved these changes May 7, 2024

View reviewed changes

pranavrth merged commit 5c4a652 into confluentinc:master May 7, 2024
1 of 2 checks passed

lpsinger deleted the fix-segfault-on-admin-client-flaky-connection branch May 7, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault with describe_topics and flaky connection #1692

Fix segfault with describe_topics and flaky connection #1692

lpsinger commented Dec 15, 2023

cla-assistant bot commented Dec 15, 2023 •

edited

Loading

cla-assistant bot commented Dec 15, 2023

pranavrth commented Mar 8, 2024

lpsinger commented Mar 20, 2024

pranavrth left a comment

pranavrth Apr 24, 2024

emasab Apr 25, 2024

pranavrth Apr 24, 2024

emasab Apr 25, 2024

pranavrth Apr 24, 2024

lpsinger Apr 24, 2024

emasab Apr 25, 2024 •

edited

Loading

pranavrth commented Apr 24, 2024

emasab Apr 25, 2024 •

edited

Loading

emasab Apr 25, 2024 •

edited

Loading

emasab Apr 25, 2024

lpsinger commented May 2, 2024

emasab commented May 3, 2024 •

edited

Loading

lpsinger commented May 3, 2024

pranavrth commented May 6, 2024

pranavrth commented May 7, 2024

pranavrth commented May 7, 2024

pranavrth left a comment

lpsinger commented May 7, 2024

Fix segfault with describe_topics and flaky connection #1692

Fix segfault with describe_topics and flaky connection #1692

Conversation

lpsinger commented Dec 15, 2023

cla-assistant bot commented Dec 15, 2023 • edited Loading

cla-assistant bot commented Dec 15, 2023

pranavrth commented Mar 8, 2024

lpsinger commented Mar 20, 2024

pranavrth left a comment

Choose a reason for hiding this comment

pranavrth Apr 24, 2024

Choose a reason for hiding this comment

emasab Apr 25, 2024

Choose a reason for hiding this comment

pranavrth Apr 24, 2024

Choose a reason for hiding this comment

emasab Apr 25, 2024

Choose a reason for hiding this comment

pranavrth Apr 24, 2024

Choose a reason for hiding this comment

lpsinger Apr 24, 2024

Choose a reason for hiding this comment

emasab Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

pranavrth commented Apr 24, 2024

emasab Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

emasab Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

emasab Apr 25, 2024

Choose a reason for hiding this comment

lpsinger commented May 2, 2024

emasab commented May 3, 2024 • edited Loading

lpsinger commented May 3, 2024

pranavrth commented May 6, 2024

pranavrth commented May 7, 2024

pranavrth commented May 7, 2024

pranavrth left a comment

Choose a reason for hiding this comment

lpsinger commented May 7, 2024

cla-assistant bot commented Dec 15, 2023 •

edited

Loading

emasab Apr 25, 2024 •

edited

Loading

emasab Apr 25, 2024 •

edited

Loading

emasab Apr 25, 2024 •

edited

Loading

emasab commented May 3, 2024 •

edited

Loading