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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Otherwise, for example, the file descriptor for the main PID file will
leak all the way down to event scripts.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12898

Please review and maybe push...

peace & happiness,
martin

0001-ctdb-common-Set-close-on-exec-when-creating-PID-file.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Thu, Jul 13, 2017 at 4:21 PM, Martin Schwenke via samba-technical <
[hidden email]> wrote:

> Otherwise, for example, the file descriptor for the main PID file will
> leak all the way down to event scripts.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12898
>
> Please review and maybe push...
>
> peace & happiness,
> martin
>

Pushed to autobuild.

Amitay.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
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);.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Thu, 13 Jul 2017 10:51:50 +0200, Volker Lendecke
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
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.

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

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

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Thu, 27 Jul 2017 15:13:45 +0200, Volker Lendecke
<[hidden email]> 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...

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Fri, 28 Jul 2017 16:43:33 +1000, Martin Schwenke via samba-technical
<[hidden email]> wrote:

> On Thu, 27 Jul 2017 15:13:45 +0200, Volker Lendecke
> <[hidden email]> 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

samba.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Mon, Jul 31, 2017 at 03:44:46PM +1000, Martin Schwenke wrote:

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

Great, thanks! Pushed!

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Mon, Jul 31, 2017 at 11:37:54AM +0200, Volker Lendecke wrote:

> On Mon, Jul 31, 2017 at 03:44:46PM +1000, Martin Schwenke wrote:
> > 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...
>
> Great, thanks! Pushed!

Hmm. Fails with

[50(716)/2182 at 11m38s] wafsamba.duplicate_symbols
Waf: Entering directory `/memdisk/vlendec/a/b456000/samba/bin'
        Selected embedded Heimdal build
[4090/4214] symbol duplicate checking:
Waf: Leaving directory `/memdisk/vlendec/a/b456000/samba/bin'
Build failed:
default/source3/lib/netapi/examples/localgroup/localgroup_setinfo: Symbol pidfile_close linked in multiple libraries ['samba-util', '/lib/x86_64-linux-gnu/libbsd.so.0.6.0']
UNEXPECTED(failure):
wafsamba.duplicate_symbols.duplicate_symbols(none)
REASON: Exception: Exception:

Can you check?

Thanks, Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Mon, 31 Jul 2017 18:55:57 +0200, Volker Lendecke
<[hidden email]> wrote:

> On Mon, Jul 31, 2017 at 11:37:54AM +0200, Volker Lendecke wrote:
> > On Mon, Jul 31, 2017 at 03:44:46PM +1000, Martin Schwenke wrote:  
> > > 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...  
> >
> > Great, thanks! Pushed!  
>
> Hmm. Fails with
>
> [50(716)/2182 at 11m38s] wafsamba.duplicate_symbols
> Waf: Entering directory `/memdisk/vlendec/a/b456000/samba/bin'
>         Selected embedded Heimdal build
> [4090/4214] symbol duplicate checking:
> Waf: Leaving directory `/memdisk/vlendec/a/b456000/samba/bin'
> Build failed:
> default/source3/lib/netapi/examples/localgroup/localgroup_setinfo: Symbol pidfile_close linked in multiple libraries ['samba-util', '/lib/x86_64-linux-gnu/libbsd.so.0.6.0']
> UNEXPECTED(failure):
> wafsamba.duplicate_symbols.duplicate_symbols(none)
> REASON: Exception: Exception:
>
> Can you check?
Sorry...

OK, I've renamed pidfile_close() -> pidfile_fd_close() to stop it
colliding with the BSD pidfile_close().  It now passes a private
autobuild on sn-devel.  New patch attached.

When this is in I'll do the extra commit to add documentation to
lib/util/pidfile.h.

The BSD pidfile_*() functions actually look pretty good - I've only
scanned the manpage. Perhaps in the future we can switch to them or wrap
them. Before doing that I'd like to look at their current
implementation.  ;-)

peace & happiness,
martin

samba.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Tue, Aug 01, 2017 at 06:07:55PM +1000, Martin Schwenke wrote:
> > Can you check?
>
> Sorry...

Pushed, thanks.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Tue, 1 Aug 2017 14:19:14 +0200, Volker Lendecke
<[hidden email]> wrote:

> On Tue, Aug 01, 2017 at 06:07:55PM +1000, Martin Schwenke wrote:
> > > Can you check?  
> >
> > Sorry...  
>
> Pushed, thanks.

Fell over in a winbind test, pushed again...

peace & happiness,
martin

Loading...