CTDB: Small patch set fixing memory leak and other things for ctdb_daemon_read_cb

[Martin Schwenke]

On Fri, 16 Mar 2018 11:50:27 +0100, Swen Schillig <swen at vnet.ibm.com>
wrote:

> On Fri, 2018-03-16 at 20:46 +1100, Martin Schwenke wrote:
> > On Tue, 13 Mar 2018 10:06:56 +0100, Swen Schillig via samba-technical
> > <samba-technical at lists.samba.org> wrote:
> >   
> > > The attached file contains 3 small patches fixing a memory leak,
> > > removes not require checks and is cleaning up the code for column
> > > length, function call formatting and using the right logging
> > > macros.  
> > 
> > For the 1st commit, the commit message says:
> > 
> >   Within ctdb_daemon_read_cb the provided data is checked for sanity,
> >   e.g. correct size and content. This is not required because it was
> >   done already by the caller.
> > 
> > You might as well name the caller.
> > 
> > I assume you mean queue_process() in ctdb/common/ctdb_io.c.  However,
> > I
> > don't see where that function confirms that it has read a full
> > header.  
> That is the pkt_size,
> we do 
> 	pkt_size = *(uint32_t *)queue->buffer.data;
> and
>  	if (queue->buffer.length < pkt_size) {
> 		...
> 		return;
> 	}
> in queue_process before we're calling the callback.
> which is the same as 
>  	hdr = (struct ctdb_req_header *)data;
> 	if (cnt != hdr->length) 
> having struct ctdb_req_header as
> 	struct ctdb_req_header {
> 		uint32_t length;
> 		...
> 	}

It isn't the same.  :-(

You are correct in saying that queue_process() ensures that it always
passes a complete packet to the callback, based on the packet length
at the beginning of the packet.  queue_process() doesn't check that the
packet length is reasonable.

I think you're assuming that any packet being read is long enough to
contain a valid header.  What happens if something sends a short packet
that is less than the length of a header?  It is true that this
shouldn't happen.  However, if it does happen (due to a bug elsewhere)
then ctdb_daemon_read_cb() casts the short packet to a header struct
and potentially accesses fields beyond the end of the packet.

So, I don't think you can remove that check without some very careful
thinking and justification. 

> > In the 2nd commit, since the debug message is very generic and is
> > meant
> > for debugging, please use DBG_DEBUG(), since this logs the function
> > name, instead of D_DEBUG().  
> I only learned recently, that I should replace those "old-fashioned"
> macros by the newer-/shorter-ones as they do include the name already.
> ...and if you're looking at 
> 
> #define D_DEBUG(...)		DEBUG(DBGLVL_DEBUG,	(__VA_ARGS__))
> and 
> #define DEBUG( level, body ) \
>   (void)( ((level) <= MAX_DEBUG_LEVEL) && \
> 	  unlikely(DEBUGLEVEL_CLASS[ DBGC_CLASS ] >= (level))		\
>        && (dbghdrclass( level, DBGC_CLASS, __location__, __FUNCTION__ )) \
>        && (dbgtext body) )
> 
> this seems to be the case, right ?

Please read dbghdrclass() and ctdb/common/logging.c.

peace & happiness,
martin