Bug 54343

Summary: Add sanity checks for malloc() in mpm_winnt.c and service.c
Product: Apache httpd-2 Reporter: Bill Parker <wp02855>
Component: mpm_winntAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: NEW ---    
Severity: major    
Priority: P2    
Version: 2.4.3   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: patch file for above description
adds sanity check for malloc() in service.c

Description Bill Parker 2012-12-22 18:33:49 UTC
Created attachment 29789 [details]
patch file for above description

Hello All,

	In reviewing code in directory 'httpd-2.4.3/server/mpm/winnt',
file 'mpm_winnt.c', function 'create_process()', I found two
instances where malloc() is called without a check for the return
value of NULL, indicating failure.  Additionally, I found nowhere
in the function where the memory allocated to 'arg' and 'arg[0]'
is ever released by a call to free().  The patch below adds the
tests in the case where malloc() fails:

--- mpm_winnt.c.orig    2012-12-21 16:45:58.769855888 -0800
+++ mpm_winnt.c 2012-12-22 07:04:44.133555336 -0800
@@ -582,9 +582,21 @@
         }
 
         args = malloc((ap_server_conf->process->argc + 1) * sizeof (char*));
+        if (args == NULL) {
+            /* the fprintf below should be replaced with the equiv ap_log_error() call */
+            fprintf(stderr, "Unable to allocate memory for args in create_process()\n");
+            apr_pool_destroy(ptemp);
+            return -1;
+        }
         memcpy(args + 1, ap_server_conf->process->argv + 1,
                (ap_server_conf->process->argc - 1) * sizeof (char*));
         args[0] = malloc(strlen(cmd) + 1);
+        if (args[0] == NULL) {
+            /* the fprintf below should be replaced with the equiv ap_log_error() call */
+            fprintf(stderr, "Unable to allocate memory for args in create_process()\n");
+            apr_pool_destroy(ptemp);
+            return -1;
+        }
         strcpy(args[0], cmd);
         args[ap_server_conf->process->argc] = NULL;
     }
@@ -700,6 +712,8 @@
     *child_proc = new_child.hproc;
     *child_pid = new_child.pid;
 
+    free(args); /* release memory previously alloc'd */
+
     return 0;
	 
I was not sure what the correct call for 'ap_log_error()' is :(

In directory 'httpd-2.4.3/server/mpm/winnt', file 'service.c',
function 'service_nt_main_fn()', I found an instance of malloc()
being called without the return value check for NULL, indicating
failure.  In function 'mpm_merge_service_args()', I found an
instance of malloc() being called without the return value check
for NULL, indicating failure.  In function 'mpm_service_start()'
I found an instance of malloc() being called without the return
value check for NULL, indicating failure.

The patch file below adds the test for NULL being returned from
a call to malloc():

--- service.c.orig      2012-12-21 16:51:42.112518239 -0800
+++ service.c   2012-12-22 07:11:24.950522079 -0800
@@ -331,6 +331,12 @@
 
         mpm_new_argv->nalloc = mpm_new_argv->nelts + argc - 1;
         cmb_data = malloc(mpm_new_argv->nalloc * sizeof(const char *));
+        if (cmb_data == NULL) {
+            /* the fprintf below should be replaced with the appropriate call to ap_log_error() */
+
+            fprintf(stderr, "Unable to allocate memory for cmb_data in service_nt_main_fn()\n");
+            return;
+        }
 
         /* mpm_new_argv remains first (of lower significance) */
         memcpy (cmb_data, mpm_new_argv->elts,
@@ -351,6 +357,8 @@
     SetEvent(ctx->service_init);
 
     WaitForSingleObject(ctx->service_term, INFINITE);
+
+    free(cmd_data); /* release memory previously allocated by malloc() */
 }
 
 
@@ -454,6 +462,11 @@
      */
     args->nalloc = args->nelts + svc_args->nelts;
     cmb_data = malloc(args->nalloc * sizeof(const char *));
+    if (cmb_data == NULL) {
+        /* the fprintf below should be replaced with the appropriate call to ap_log_error() */
+        fprintf(stderr, "Unable to allocate memory for cmb_data in mpm_merge_service_args()\n");
+        return;
+    }
 
     /* First three args (argv[0], -f, path) remain first */
     memcpy(cmb_data, args->elts, args->elt_size * fixed_args);
@@ -470,6 +483,8 @@
     args->elts = (char *)cmb_data;
     args->nelts = args->nalloc;
 
+    free(cmb_data); /* release memory previously allocated by malloc() */
+
     return APR_SUCCESS;
 }
 
@@ -786,6 +801,11 @@
     }
 
     start_argv = malloc((argc + 1) * sizeof(const char **));
+    if (start_argv == NULL) {
+        /* the fprintf below should be replaced with the appropriate call to ap_log_error() */
+        fprintf(stderr, "Unable to allocate memory for start_argv in mpm_service_start()\n");
+        return;
+    }
     memcpy(start_argv, argv, argc * sizeof(const char **));
     start_argv[argc] = NULL;
 
@@ -809,6 +829,8 @@
                      "%s: Failed to start the service process.",
                      mpm_display_name);
 
+    free(start_argv);   /* release memory previously allocated by malloc() */
+
     return rv;
 }
  
Additionally, in the functions above, I found no cases in which
a successful allocation of memory is ever released by free()
prior to the function returning, which could potentially cause
a memory leak (could it not)?
Comment 1 Bill Parker 2012-12-22 18:34:35 UTC
Created attachment 29790 [details]
adds sanity check for malloc() in service.c
Comment 2 Stefan Fritsch 2012-12-25 21:35:28 UTC
In general, replacing malloc with ap_malloc should do the right thing. Do you have a Windows system and can test that?
Comment 3 Bill Parker 2015-05-07 21:14:07 UTC
I'm in agreement here, ap_malloc() works even on my antiquated Windows XP system that I still have hanging around.  Would someone merge/incorporate the patch into the 2.4.12 source tree?
Comment 4 Giovanni Bechis 2020-01-29 21:17:14 UTC
Conversion from malloc to ap_malloc available in bz 64049 (https://bz.apache.org/bugzilla/attachment.cgi?id=36958&action=diff)