ctdb_pkt_recv_recv regression (Re: [SCM] Samba Shared Repository - branch master updated)

[Stefan Metzmacher]

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>