ctdb_pkt_recv_recv regression (Re: [SCM] Samba Shared Repository - branch master updated)
Stefan Metzmacher
metze at samba.org
Wed Aug 28 11:26:27 UTC 2019
Hi Noel,
can you please have a look at the mail I wrote a month ago,
I think we need to fix that up.
Thanks!
metze
Am 26.07.19 um 21:31 schrieb Stefan Metzmacher:
> Hi Noel,
>
> thanks for fixing clang warnings, but
> I fear the following changes are wrong.
>
> Can we have SMB_ASSERT(hdr != NULL); after the if statements with a
> comment explaining why it's there.
>
>> diff --git a/source3/lib/ctdbd_conn.c b/source3/lib/ctdbd_conn.c
>> index d5fac572d3c..8a8fbec4552 100644
>> --- a/source3/lib/ctdbd_conn.c
>> +++ b/source3/lib/ctdbd_conn.c
>> @@ -398,15 +398,16 @@ static int ctdb_read_packet(int fd, int timeout, TALLOC_CTX *mem_ctx,
>> static int ctdb_read_req(struct ctdbd_connection *conn, uint32_t reqid,
>> TALLOC_CTX *mem_ctx, struct ctdb_req_header **result)
>> {
>> - struct ctdb_req_header *hdr;
>> + struct ctdb_req_header *hdr = NULL;
>> int ret;
>>
>> next_pkt:
>>
>> ret = ctdb_read_packet(conn->fd, conn->timeout, mem_ctx, &hdr);
>> - if (ret != 0) {
>> + if (hdr == NULL || ret != 0) {
>> DBG_ERR("ctdb_read_packet failed: %s\n", strerror(ret));
>> cluster_fatal("failed to read data from ctdbd\n");
>> + return -1;
>> }
>
> This is not strictly a bug, but it's makes the code really unclear.
>
>> DEBUG(11, ("Received ctdb packet\n"));
>> @@ -1038,7 +1039,7 @@ int ctdbd_traverse(struct ctdbd_connection *conn, uint32_t db_id,
>> int ret;
>> TDB_DATA key, data;
>> struct ctdb_traverse_start t;
>> - int32_t cstatus;
>> + int32_t cstatus = 0;
>>
>> if (ctdbd_conn_has_async_reqs(conn)) {
>> /*
>> @@ -1945,7 +1946,7 @@ static void ctdbd_parse_done(struct tevent_req *subreq)
>>
>> ret = ctdb_pkt_recv_recv(subreq, state, &hdr);
>> TALLOC_FREE(subreq);
>> - if (tevent_req_error(req, ret)) {
>> + if ((hdr == NULL) || tevent_req_error(req, ret)) {
>> DBG_ERR("ctdb_pkt_recv_recv returned %s\n", strerror(ret));
>> return;
>> }
>
> This is actually really a bug, in case ctdb_pkt_recv_recv fails
> we'll never run tevent_req_error() and the callers callback function,
> so it will just hang as hdr is always NULL if ret is not 0.
>
> Can you revert this and add SMB_ASSERT().
>
> Thanks!
> metze
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190828/dd712326/signature.sig>
More information about the samba-technical
mailing list