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

Martin Schwenke martin at meltin.net
Thu Jul 13 10:37:57 UTC 2017


On Thu, 13 Jul 2017 10:51:50 +0200, Volker Lendecke
<Volker.Lendecke at SerNet.DE> wrote:

> On Thu, Jul 13, 2017 at 04:21:19PM +1000, Martin Schwenke via samba-technical wrote:

> > Otherwise, for example, the file descriptor for the main PID file will
> > leak all the way down to event scripts.  
> 
> What prevents ctdb and the rest of samba to use a common pidfile
> handling? The version in lib/util already has that close on exec bit
> set, at least it does have a call to smb_set_close_on_exec(fd);.

Yes, that is definitely a good goal.  Amitay put together the CTDB
version because we had a bug that we were fixing under time pressure.
We decided not to solve the issues at that time...

There are a few things:

* 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.

* 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.

* Fear  ;-)

  lib/util/pidfile.c has this at the top:

  /* this code is broken - there is a race condition with the unlink
  (tridge) */


We should find a way of uniting them.  Perhaps we can end up with:

* pidfile_path_create()

  Renamed from ctdb/common/pidfile.c pidfile_create()

* Wrap pidfile_create() around pidfile_path_create()

* Drop pidfile_unlink() in favour of using struct pidfile_context and
  talloc_free()ing it to unlink/unlock/close.

* Leave pidfile_pid() pretty much how it is...

Thoughts?

peace & happiness,
martin



More information about the samba-technical mailing list