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

classic Classic list List threaded Threaded
4 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

Loading...