close cron sockets in child processes

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

close cron sockets in child processes

Florian Riehm-2
Hi,

cron(8) opens /var/run/cron.sock for communication with crontab(1).
The forked cronjobs have the socked still open.
This prevents restarting cron while a job is running:
(CRON) DEATH (already running)

I think cron's children should not inherit sockets.

ok?

friehm

Index: usr.sbin/cron/do_command.c
===================================================================
RCS file: /cvs/src/usr.sbin/cron/do_command.c,v
retrieving revision 1.56
diff -u -p -r1.56 do_command.c
--- usr.sbin/cron/do_command.c 17 Nov 2015 22:31:44 -0000 1.56
+++ usr.sbin/cron/do_command.c 20 Oct 2017 13:56:27 -0000
@@ -86,6 +86,9 @@ child_process(entry *e, user *u)
  /* mark ourselves as different to PS command watchers */
  setproctitle("running job");
 
+ /* close sockets from parent (i.e. cronSock) */
+ closefrom(3);
+
  /* discover some useful and important environment settings
  */
  usernm = e->pwd->pw_name;

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Jeremie Courreges-Anglas-2
On Fri, Oct 20 2017, Florian Riehm <[hidden email]> wrote:
> Hi,
>
> cron(8) opens /var/run/cron.sock for communication with crontab(1).
> The forked cronjobs have the socked still open.
> This prevents restarting cron while a job is running:
> (CRON) DEATH (already running)

Hah.

> I think cron's children should not inherit sockets.
>
> ok?

Looks reasonable, ok jca@

(I had to apply your diff manually, though.  Could you please add
a newline between setproctitle(3) and your comment?)

> friehm
>
> Index: usr.sbin/cron/do_command.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/cron/do_command.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 do_command.c
> --- usr.sbin/cron/do_command.c 17 Nov 2015 22:31:44 -0000 1.56
> +++ usr.sbin/cron/do_command.c 20 Oct 2017 13:56:27 -0000
> @@ -86,6 +86,9 @@ child_process(entry *e, user *u)
>   /* mark ourselves as different to PS command watchers */
>   setproctitle("running job");
>  + /* close sockets from parent (i.e. cronSock) */
> + closefrom(3);
> +
>   /* discover some useful and important environment settings
>   */
>   usernm = e->pwd->pw_name;
>

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Todd C. Miller
In reply to this post by Florian Riehm-2
On Fri, 20 Oct 2017 16:25:32 +0200, Florian Riehm wrote:

> cron(8) opens /var/run/cron.sock for communication with crontab(1).
> The forked cronjobs have the socked still open.
> This prevents restarting cron while a job is running:
> (CRON) DEATH (already running)
>
> I think cron's children should not inherit sockets.

There is a similar issue with at jobs.  Here's a comprehensive diff.

 - todd

Index: usr.sbin/cron/atrun.c
===================================================================
RCS file: /cvs/src/usr.sbin/cron/atrun.c,v
retrieving revision 1.46
diff -u -p -u -r1.46 atrun.c
--- usr.sbin/cron/atrun.c 8 Jun 2017 16:23:39 -0000 1.46
+++ usr.sbin/cron/atrun.c 20 Oct 2017 15:09:57 -0000
@@ -283,6 +283,9 @@ run_job(const atjob *job, int dfd, const
  return;
  }
 
+ /* close fds opened by the parent (e.g. cronSock) */
+ closefrom(3);
+
  /*
  * We don't want the main cron daemon to wait for our children--
  * we will do it ourselves via waitpid().
Index: usr.sbin/cron/do_command.c
===================================================================
RCS file: /cvs/src/usr.sbin/cron/do_command.c,v
retrieving revision 1.56
diff -u -p -u -r1.56 do_command.c
--- usr.sbin/cron/do_command.c 17 Nov 2015 22:31:44 -0000 1.56
+++ usr.sbin/cron/do_command.c 20 Oct 2017 15:10:03 -0000
@@ -86,6 +86,9 @@ child_process(entry *e, user *u)
  /* mark ourselves as different to PS command watchers */
  setproctitle("running job");
 
+ /* close fds opened by the parent (e.g. cronSock) */
+ closefrom(3);
+
  /* discover some useful and important environment settings
  */
  usernm = e->pwd->pw_name;

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Jeremie Courreges-Anglas-2
On Fri, Oct 20 2017, "Todd C. Miller" <[hidden email]> wrote:

> On Fri, 20 Oct 2017 16:25:32 +0200, Florian Riehm wrote:
>
>> cron(8) opens /var/run/cron.sock for communication with crontab(1).
>> The forked cronjobs have the socked still open.
>> This prevents restarting cron while a job is running:
>> (CRON) DEATH (already running)
>>
>> I think cron's children should not inherit sockets.
>
> There is a similar issue with at jobs.  Here's a comprehensive diff.

Good catch.  The cron part ought to be committed now, but I think
atrun.c needs more love.

>  - todd
>
> Index: usr.sbin/cron/atrun.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/cron/atrun.c,v
> retrieving revision 1.46
> diff -u -p -u -r1.46 atrun.c
> --- usr.sbin/cron/atrun.c 8 Jun 2017 16:23:39 -0000 1.46
> +++ usr.sbin/cron/atrun.c 20 Oct 2017 15:09:57 -0000
> @@ -283,6 +283,9 @@ run_job(const atjob *job, int dfd, const
>   return;
>   }
>  
> + /* close fds opened by the parent (e.g. cronSock) */
> + closefrom(3);
> +

That doesn't work.  When here, we hold cronSock and dfd (should be 3 and
4).  We need dfd to get fd (should be 5), thus we can't just use
"closefrom(3)".  The straightest way IMO is to close what we should
close (including dfd), but this means making cronSock a global.

I would also propose sprinkling more O_CLOEXEC magic but that can be in
another diff.

Thoughts?


Index: atrun.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/cron/atrun.c,v
retrieving revision 1.46
diff -u -p -r1.46 atrun.c
--- atrun.c 8 Jun 2017 16:23:39 -0000 1.46
+++ atrun.c 23 Oct 2017 03:21:20 -0000
@@ -283,6 +283,10 @@ run_job(const atjob *job, int dfd, const
  return;
  }
 
+ /* Close fds opened by the parent. */
+ close(cronSock);
+ close(dfd);
+
  /*
  * We don't want the main cron daemon to wait for our children--
  * we will do it ourselves via waitpid().
Index: cron.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/cron/cron.c,v
retrieving revision 1.76
diff -u -p -r1.76 cron.c
--- cron.c 7 Jun 2017 23:36:43 -0000 1.76
+++ cron.c 23 Oct 2017 02:10:54 -0000
@@ -60,7 +60,6 @@ static int open_socket(void);
 
 static volatile sig_atomic_t got_sigchld;
 static time_t timeRunning, virtualTime, clockTime;
-static int cronSock;
 static long GMToff;
 static cron_db *database;
 static at_db *at_database;
@@ -68,6 +67,7 @@ static double batch_maxload = BATCH_MA
 static int NoFork;
 static time_t StartTime;
  gid_t cron_gid;
+ int cronSock;
 
 static void
 usage(void)
Index: globals.h
===================================================================
RCS file: /d/cvs/src/usr.sbin/cron/globals.h,v
retrieving revision 1.14
diff -u -p -r1.14 globals.h
--- globals.h 7 Jun 2017 23:36:43 -0000 1.14
+++ globals.h 23 Oct 2017 02:03:18 -0000
@@ -18,5 +18,6 @@
  */
 
 extern gid_t cron_gid;
+extern int cronSock;
 extern int LineNumber;
 extern char *__progname;


--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Florian Riehm-2
On 10/23/17 09:05, Jeremie Courreges-Anglas wrote:

> On Fri, Oct 20 2017, "Todd C. Miller" <[hidden email]> wrote:
>> On Fri, 20 Oct 2017 16:25:32 +0200, Florian Riehm wrote:
>>
>>> cron(8) opens /var/run/cron.sock for communication with crontab(1).
>>> The forked cronjobs have the socked still open.
>>> This prevents restarting cron while a job is running:
>>> (CRON) DEATH (already running)
>>>
>>> I think cron's children should not inherit sockets.
>>
>> There is a similar issue with at jobs.  Here's a comprehensive diff.
>
> Good catch.  The cron part ought to be committed now, but I think
> atrun.c needs more love.
Good catch, indeed.
I have commited the cron part.

>
>>   - todd
>>
>> Index: usr.sbin/cron/atrun.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/cron/atrun.c,v
>> retrieving revision 1.46
>> diff -u -p -u -r1.46 atrun.c
>> --- usr.sbin/cron/atrun.c 8 Jun 2017 16:23:39 -0000 1.46
>> +++ usr.sbin/cron/atrun.c 20 Oct 2017 15:09:57 -0000
>> @@ -283,6 +283,9 @@ run_job(const atjob *job, int dfd, const
>>   return;
>>   }
>>  
>> + /* close fds opened by the parent (e.g. cronSock) */
>> + closefrom(3);
>> +
>
> That doesn't work.  When here, we hold cronSock and dfd (should be 3 and
> 4).  We need dfd to get fd (should be 5), thus we can't just use
> "closefrom(3)".  The straightest way IMO is to close what we should
> close (including dfd), but this means making cronSock a global.
>
> I would also propose sprinkling more O_CLOEXEC magic but that can be in
> another diff.

I guess it is ok to make cronSock global. closefrom() is good if
you have obvious situations. In more complex scenarios like this, I
prefer close() since it shows more clearly what happens.

>
> Thoughts?
>
>
> Index: atrun.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/cron/atrun.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 atrun.c
> --- atrun.c 8 Jun 2017 16:23:39 -0000 1.46
> +++ atrun.c 23 Oct 2017 03:21:20 -0000
> @@ -283,6 +283,10 @@ run_job(const atjob *job, int dfd, const
>   return;
>   }
>  
> + /* Close fds opened by the parent. */
> + close(cronSock);
> + close(dfd);
> +
>   /*
>   * We don't want the main cron daemon to wait for our children--
>   * we will do it ourselves via waitpid().
> Index: cron.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/cron/cron.c,v
> retrieving revision 1.76
> diff -u -p -r1.76 cron.c
> --- cron.c 7 Jun 2017 23:36:43 -0000 1.76
> +++ cron.c 23 Oct 2017 02:10:54 -0000
> @@ -60,7 +60,6 @@ static int open_socket(void);
>  
>   static volatile sig_atomic_t got_sigchld;
>   static time_t timeRunning, virtualTime, clockTime;
> -static int cronSock;
>   static long GMToff;
>   static cron_db *database;
>   static at_db *at_database;
> @@ -68,6 +67,7 @@ static double batch_maxload = BATCH_MA
>   static int NoFork;
>   static time_t StartTime;
>   gid_t cron_gid;
> + int cronSock;
>  
>   static void
>   usage(void)
> Index: globals.h
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/cron/globals.h,v
> retrieving revision 1.14
> diff -u -p -r1.14 globals.h
> --- globals.h 7 Jun 2017 23:36:43 -0000 1.14
> +++ globals.h 23 Oct 2017 02:03:18 -0000
> @@ -18,5 +18,6 @@
>    */
>  
>   extern gid_t cron_gid;
> +extern int cronSock;
>   extern int LineNumber;
>   extern char *__progname;
>
>

ok friehm

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Todd C. Miller
In reply to this post by Jeremie Courreges-Anglas-2
On Mon, 23 Oct 2017 09:05:48 +0200, Jeremie Courreges-Anglas wrote:

> That doesn't work.  When here, we hold cronSock and dfd (should be 3 and
> 4).  We need dfd to get fd (should be 5), thus we can't just use
> "closefrom(3)".  The straightest way IMO is to close what we should
> close (including dfd), but this means making cronSock a global.

OK millert@

> I would also propose sprinkling more O_CLOEXEC magic but that can be in
> another diff.

Sure.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Jeremie Courreges-Anglas-2
On Mon, Oct 23 2017, "Todd C. Miller" <[hidden email]> wrote:

> On Mon, 23 Oct 2017 09:05:48 +0200, Jeremie Courreges-Anglas wrote:
>
>> That doesn't work.  When here, we hold cronSock and dfd (should be 3 and
>> 4).  We need dfd to get fd (should be 5), thus we can't just use
>> "closefrom(3)".  The straightest way IMO is to close what we should
>> close (including dfd), but this means making cronSock a global.
>
> OK millert@
>
>> I would also propose sprinkling more O_CLOEXEC magic but that can be in
>> another diff.
>
> Sure.

Here it is.  Liberal use of O_CLOEXEC except for poke_daemon() which is
more obvious (but I can add it there too if you prefer).  cronSock
already uses SOCK_CLOEXEC.

ok?


Index: atrun.c
===================================================================
RCS file: /cvs/src/usr.sbin/cron/atrun.c,v
retrieving revision 1.47
diff -u -p -r1.47 atrun.c
--- atrun.c 23 Oct 2017 15:15:22 -0000 1.47
+++ atrun.c 23 Oct 2017 15:26:45 -0000
@@ -83,7 +83,8 @@ scan_atjobs(at_db **db, struct timespec
  struct dirent *file;
  struct stat sb;
 
- if ((dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY)) == -1) {
+ dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY|O_CLOEXEC);
+ if (dfd == -1) {
  syslog(LOG_ERR, "(CRON) OPEN FAILED (%s)", _PATH_AT_SPOOL);
  return (0);
  }
@@ -175,7 +176,8 @@ atrun(at_db *db, double batch_maxload, t
  if (db == NULL)
  return;
 
- if ((dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY)) == -1) {
+ dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY|O_CLOEXEC);
+ if (dfd == -1) {
  syslog(LOG_ERR, "(CRON) OPEN FAILED (%s)", _PATH_AT_SPOOL);
  return;
  }
@@ -262,7 +264,8 @@ run_job(const atjob *job, int dfd, const
  char *nargv[2], *nenvp[1];
 
  /* Open the file and unlink it so we don't try running it again. */
- if ((fd = openat(dfd, atfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW, 0)) < 0) {
+ fd = openat(dfd, atfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC, 0);
+ if (fd == -1) {
  syslog(LOG_ERR, "(CRON) CAN'T OPEN (%s)", atfile);
  return;
  }
Index: database.c
===================================================================
RCS file: /cvs/src/usr.sbin/cron/database.c,v
retrieving revision 1.35
diff -u -p -r1.35 database.c
--- database.c 7 Jun 2017 23:36:43 -0000 1.35
+++ database.c 23 Oct 2017 15:26:45 -0000
@@ -182,7 +182,8 @@ process_crontab(int dfd, const char *una
  goto next_crontab;
  }
 
- crontab_fd = openat(dfd, fname, O_RDONLY|O_NONBLOCK|O_NOFOLLOW);
+ crontab_fd = openat(dfd, fname,
+    O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC);
  if (crontab_fd < 0) {
  /* crontab not accessible?
  */

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Todd C. Miller
On Mon, 23 Oct 2017 17:27:15 +0200, Jeremie Courreges-Anglas wrote:

> Here it is.  Liberal use of O_CLOEXEC except for poke_daemon() which is
> more obvious (but I can add it there too if you prefer).  cronSock
> already uses SOCK_CLOEXEC.

You can't set O_CLOEXEC for the fd in atrun.c as that will become
the executed command's stdin.  The others are OK millert@

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Jeremie Courreges-Anglas-2
In reply to this post by Jeremie Courreges-Anglas-2
On Mon, Oct 23 2017, Jeremie Courreges-Anglas <[hidden email]> wrote:

> On Mon, Oct 23 2017, "Todd C. Miller" <[hidden email]> wrote:
>> On Mon, 23 Oct 2017 09:05:48 +0200, Jeremie Courreges-Anglas wrote:
>>
>>> That doesn't work.  When here, we hold cronSock and dfd (should be 3 and
>>> 4).  We need dfd to get fd (should be 5), thus we can't just use
>>> "closefrom(3)".  The straightest way IMO is to close what we should
>>> close (including dfd), but this means making cronSock a global.
>>
>> OK millert@
>>
>>> I would also propose sprinkling more O_CLOEXEC magic but that can be in
>>> another diff.
>>
>> Sure.
>
> Here it is.  Liberal use of O_CLOEXEC except for poke_daemon() which is
> more obvious (but I can add it there too if you prefer).  cronSock
> already uses SOCK_CLOEXEC.
>
> ok?

Note: there may be others places where we could use close-on-exec mode
"e" for fopen, not sure about this yet.  Feel free to beat me to it. :)

>
> Index: atrun.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/cron/atrun.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 atrun.c
> --- atrun.c 23 Oct 2017 15:15:22 -0000 1.47
> +++ atrun.c 23 Oct 2017 15:26:45 -0000
> @@ -83,7 +83,8 @@ scan_atjobs(at_db **db, struct timespec
>   struct dirent *file;
>   struct stat sb;
>  
> - if ((dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY)) == -1) {
> + dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY|O_CLOEXEC);
> + if (dfd == -1) {
>   syslog(LOG_ERR, "(CRON) OPEN FAILED (%s)", _PATH_AT_SPOOL);
>   return (0);
>   }
> @@ -175,7 +176,8 @@ atrun(at_db *db, double batch_maxload, t
>   if (db == NULL)
>   return;
>  
> - if ((dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY)) == -1) {
> + dfd = open(_PATH_AT_SPOOL, O_RDONLY|O_DIRECTORY|O_CLOEXEC);
> + if (dfd == -1) {
>   syslog(LOG_ERR, "(CRON) OPEN FAILED (%s)", _PATH_AT_SPOOL);
>   return;
>   }
> @@ -262,7 +264,8 @@ run_job(const atjob *job, int dfd, const
>   char *nargv[2], *nenvp[1];
>  
>   /* Open the file and unlink it so we don't try running it again. */
> - if ((fd = openat(dfd, atfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW, 0)) < 0) {
> + fd = openat(dfd, atfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC, 0);
> + if (fd == -1) {
>   syslog(LOG_ERR, "(CRON) CAN'T OPEN (%s)", atfile);
>   return;
>   }
> Index: database.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/cron/database.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 database.c
> --- database.c 7 Jun 2017 23:36:43 -0000 1.35
> +++ database.c 23 Oct 2017 15:26:45 -0000
> @@ -182,7 +182,8 @@ process_crontab(int dfd, const char *una
>   goto next_crontab;
>   }
>  
> - crontab_fd = openat(dfd, fname, O_RDONLY|O_NONBLOCK|O_NOFOLLOW);
> + crontab_fd = openat(dfd, fname,
> +    O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC);
>   if (crontab_fd < 0) {
>   /* crontab not accessible?
>   */

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: close cron sockets in child processes

Jeremie Courreges-Anglas-2
In reply to this post by Todd C. Miller
On Mon, Oct 23 2017, "Todd C. Miller" <[hidden email]> wrote:
> On Mon, 23 Oct 2017 17:27:15 +0200, Jeremie Courreges-Anglas wrote:
>
>> Here it is.  Liberal use of O_CLOEXEC except for poke_daemon() which is
>> more obvious (but I can add it there too if you prefer).  cronSock
>> already uses SOCK_CLOEXEC.
>
> You can't set O_CLOEXEC for the fd in atrun.c as that will become
> the executed command's stdin.  The others are OK millert@

Gaah, yes indeed.  I should have used my existing diff instead of
re-doing it from scratch.  I'll check everything again before
committing, thanks.

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE