[PATCH] ctdb-common: Set close-on-exec when creating PID file (bug 12898)

[Martin Schwenke]

On Fri, 28 Jul 2017 16:43:33 +1000, Martin Schwenke via samba-technical
<samba-technical at lists.samba.org> wrote:

> On Thu, 27 Jul 2017 15:13:45 +0200, Volker Lendecke
> <Volker.Lendecke at SerNet.DE> wrote:
> 
> > On Thu, Jul 13, 2017 at 08:37:57PM +1000, Martin Schwenke wrote:  
> > > * API
> > > 
> > >   We really just want a file pathname instead of directory and basename.
> > >   The main reason for this is that initscripts and systemd unit files
> > >   (and. consequently, ctdbd_wrapper) need to know the full pathname, so
> > >   it doesn't make sense to split it and re-join it.    
> > 
> > Fine with that. All callers of lib/util/pidfile.c use
> > lp_pid_directory() or the source4 equivalent. That is easily
> > wrappable.  
> 
> Agreed.  :-)
> 
> > > * Coverity hit
> > > 
> > >   lib/util/pidfile.c currently leaks the file descriptor and Coverity
> > >   notices.  Amitay's implementation stores it in a context.  When the
> > >   context is freed then the destructor does unlink/unlock/close.    
> > 
> > To be honest, I see this as a cosmetic issue. The pidfile is kept open
> > by the parent process deliberately. It's a bit like the
> > autofree_context discussion: Depending on proper rundown by a talloc
> > destructor is important for more dynamic things. We should not depend
> > on this for main process exit. Other opinions? I'd call this a
> > "deliberate" in Coverity.  
> 
> I don't think it matters.  If the process exits without calling the
> destructor then the lock is released, so there's a harmless stale
> pidfile.  However, as a mechanism for ensuring the file is unlinked in
> most circumstances, the talloc destructor is "nice to have" - the
> pidfile gets cleaned up without having to keep the filename around.
> 
> > > * Fear  ;-)
> > > 
> > >   lib/util/pidfile.c has this at the top:
> > > 
> > >   /* this code is broken - there is a race condition with the unlink
> > >   (tridge) */    
> > 
> > Not sure this still is true. If the ctdb version does this better, I'm
> > more than happy to port the fix.  
> 
> Yeah, I've spent some time looking and there is still a problem...
> 
> The problem is that pidfile_pid() unlinks the file if the PID doesn't
> exist.  Say there are 2 calls to pidfile_pid() in flight with at least
> one of them coming from pidfile_create().  Both calls find the PID
> doesn't exist and branch to noproc: to do the unlink.  The one called
> from pidfile_create() returns first, creates the PID file and the second
> caller to pidfile_pid() unlinks it.
> 
> I don't think pidfile_pid() has any business unlinking the pidfile.  It
> should be a read-only operation.
> 
> The only time the pidfile should be *replaced* is if pidfile_create()
> finds that it is stale because it can take the lock, in which case it
> overwrites it. That means dropping O_EXCL from the open() in
> pidfile_create(). There's no need to call pidfile_pid() in
> pidfile_create() because that itself is racy and doesn't tell you
> anything.  In that case you basically end up with the core of Amitay's
> code in the CTDB version.  :-)
> 
> If you (and perhaps others) agree with this analysis then I'm happy to
> do the work to incorporate the core of the CTDB version of
> pidfile_create() into a low-level function in lib/util/pidfile.* and
> wrap both existing interfaces around that... along with the
> simplification to pidfile_pid() to stop it unlinking supposedly stale
> pidfiles.  I can do this on Monday...

Patch attached.

Obvious extensions to this include:

* Move pidfile_context_create() to lib/util/pidfile.*

  This means that ctdb/common/pidfile.* would be
  completely removed.  Also pidfile_path_create() and pidfile_close()
  would be static.

* Add documentation to lib/util/pidfile.h

  I didn't want to do this until I knew whether pidfile_path_create()
  and pidfile_close() will disappear from there.  ;-)

Please review and maybe push...

peace & happiness,
martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: samba.patch
Type: text/x-patch
Size: 16536 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170731/cb468591/samba.bin>