Recommendation on validating the return value of setsid() along with error messages

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

Recommendation on validating the return value of setsid() along with error messages

niuxu
Our team works on log enhancement by learning from historical log revisions in evolution.

By mining historical patches, we find two log revisions that added validation about the return value of setsid() along with log messages. So we suggest that the return value of setsid() should be well handled.

Applying our tool on postfix-3.2.4, we find two missed spots:

1) Line 709 in file: postfix-3.2.4/src/proxymap/proxymap.c
static void post_jail_init(char *service_name, char **unused_argv)
{  
    ...
    /*
     * Never, ever, get killed by a master signal, as that could corrupt a
     * persistent database when we're in the middle of an update.
     */
    if (proxy_writer != 0)
        setsid();
}

2) Line 248 in file: postfix-3.2.4/src/util/spawn_command.c
WAIT_STATUS_T spawn_command(int key,...)
{
    ...
    case 0:
        if (args.uid != (uid_t) - 1 || args.gid != (gid_t) - 1)
            set_ugid(args.uid, args.gid);
        setsid();

3) Line 670 in postfix-3.2.4/src/verify/verify.c
static void pre_jail_init(char *unused_name, char **unused_argv)
{
    mode_t  saved_mask;
    VSTRING *redirect;
    /*
     * Never, ever, get killed by a master signal, as that would corrupt the
     * database when we're in the middle of an update.
     */
    setsid();
    ...

And following are two historical patches that support our opinion:
1)In file postfix-2.0.20/src/global/pipe_command.c
  /*
  * Child. Run the child in a separate process group so that the
  * parent can kill not just the child but also its offspring.
  */
     case 0:
  set_ugid(args.uid, args.gid);
- setsid();
+ if (setsid() < 0)
+    msg_warn("setsid failed: %m");

2) In file postfix-2.0.20/src/master/master.c
    for (fd = 0; fd < 3; fd++) {
  (void) close(fd);
  if (open("/dev/null", O_RDWR, 0) != fd)
     msg_fatal("open /dev/null: %m");
     }
-    setsid();
+
+    /*
+     * Run in a separate process group, so that "postfix stop" can terminate
+     * all MTA processes cleanly. Give up if we can't separate from our
+     * parent process. We're not supposed to blow away the parent.
+     */
+    if (setsid() == -1)
+ msg_fatal("unable to set session and process group ID: %m");

Thanks for your reading and we are looking forward to your reply about the correctness of our suggestions.
May you a good day!

Best Regards,
Xu