Andres Freund <and...@anarazel.de> writes:
> I'd still like to get something like your CLOEXEC patch applied
> independently however.

Here's an updated version of that, which makes use of our previous
conclusion that F_SETFD/FD_CLOEXEC are available everywhere except
Windows, and fixes some sloppy thinking about the EXEC_BACKEND case.

I went ahead and changed the call to epoll_create into epoll_create1.
I'm not too concerned about loss of portability there --- it seems
unlikely that many people are still using ten-year-old glibc, and
even less likely that any of them would be interested in running
current Postgres on their stable-unto-death platform.  We could add
a configure test for epoll_create1 if you feel one's needed, but
I think it'd just be a waste of cycles.

I propose to push this into HEAD and 9.6 too.

                        regards, tom lane

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 0d0701a..946fbff 100644
*** a/src/backend/storage/ipc/latch.c
--- b/src/backend/storage/ipc/latch.c
*************** static volatile sig_atomic_t waiting = f
*** 118,123 ****
--- 118,126 ----
  static int	selfpipe_readfd = -1;
  static int	selfpipe_writefd = -1;
  
+ /* Process owning the self-pipe --- needed for checking purposes */
+ static int	selfpipe_owner_pid = 0;
+ 
  /* Private function prototypes */
  static void sendSelfPipeByte(void);
  static void drainSelfPipe(void);
*************** InitializeLatchSupport(void)
*** 146,152 ****
  #ifndef WIN32
  	int			pipefd[2];
  
! 	Assert(selfpipe_readfd == -1);
  
  	/*
  	 * Set up the self-pipe that allows a signal handler to wake up the
--- 149,189 ----
  #ifndef WIN32
  	int			pipefd[2];
  
! 	if (IsUnderPostmaster)
! 	{
! 		/*
! 		 * We might have inherited connections to a self-pipe created by the
! 		 * postmaster.  It's critical that child processes create their own
! 		 * self-pipes, of course, and we really want them to close the
! 		 * inherited FDs for safety's sake.
! 		 */
! 		if (selfpipe_owner_pid != 0)
! 		{
! 			/* Assert we go through here but once in a child process */
! 			Assert(selfpipe_owner_pid != MyProcPid);
! 			/* Release postmaster's pipe FDs; ignore any error */
! 			(void) close(selfpipe_readfd);
! 			(void) close(selfpipe_writefd);
! 			/* Clean up, just for safety's sake; we'll set these below */
! 			selfpipe_readfd = selfpipe_writefd = -1;
! 			selfpipe_owner_pid = 0;
! 		}
! 		else
! 		{
! 			/*
! 			 * Postmaster didn't create a self-pipe ... or else we're in an
! 			 * EXEC_BACKEND build, in which case it doesn't matter since the
! 			 * postmaster's pipe FDs were closed by the action of FD_CLOEXEC.
! 			 */
! 			Assert(selfpipe_readfd == -1);
! 		}
! 	}
! 	else
! 	{
! 		/* In postmaster or standalone backend, assert we do this but once */
! 		Assert(selfpipe_readfd == -1);
! 		Assert(selfpipe_owner_pid == 0);
! 	}
  
  	/*
  	 * Set up the self-pipe that allows a signal handler to wake up the
*************** InitializeLatchSupport(void)
*** 154,176 ****
  	 * that SetLatch won't block if the event has already been set many times
  	 * filling the kernel buffer. Make the read-end non-blocking too, so that
  	 * we can easily clear the pipe by reading until EAGAIN or EWOULDBLOCK.
  	 */
  	if (pipe(pipefd) < 0)
  		elog(FATAL, "pipe() failed: %m");
  	if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl() failed on read-end of self-pipe: %m");
  	if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl() failed on write-end of self-pipe: %m");
  
  	selfpipe_readfd = pipefd[0];
  	selfpipe_writefd = pipefd[1];
  #else
  	/* currently, nothing to do here for Windows */
  #endif
  }
  
  /*
!  * Initialize a backend-local latch.
   */
  void
  InitLatch(volatile Latch *latch)
--- 191,220 ----
  	 * that SetLatch won't block if the event has already been set many times
  	 * filling the kernel buffer. Make the read-end non-blocking too, so that
  	 * we can easily clear the pipe by reading until EAGAIN or EWOULDBLOCK.
+ 	 * Also, make both FDs close-on-exec, since we surely do not want any
+ 	 * child processes messing with them.
  	 */
  	if (pipe(pipefd) < 0)
  		elog(FATAL, "pipe() failed: %m");
  	if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl(F_SETFL) failed on read-end of self-pipe: %m");
  	if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl(F_SETFL) failed on write-end of self-pipe: %m");
! 	if (fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1)
! 		elog(FATAL, "fcntl(F_SETFD) failed on read-end of self-pipe: %m");
! 	if (fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1)
! 		elog(FATAL, "fcntl(F_SETFD) failed on write-end of self-pipe: %m");
  
  	selfpipe_readfd = pipefd[0];
  	selfpipe_writefd = pipefd[1];
+ 	selfpipe_owner_pid = MyProcPid;
  #else
  	/* currently, nothing to do here for Windows */
  #endif
  }
  
  /*
!  * Initialize a process-local latch.
   */
  void
  InitLatch(volatile Latch *latch)
*************** InitLatch(volatile Latch *latch)
*** 181,187 ****
  
  #ifndef WIN32
  	/* Assert InitializeLatchSupport has been called in this process */
! 	Assert(selfpipe_readfd >= 0);
  #else
  	latch->event = CreateEvent(NULL, TRUE, FALSE, NULL);
  	if (latch->event == NULL)
--- 225,231 ----
  
  #ifndef WIN32
  	/* Assert InitializeLatchSupport has been called in this process */
! 	Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid);
  #else
  	latch->event = CreateEvent(NULL, TRUE, FALSE, NULL);
  	if (latch->event == NULL)
*************** InitLatch(volatile Latch *latch)
*** 199,204 ****
--- 243,252 ----
   * containing the latch with ShmemInitStruct. (The Unix implementation
   * doesn't actually require that, but the Windows one does.) Because of
   * this restriction, we have no concurrency issues to worry about here.
+  *
+  * Note that other handles created in this module are never marked as
+  * inheritable.  Thus we do not need to worry about cleaning up child
+  * process references to postmaster-private latches or WaitEventSets.
   */
  void
  InitSharedLatch(volatile Latch *latch)
*************** OwnLatch(volatile Latch *latch)
*** 244,250 ****
  
  #ifndef WIN32
  	/* Assert InitializeLatchSupport has been called in this process */
! 	Assert(selfpipe_readfd >= 0);
  #endif
  
  	if (latch->owner_pid != 0)
--- 292,298 ----
  
  #ifndef WIN32
  	/* Assert InitializeLatchSupport has been called in this process */
! 	Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid);
  #endif
  
  	if (latch->owner_pid != 0)
*************** DisownLatch(volatile Latch *latch)
*** 277,283 ****
   * is incurred when WL_TIMEOUT is given, so avoid using a timeout if possible.
   *
   * The latch must be owned by the current process, ie. it must be a
!  * backend-local latch initialized with InitLatch, or a shared latch
   * associated with the current process by calling OwnLatch.
   *
   * Returns bit mask indicating which condition(s) caused the wake-up. Note
--- 325,331 ----
   * is incurred when WL_TIMEOUT is given, so avoid using a timeout if possible.
   *
   * The latch must be owned by the current process, ie. it must be a
!  * process-local latch initialized with InitLatch, or a shared latch
   * associated with the current process by calling OwnLatch.
   *
   * Returns bit mask indicating which condition(s) caused the wake-up. Note
*************** CreateWaitEventSet(MemoryContext context
*** 517,525 ****
  	set->nevents_space = nevents;
  
  #if defined(WAIT_USE_EPOLL)
! 	set->epoll_fd = epoll_create(nevents);
  	if (set->epoll_fd < 0)
! 		elog(ERROR, "epoll_create failed: %m");
  #elif defined(WAIT_USE_WIN32)
  
  	/*
--- 565,573 ----
  	set->nevents_space = nevents;
  
  #if defined(WAIT_USE_EPOLL)
! 	set->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
  	if (set->epoll_fd < 0)
! 		elog(ERROR, "epoll_create1 failed: %m");
  #elif defined(WAIT_USE_WIN32)
  
  	/*
*************** CreateWaitEventSet(MemoryContext context
*** 540,545 ****
--- 588,599 ----
  
  /*
   * Free a previously created WaitEventSet.
+  *
+  * Note: preferably, this shouldn't have to free any resources that could be
+  * inherited across an exec().  If it did, we'd likely leak those resources in
+  * many scenarios.  For the epoll case, we ensure that by setting FD_CLOEXEC
+  * when the FD is created.  For the Windows case, we assume that the handles
+  * involved are non-inheritable.
   */
  void
  FreeWaitEventSet(WaitEventSet *set)
*************** FreeWaitEventSet(WaitEventSet *set)
*** 586,592 ****
   * used to modify previously added wait events using ModifyWaitEvent().
   *
   * In the WL_LATCH_SET case the latch must be owned by the current process,
!  * i.e. it must be a backend-local latch initialized with InitLatch, or a
   * shared latch associated with the current process by calling OwnLatch.
   *
   * In the WL_SOCKET_READABLE/WRITEABLE case, EOF and error conditions are
--- 640,646 ----
   * used to modify previously added wait events using ModifyWaitEvent().
   *
   * In the WL_LATCH_SET case the latch must be owned by the current process,
!  * i.e. it must be a process-local latch initialized with InitLatch, or a
   * shared latch associated with the current process by calling OwnLatch.
   *
   * In the WL_SOCKET_READABLE/WRITEABLE case, EOF and error conditions are
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to