Currently Nagios uses popen and system to run active check commands, with shell intepretation. Suggest we use execv (or similar) instead so that there is no shell expansion required.

This means that 1 less process (sh) is required to execute an active check, which should give a performance improvement.

When running the active check, see if there are any shell metacharacters. If there are, then fallback to the shell invocation. Otherwise use the new execv method.

From: Hiren Patel  gmail.com>
Subject: tracker id 86 - software freedom day event
Newsgroups: gmane.network.nagios.devel
Date: 2009-09-19 21:31:52 GMT (34 weeks, 19 hours and 6 minutes ago)

the Johannesburg South Africa software freedom day event had a session 
where users could work on bugs/features on any project of their choice, 
I chose to work on tracker id 86 in the event.

attached are diffs for the lists review, only very basic testing has 
been done. any further testing or comments and concerns appreciated.

one area I'm not too sure of, is if waitpid can be interrupted by a 
signal and needs to be handled differently to what it currently is.

I'd like to take this opportunity on software freedom day to thank the 
nagios project for its work, and the free software community at large.

--- nagios/base/checks.c    2009-08-11 19:29:52.000000000 +0200
+++ checks.c    2009-09-19 23:12:46.000000000 +0200
@@ -338,7 +338,7 @@
    int fork_error=FALSE;
    int wait_result=0;
    host *temp_host=NULL;
-   FILE *fp=NULL;
+   FILE *fp=NULL, *chldfp;
    int pclose_result=0;
    mode_t new_umask=077;
    mode_t old_umask;
@@ -346,6 +346,9 @@
    double old_latency=0.0;
    dbuf checkresult_dbuf;
    int dbuf_chunk=1024;
+   int pipefds[2], chldstatus, i;
+   char *chldargs[MAXCHLDARGS];
+   char *s , *p;
 #ifdef USE_EVENT_BROKER
    int neb_result=OK;
 #endif
@@ -771,22 +774,102 @@


            /* run the plugin check command */
-           fp=popen(processed_command,"r");
-           if(fp==NULL)
-               _exit(STATE_UNKNOWN);
+           if (!has_shell_metachars(processed_command)){
+               if (pipe(pipefds) == -1){
+                   logit(NSLOG_RUNTIME_WARNING,TRUE,"error creating pipe: %s\n",strerror(errno));
+                   _exit(STATE_UNKNOWN);
+               }

-           /* initialize buffer */
-           strcpy(output_buffer,"");
+               pid = fork();
+               if (pid == -1){
+                   logit(NSLOG_RUNTIME_WARNING,TRUE,"fork error\n");
+                   _exit(STATE_UNKNOWN);
+               }

-           /* get all lines of plugin output - escape newlines */
-           while(fgets(output_buffer,sizeof(output_buffer)-1,fp)){
-               temp_buffer=escape_newlines(output_buffer);
-               dbuf_strcat(&checkresult_dbuf,temp_buffer);
-               my_free(temp_buffer);
+               if (pid == 0){
+                   close(pipefds[0]);
+
+                   if (dup2(pipefds[1],STDOUT_FILENO) == -1){
+                       logit(NSLOG_RUNTIME_WARNING,TRUE,"dup2 error\n");
+                       _exit(EXIT_FAILURE);
+                   }
+
+                   if (dup2(pipefds[1],STDERR_FILENO) == -1){
+                       logit(NSLOG_RUNTIME_WARNING,TRUE,"dup2 error\n");
+                       _exit(EXIT_FAILURE);
+                   }
+                   close(pipefds[0]);
+
+                   s = strchr(processed_command,' ');
+                   if (s){
+                       *s = '\0';
+                       p = s+1;
+
+                       chldargs[0] = processed_command;
+                       for(i=1;i= MAXCHLDARGS-2){
+                           logit(NSLOG_RUNTIME_WARNING,TRUE,"overlimit args for command %s\n",chldargs[0]);
+                           _exit(EXIT_FAILURE);
+                       }
+                       else
+                           chldargs[++i] = '\0';
+                   }
+                   else{
+                       chldargs[0] = processed_command;
+                       chldargs[1] = '\0';
+                   }
+
+                   log_debug_info(DEBUGL_CHECKS,0,"running process %s via execv\n",processed_command);
+                   execv(chldargs[0],chldargs);
+                   _exit(EXIT_FAILURE);
                }

-           /* close the process */
-           pclose_result=pclose(fp);
+               close(pipefds[1]);
+
+               chldfp = fdopen(pipefds[0],"r");
+               if (chldfp == NULL){
+                   logit(NSLOG_RUNTIME_WARNING,TRUE,"fdopen error\n");
+                   _exit(EXIT_FAILURE);
+               }
+
+               while(fgets(output_buffer,sizeof(output_buffer)-1,chldfp)){
+                   temp_buffer=escape_newlines(output_buffer);
+                   dbuf_strcat(&checkresult_dbuf,temp_buffer);
+                   my_free(temp_buffer);
+               }
+
+               fclose(chldfp);
+               waitpid(pid,&chldstatus,0);
+               pclose_result=WEXITSTATUS(chldstatus);
+           }
+           else{
+               log_debug_info(DEBUGL_CHECKS,0,"running process %s via popen\n",processed_command);
+               fp=popen(processed_command,"r");
+               if(fp==NULL)
+                   _exit(STATE_UNKNOWN);
+   
+               strcpy(output_buffer,"");
+   
+               while(fgets(output_buffer,sizeof(output_buffer)-1,fp)){
+                   temp_buffer=escape_newlines(output_buffer);
+                   dbuf_strcat(&checkresult_dbuf,temp_buffer);
+                   my_free(temp_buffer);
+                   }
+   
+               pclose_result=pclose(fp);
+           }

            /* reset the alarm */
            alarm(0);

--- nagios/include/nagios.h 2008-12-14 16:52:23.000000000 +0200
+++ nagios.h    2009-09-19 18:18:08.000000000 +0200
@@ -44,6 +44,7 @@
    command file. EG 10/19/07
 */
 #define MAX_PLUGIN_OUTPUT_LENGTH                8192    /* max length of plugin output (including perf data) */
+#define MAXCHLDARGS    20



@@ -807,6 +808,7 @@

 char *get_program_version(void);
 char *get_program_modification_date(void);
+int has_shell_metachars(const char *);

 mmapfile *mmap_fopen(char *);              /* open a file read-only via mmap() */
 int mmap_fclose(mmapfile *);

--- utils.c 2009-08-11 19:29:52.000000000 +0200
+++ /tmp/utils.c    2009-09-19 13:31:19.000000000 +0200
@@ -4589,7 +4589,12 @@
    return (char *)PROGRAM_MODIFICATION_DATE;
         }

-
+int has_shell_metachars(const char *s){
+   if (strpbrk(s,"!$^&*()~[]\\|{};<>?'\""))
+       return 1;
+   else
+       return 0;
+}

 /******************************************************************/
 /*********************** CLEANUP FUNCTIONS ************************/

modify execv to execvp, accepting 4096 cmd args, for both host and service checks with adapted error handling

the previous version only allowed 20-1 chld args, and was limited on the
error output. it is now a combined diff from Hiren, Christoph and Matthieu,
having all features applied and several bugs fixed (pipe returns -1 on error)

Furthermore, it's also applied for host checks.

execvp searches in path too, e.g. if plugin path is not set correctly but
is in PATH.

The run check procedure still falls back on popen execution, if the plugin
command contains shell meta characters. (Can be compared to Perl's exec vs
system)

kudos to Hiren Patel, Christoph Maser and Matthieu Kermagoret

refs #436

execvp searches in PATH too like popen, and returns if an error occurs, outputting the error string

revamped the previous attempt a bit. there might be discussions about
allowing PATH or not PATH env variables. But by completely breaking
compatibility this won't be nice.

the current attempt only checks if the first argument is set and then
calling execvp - this looks in $PATH if there is no trailing slash set
on the executable.
In case of an error, execvp returns -1 and this is where the error is
put into syslog, exiting with state unknown. (a further adaption would be
setting the output of the plugin like popen behaves)

if you consider not using the PATH for that, you can simply change execvp to
execv and recompile the code. The checks on executable/there are still valid,
except PATH lookups.

refs #436
refs #617

modify execv to execvp, accepting 4096 cmd args, for both host and service checks with adapted error handling

the previous version only allowed 20-1 chld args, and was limited on the
error output. it is now a combined diff from Hiren, Christoph and Matthieu,
having all features applied and several bugs fixed (pipe returns -1 on error)

Furthermore, it's also applied for host checks.

execvp searches in path too, e.g. if plugin path is not set correctly but
is in PATH.

The run check procedure still falls back on popen execution, if the plugin
command contains shell meta characters. (Can be compared to Perl's exec vs
system)

kudos to Hiren Patel, Christoph Maser and Matthieu Kermagoret

refs #436

execvp searches in PATH too like popen, and returns if an error occurs, outputting the error string

revamped the previous attempt a bit. there might be discussions about
allowing PATH or not PATH env variables. But by completely breaking
compatibility this won't be nice.

the current attempt only checks if the first argument is set and then
calling execvp - this looks in $PATH if there is no trailing slash set
on the executable.
In case of an error, execvp returns -1 and this is where the error is
put into syslog, exiting with state unknown. (a further adaption would be
setting the output of the plugin like popen behaves)

if you consider not using the PATH for that, you can simply change execvp to
execv and recompile the code. The checks on executable/there are still valid,
except PATH lookups.

refs #436
refs #617