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

[Swen Schillig]

On Mon, 2018-03-19 at 14:35 +1100, Martin Schwenke via samba-technical
wrote:
> 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're right.
I need to add that one check back.

> 
> 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,	(__V
> > A_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

Removed the cleanup.

Please have a look again.

Thanks in advance.

Cheers Swen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb_daemon_read_cb.patch
Type: text/x-patch
Size: 2996 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180319/2e8f2893/ctdb_daemon_read_cb.bin>