Public SourceMage Discussion List

[seth AT swoolley.homeip.net]

Hi Jens, this is awesome!  I guess I'm the "simpleinit-msb" maintainer 
for smgl, so I'll be reviewing this patch.  Can you file a bug in 
bugzilla and attached it there so we can discuss it further in 
nitty-gritty?

One question I have is what situation did you find this helped you?  
Normally it won't offer any advantages to regular users, but if you're 
doing funky stuff with filesystems, having to be hindered by 
maintaining that fs-named fifo just to have the machine boot up can be 
an annoyance.

There are a number of things in here that don't seem to directly relate 
to your change (snprintf cleanups, moving a close(fd) (I'll have to look 
closer at this), reading to fill an entire buffer, and not just the 
command size).  They all could possibly be good, but I'd like to review 
them individually to make sure they don't have any bad side-effects 
before throwing this in.

For any patches to the basesystem like this, we like to be very 
intentional about what patches we add.  Normally we do as little as 
possible (to be honest to upstream), but since there is no longer an 
official "upstream", I volunteered to maintain our local copy as the 
need arose.  Well, the need. arose it looks like.

Thanks again, Jens, and, please, file a bug (so you are "owner" of it) 
and attach it, and be sure to CC me on it.

Seth

On Thu, Mar 13, 2008 at 09:01:42AM +0100, Jens Laas wrote:
> 
> Hello
> 
> I've made a patch that converts simpleinit from using a fifo file to use 
> an abstract socket for communicating with initctl.
> 
> This means there is no longer a file in the filesystem that needs to be 
> maintained since abtract sockets do not map onto the filesystem.
> 
> Tested on one computer where it works.
> 
> The patch goes on top of the patches already in simpleinit-msb.
> 
> Cheers
> Jens L??s
> 
> -----------------------------------------------------------------------
>     'In theory, there is no difference between theory and practice.
>      But, in practice, there is.'
> -----------------------------------------------------------------------
>     Jens L??s                              Email: jens.laas AT data.slu.se
>     Department of Computer Services, SLU   Phone: +46 18 67 35 15
>     Vindbrov?gen 1
>     P.O. Box 7079
>     S-750 07 Uppsala
>     SWEDEN
> -----------------------------------------------------------------------

> Only in simpleinit-msb-1.2-jel: defines.h
> Only in simpleinit-msb-1.2-jel/lib: my_reboot.o
> Only in simpleinit-msb-1.2-jel/lib: xstrncpy.o
> diff -ur simpleinit-msb-1.2-orig/login-utils/initctl.c 
> simpleinit-msb-1.2-jel/login-utils/initctl.c
> --- simpleinit-msb-1.2-orig/login-utils/initctl.c     2001-09-26 
> 12:59:46.000000000 +0200
> +++ simpleinit-msb-1.2-jel/login-utils/initctl.c      2008-03-12 
> 13:54:43.000000000 +0100
> @@ -95,6 +95,8 @@
>  #include <stdlib.h>
>  #include <limits.h>
>  #include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <signal.h>
> @@ -106,12 +108,40 @@
>  static void signal_handler (int sig);
>  static void fix_userspace_paths(void);
>  static void output_usage(void);
> +static int init_connect(const char *name);
>  
>  static int caught_signal = 0;
>  static int userspace = 0;
> -static char initctl_name[PATH_MAX]="/dev/initctl";
> +static char initctl_name[PATH_MAX]=INITSOCKETNAME;
>  static char pidfile_name[PATH_MAX]="/dev/init.pid";
>  
> +int init_connect(const char *name)
> +{
> +  int s, rc;
> +  struct sockaddr_un dst;
> +  socklen_t dstsize = 0;
> +  
> +  dstsize = sizeof(dst);
> +  
> +  memset(&dst,0,sizeof(dst));
> +  dst.sun_family = AF_LOCAL;
> +  
> +  s = socket(dst.sun_family, SOCK_STREAM, 0);
> +  if(s < 0) return -1;
> +
> +  strcpy(dst.sun_path+1, name);
> +
> +  rc = connect(s, (struct sockaddr *)&dst, dstsize);
> +  
> +  if(rc == -1)
> +    {
> +      close(s);
> +      return -1;
> +    }
> +
> +  return s;
> +}
> +
>  int main (int argc, char **argv)
>  {
>      int fd, nbytes;
> @@ -191,7 +221,7 @@
>      }
>  
>      /* test for R_OK instead of W_OK so that it will work on read-only fs 
> */
> -    if ((access(initctl_name,R_OK)!=0) || 
> (getenv("USERSPACE_INIT")!=NULL)) userspace=1;
> +    if ((getuid()!=0) || (getenv("USERSPACE_INIT")!=NULL)) userspace=1;
>      if (userspace) fix_userspace_paths();
>      
>      switch (command->command)
> @@ -207,29 +237,15 @@
>       else command->name[0] = '\0';
>       break;
>        case COMMAND_DUMP_LIST:
> -     if (tmpnam (command->name) == NULL)
> -     {
> -         fprintf (stderr, "Unable to create a unique filename\t%s\n",
> -                  ERRSTRING);
> -         exit (1);
> -     }
> -     if (mkfifo (command->name, S_IRUSR|S_IWUSR) != 0)
> -     {
> -         fprintf (stderr, "Unable to create FIFO: \"%s\"\t%s\n",
> -                  command->name, ERRSTRING);
> -         exit (1);
> -     }
>       break;
>      }
>      
>      
>      {
>        long arg;
> -      /*The following open() MUST use O_WRONLY|O_NONBLOCK to notice if the
> -      other end of the fifo is not open (i.e. init is not responsive). Note
> -      especially that opening O_RDWR will *not* suit this purpose 
> -      (see fifo(4) )*/
> -      if ( ( fd = open (initctl_name, O_WRONLY|O_NONBLOCK, 0) ) < 0 )
> +      /* connect to inits acstract socket */
> +      
> +      if ( ( fd = init_connect(initctl_name) ) < 0 )
>        {
>         fprintf (stderr, "Error opening %s: %s\n", initctl_name, ERRSTRING);
>         exit (1);
> @@ -240,11 +256,11 @@
>        fcntl(fd,F_SETFL,arg);
>      }  
>      if (userspace){
> -       int fd;
> -       if (((fd=open(pidfile_name,O_RDONLY)) < 0) ||
> -           (read(fd,&init_pid,sizeof(init_pid)) < 
> (ssize_t)sizeof(init_pid) )
> +       int pfd;
> +       if (((pfd=open(pidfile_name,O_RDONLY)) < 0) ||
> +           (read(pfd,&init_pid,sizeof(init_pid)) < 
> (ssize_t)sizeof(init_pid) )
>            ) {fprintf(stderr,"Error reading init.pid\n"); exit(1); }
> -       close(fd);   
> +       close(pfd);   
>      }
>      
>      command->command |= command_flags;
> @@ -253,12 +269,13 @@
>       fprintf (stderr, "Error writing %s: %s\n", initctl_name, ERRSTRING);
>       exit (1);
>      }
> -    close (fd);
> +
>      kill(init_pid,SIGCHLD); /* wake up init if sleeping, to process 
> command */
>      command->command &= CLEAR_FLAGS;
>      
>      if (command->command != COMMAND_DUMP_LIST)
>      {
> +      close(fd);
>       sigemptyset (&ss);
>       while (caught_signal == 0) sigsuspend (&ss);
>       switch (command->command)
> @@ -295,20 +312,12 @@
>       }
>       return 3;
>      }
> -    /*  Read back the data and display it  */
> -    /*  The open call blocks until the other end is opened, too */
> -    if ( ( fd = open (command->name, O_RDONLY, 0) ) < 0 )
> -    {
> -     fprintf (stderr, "Error opening:\"%s\"\t%s\n",
> -              command->name, ERRSTRING);
> -     exit (1);
> -    }
> -    unlink (command->name); /* fifo will keep working till fd is closed */
> +
>      fflush (stdout);
> -    while ( ( nbytes = read (fd, buffer, COMMAND_SIZE) ) > 0 )
> +    while ( ( nbytes = read (fd, buffer, sizeof(buffer)) ) > 0 )
>       write (1, buffer, nbytes);
>      close (fd);
> -    return (0);
> +    return 0;
>  }   /*  End Function main  */
>  
>  static void signal_handler (int sig)
> diff -ur simpleinit-msb-1.2-orig/login-utils/simpleinit.c 
> simpleinit-msb-1.2-jel/login-utils/simpleinit.c
> --- simpleinit-msb-1.2-orig/login-utils/simpleinit.c  2008-03-09 
> 12:26:26.000000000 +0100
> +++ simpleinit-msb-1.2-jel/login-utils/simpleinit.c   2008-03-12 
> 13:47:07.000000000 +0100
> @@ -203,6 +203,8 @@
>   */
>  
>  #include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <stdio.h>
> @@ -290,7 +292,7 @@
>  static int sigint_critical = 1; 
>  static int have_cad_prog = 0;
>  static int userspace = 0;  /*1 means init has been started by user not 
> kernel*/
> -static int initctl_fd = -1;
> +static int initctl_socket = -1;
>  static sigset_t all_signals,no_signals,oldsigmask;
>  static int sigenum;
>  
> @@ -324,7 +326,7 @@
>  void sigquit_handler (int sig);
>  void userspace_sigquit_handler (int sig);
>  void sigterm_handler (int sig);
> -void open_initctl_fifo (void);
> +void open_initctl_socket (void);
>  void sigusr_handler (int sig);
>  #ifdef SET_TZ
>  void set_tz (void);
> @@ -445,7 +447,7 @@
>                       }       
>       }
>  
> -  open_initctl_fifo();
> +  open_initctl_socket();
>  
>       if (!no_selinux) {
>               if (load_policy() != 0) {
> @@ -465,7 +467,7 @@
>       
>       if (!userspace)
>         for (i = 0; i < getdtablesize (); i++)
> -             if (i != initctl_fd) close (i);
> +             if (i != initctl_socket) close (i);
>  
>       /*If we get a SIGTSTP before multi-user mode, do nothing*/
>       while(stopped)  
> @@ -718,7 +720,7 @@
>               inittab[i].pid = -1;
>               inittab[i].avg_respawn_delay = -1;
>               inittab[i].last_start = ct;
> -             sprintf (txt,"respawning: \"%s\" too fast: quenching entry\n",
> +             snprintf (txt, sizeof(txt), "respawning: \"%s\" too fast: 
> quenching entry\n",
>                        inittab[i].tty);
>               err (_(txt));
>               return;
> @@ -754,7 +756,7 @@
>               for(j = 0; j < getdtablesize(); j++)
>                       (void) close(j);
>  
> -             sprintf(term, "TERM=%s", inittab[i].termcap);
> +             snprintf(term, sizeof(term), "TERM=%s", inittab[i].termcap);
>               env[0] = term;
>               env[1] = NULL;
>  #ifdef SET_TZ
> @@ -899,7 +901,7 @@
>  
>  #ifdef SPECIAL_CONSOLE_TERM
>               /* special-case termcap for the console ttys */
> -             (void) sprintf(tty, "/dev/%s", inittab[i].tty);
> +             (void) snprintf(tty, sizeof(tty), "/dev/%s", inittab[i].tty);
>               if(!termenv || stat(tty, &stb) < 0) {
>                       err(_("no TERM or cannot stat tty\n"));
>               } else {
> @@ -950,25 +952,59 @@
>      if (!stopped) hup_handler (sig);
>  }   /*  End Function sigtstp_handler  */
>  
> -void open_initctl_fifo()
> +void open_initctl_socket()
>  {
> -     if ( ( initctl_fd = open (initctl_name, O_RDWR|O_NONBLOCK, 0) ) < 0 ) 
> {
> -             mkfifo (initctl_name, S_IRUSR | S_IWUSR);
> -             if ( ( initctl_fd = open (initctl_name, O_RDWR|O_NONBLOCK, 0) 
> ) < 0 )
> -                     err ( _("error opening fifo\n") );
> -     }
> -     fcntl(initctl_fd,F_SETFD,FD_CLOEXEC); /*make sure no one inherits the 
> fd*/
> +  struct sockaddr_un src;
> +  socklen_t srcsize = 0;
> +  int flags;
> +  
> +  srcsize = sizeof(src);
> +
> +  memset(&src, 0, sizeof(src));
> +  
> +  src.sun_family = AF_LOCAL;
> +  if( (initctl_socket = socket(src.sun_family, SOCK_STREAM, 0)) == -1)
> +    {
> +      err ( _("error creating socket\n") );
> +      return;
> +    }
> +
> +  strcpy(src.sun_path+1, INITSOCKETNAME);
> +  
> +  if( bind(initctl_socket, (struct sockaddr*)&src, srcsize) == -1)
> +    {
> +      err ( _("error binding socket\n") );
> +      close(initctl_socket);
> +      return;
> +    }
> +
> +  flags = fcntl(initctl_socket, F_GETFL);
> +  if(fcntl(initctl_socket, F_SETFL, flags | O_NONBLOCK)==-1)
> +    {
> +      err ( _("error set socket nonblock\n") );
> +      close(initctl_socket);
> +      return;
> +    }    
> +  
> +  if(listen(initctl_socket, 3) == -1)
> +    {
> +      err ( _("error listen socket\n") );
> +      close(initctl_socket);
> +      return;
> +    }
> +
> +  fcntl(initctl_socket,F_SETFD,FD_CLOEXEC); /*make sure no one inherits 
> the fd*/
>  }
>  
>  void sigusr_handler (int sig)
>  {
> +  /* why are we reopening fds 0,1,2 on /dev/console? */
>      close (0);
>      close (1);
>      close (2);
>         open ("/dev/console", O_RDONLY, 0);
>         open ("/dev/console", O_RDWR, 0);
>         dup2 (1, 2);
> -    open_initctl_fifo();
>      sig=sig;
>  }
>  
> @@ -1222,7 +1258,7 @@
>  static void finish_rollback(int sig);
>  static void cleanup_and_continue_rollback(void);
>  static int process_pidstat (pid_t pid, int status);
> -static void process_command (unsigned command,const struct command_struct 
> *comdata);
> +static void process_command (unsigned command,const struct command_struct 
> *comdata, int fd);
>  static struct service_struct *find_service_in_list (const char *name,
>                                                   struct service_struct 
> *sv);
>  static struct script_struct *find_script_byname
> @@ -1244,6 +1280,24 @@
>  static void output_deadlock_scripts( const struct script_struct* start, 
>                                       const struct script_struct* stop,
>                                       const struct service_struct* service);
> +static int verify_forging(int fd);
> +
> +/*
> + * Check that caller has same uid as us or is root.
> + * Written by Alexey Kuznetsov (taken from iproute2)
> + */
> +static int verify_forging(int fd)
> +{
> +  struct ucred cred;
> +  socklen_t olen = sizeof(cred);
> +  
> +  if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, (void*)&cred, &olen) ||
> +      olen < sizeof(cred))
> +    return -1;
> +  if (cred.uid == getuid() || cred.uid == 0)
> +    return 0;
> +  return -1;
> +}
>  
>  pid_t mywait (int *status)
>  /*  [RETURNS] The pid for a process to be reaped, 0 if no process is to be
> @@ -1252,11 +1306,11 @@
>  {
>      char buffer[COMMAND_SIZE];
>      pid_t pid;
> -    int ival;
> +    int ival, fd;
>      sigset_t oldss, suspss;
>      struct command_struct *command = (struct command_struct *) buffer;
>  
> -    if (initctl_fd < 0) return waitpid (-1, status, 0);
> +    if (initctl_socket < 0) return waitpid (-1, status, 0);
>      if (status == NULL) status = &ival;
>      
>      sigprocmask(SIG_SETMASK,NULL,&oldss);
> @@ -1273,12 +1327,17 @@
>       return process_pidstat (pid, *status);
>        }
>        
> -      if ((read(initctl_fd, buffer, COMMAND_SIZE))==COMMAND_SIZE)
> -      {
> -        sigprocmask(SIG_SETMASK,&oldss,NULL);
> -        process_command(command->command,command);
> -        return 0;
> -      }
> +      if( (fd = accept(initctl_socket, NULL, NULL)) >= 0)
> +     {
> +       if(verify_forging(fd) == 0)
> +         if ((read(fd, buffer, COMMAND_SIZE))==COMMAND_SIZE)
> +           {
> +             sigprocmask(SIG_SETMASK,&oldss,NULL);
> +             process_command(command->command,command,fd);
> +             return 0;
> +           }
> +       close(fd);
> +     }
>        sigsuspend(&suspss);
>      }  
>  }   /*  End Function mywait  */
> @@ -1386,7 +1445,7 @@
>       free (victim);
>       if (!quiet_rollback) err (txt);
>      }        
> -    process_command(COMMAND_ROLLBACK,NULL); /* continue rollback */
> +    process_command(COMMAND_ROLLBACK,NULL,-1); /* continue rollback */
>  }
>  
>  static void finish_rollback(int sig)
> @@ -1397,7 +1456,7 @@
>       rb_forced=0;
>  }
>  
> -static void process_command (unsigned command,const struct command_struct 
> *comdata)
> +static void process_command (unsigned command,const struct command_struct 
> *comdata, int fd)
>  {
>      int ival;
>      struct script_struct *script,*script2;
> @@ -1412,6 +1471,8 @@
>      You should kill the requester with
>      SIG_ROLLBACK in that case (unless the function is unaffected like
>      COMMAND_TEST and COMMAND_DUMP_LIST)*/
> +
> +    if((fd >=0) && (command != COMMAND_DUMP_LIST)) close(fd);
>      
>      switch (command)
>      {
> @@ -1610,6 +1671,7 @@
>       }
>       break;
>        case COMMAND_DUMP_LIST:
> +     /* DUMP_LIST is the only command that returns data */
>          sigprocmask(SIG_BLOCK,&all_signals,&oldsigmask);
>       if (fork () == 0) /* Do it in a child process so pid=1 doesn't block 
> */
>       {
> @@ -1617,7 +1679,7 @@
>  
>           for (sigenum=1; sigenum<NSIG; ++sigenum) signal(sigenum, SIG_DFL);
>           sigprocmask(SIG_UNBLOCK,&all_signals,NULL);
> -         if ( ( fp = fopen (comdata->name, "w") ) == NULL ) _exit (1);
> +         if ( ( fp = fdopen (fd, "r+") ) == NULL ) _exit (1);
>           show_scripts (fp, available_list.first, "AVAILABLE");
>           show_scripts (fp, starting_list.first, "STARTING");
>           fputs ("UNAVAILABLE SERVICES:\n", fp);
> @@ -1627,7 +1689,9 @@
>                        service->failed ? "FAILED" : "not configured");
>           fclose (fp);
>           _exit (0);
> -     } else sigprocmask(SIG_SETMASK,&oldsigmask,NULL);
> +     } 
> +     sigprocmask(SIG_SETMASK,&oldsigmask,NULL);
> +     close(fd);
>       break;
>        case COMMAND_PROVIDE:
>       if (rb_script != RB_NOROLLBACK){
> @@ -1800,7 +1864,7 @@
>             }
>             
>             exec_with_params (fpath, start, NULL);
> -           sprintf (txt, "errno %u running program: \"%s\"\n", errno, 
> fpath);
> +           snprintf (txt, sizeof(txt), "errno %u running program: 
> \"%s\"\n", errno, fpath);
>             err ( _(txt) );
>             _exit (SIG_FAILED);
>           }  
> Only in simpleinit-msb-1.2-orig/login-utils: simpleinit.c~
> diff -ur simpleinit-msb-1.2-orig/login-utils/simpleinit.h 
> simpleinit-msb-1.2-jel/login-utils/simpleinit.h
> --- simpleinit-msb-1.2-orig/login-utils/simpleinit.h  2001-09-26 
> 12:59:46.000000000 +0200
> +++ simpleinit-msb-1.2-jel/login-utils/simpleinit.h   2008-03-11 
> 11:10:08.000000000 +0100
> @@ -25,6 +25,8 @@
>  #define SIG_ROLLBACK    SIGURG   /*  Rollback in progress                 
> */
>  #define SIG_NO_INFO     SIGURG   /*  No info about service available      
> */
>  
> +#define INITSOCKETNAME "/simpleinit"
> +
>  struct command_struct  /*  Must always be COMMAND_SIZE  */
>  {
>      unsigned int command;
> Only in simpleinit-msb-1.2-jel: make_include

> _______________________________________________
> SM-Discuss mailing list
> SM-Discuss AT lists.ibiblio.org
> http://lists.ibiblio.org/mailman/listinfo/sm-discuss