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)?
Created attachment 29790 [details] adds sanity check for malloc() in service.c
In general, replacing malloc with ap_malloc should do the right thing. Do you have a Windows system and can test that?
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?
Conversion from malloc to ap_malloc available in bz 64049 (https://bz.apache.org/bugzilla/attachment.cgi?id=36958&action=diff)