thr3ads.net - Icecast dev - [Icecast-dev] You don't check for malloc failure [May 2011]

If this information is useful, please help other people find it:
Share via:

Peter J. Philipp

2011-Apr-29 07:05 UTC

[Icecast-dev] You don't check for malloc failure

Hi,

You don't check for malloc failure.  I've made a patch that is possibly 
wrong but it saves the program from SIGSEGV and replaces it with SIGABRT.

-peter

diff -ru icecast-2.3.2-ORIG/src/admin.c icecast-2.3.2/src/admin.c
--- icecast-2.3.2-ORIG/src/admin.c	Fri May  2 17:40:00 2008
+++ icecast-2.3.2/src/admin.c	Thu Apr 28 19:53:03 2011
@@ -291,6 +291,9 @@
         fullpath_xslt_template_len = strlen (config->adminroot_dir) + 
             strlen (xslt_template) + 2;
         fullpath_xslt_template = malloc(fullpath_xslt_template_len);
+	if (fullpath_xslt_template == NULL) {
+		abort();
+	}
         snprintf(fullpath_xslt_template, fullpath_xslt_template_len,
"%s%s%s",
             config->adminroot_dir, PATH_SEPARATOR, xslt_template);
         config_release_config();
diff -ru icecast-2.3.2-ORIG/src/auth_htpasswd.c
icecast-2.3.2/src/auth_htpasswd.c
--- icecast-2.3.2-ORIG/src/auth_htpasswd.c	Wed Apr 23 03:55:22 2008
+++ icecast-2.3.2/src/auth_htpasswd.c	Thu Apr 28 19:53:15 2011
@@ -159,6 +159,9 @@
         entry = calloc (1, sizeof (htpasswd_user));
         len = strlen (line) + 1;
         entry->name = malloc (len);
+	if (entry->name == NULL) {
+		abort();
+	}
         *sep = 0;
         memcpy (entry->name, line, len);
         entry->pass = entry->name + (sep-line) + 1;
diff -ru icecast-2.3.2-ORIG/src/auth_url.c icecast-2.3.2/src/auth_url.c
--- icecast-2.3.2-ORIG/src/auth_url.c	Thu Nov  8 16:38:01 2007
+++ icecast-2.3.2/src/auth_url.c	Thu Apr 28 19:53:24 2011
@@ -219,6 +219,9 @@
             {
                 int len = strlen (client->username) + strlen
(client->password) + 2;
                 userpwd = malloc (len);
+		if (userpwd == NULL) {
+			abort();
+		}
                 snprintf (userpwd, len, "%s:%s", client->username,
client->password);
                 curl_easy_setopt (url->handle, CURLOPT_USERPWD, userpwd);
             }
@@ -307,6 +310,9 @@
             {
                 int len = strlen (client->username) + strlen
(client->password) + 2;
                 userpwd = malloc (len);
+		if (userpwd == NULL) {
+			abort();
+		}
                 snprintf (userpwd, len, "%s:%s", client->username,
client->password);
                 curl_easy_setopt (url->handle, CURLOPT_USERPWD, userpwd);
             }
diff -ru icecast-2.3.2-ORIG/src/cfgfile.c icecast-2.3.2/src/cfgfile.c
--- icecast-2.3.2-ORIG/src/cfgfile.c	Fri May  2 04:56:58 2008
+++ icecast-2.3.2/src/cfgfile.c	Thu Apr 28 19:53:33 2011
@@ -964,6 +964,9 @@
                
configuration->adminroot_dir[strlen(configuration->adminroot_dir)-1] = 0;
         } else if (xmlStrcmp (node->name, XMLSTR("alias")) == 0) {
             alias = malloc(sizeof(aliases));
+            if (alias == NULL) {
+		abort();
+            }
             alias->next = NULL;
             alias->source = (char *)xmlGetProp(node,
XMLSTR("source"));
             if(alias->source == NULL) {
@@ -1073,6 +1076,9 @@
     char *tmp;
 
     server = (ice_config_dir_t *)malloc(sizeof(ice_config_dir_t));
+    if (server == NULL) {
+	abort();
+    }
     server->touch_interval = configuration->touch_interval;
     server->host = NULL;
     addnode = 0;
diff -ru icecast-2.3.2-ORIG/src/connection.c icecast-2.3.2/src/connection.c
--- icecast-2.3.2-ORIG/src/connection.c	Wed Dec 19 19:58:00 2007
+++ icecast-2.3.2/src/connection.c	Thu Apr 28 19:53:43 2011
@@ -532,6 +532,9 @@
 
     /* malloc enough room for a full IP address (including ipv6) */
     ip = (char *)malloc(MAX_ADDR_LEN);
+    if (ip == NULL) {
+	abort();
+    }
 
     sock = sock_accept(serversock, ip, MAX_ADDR_LEN);
     if (sock != SOCK_ERROR)
diff -ru icecast-2.3.2-ORIG/src/format_vorbis.c
icecast-2.3.2/src/format_vorbis.c
--- icecast-2.3.2-ORIG/src/format_vorbis.c	Thu Sep 13 00:40:55 2007
+++ icecast-2.3.2/src/format_vorbis.c	Thu Apr 28 19:54:26 2011
@@ -401,6 +401,9 @@
     plugin->set_tag = vorbis_set_tag;
 
     vorbis->bos_page.header = malloc (page->header_len +
page->body_len);
+    if (vorbis->bos_page.header == NULL) {
+	abort();
+    }
     
     memcpy (vorbis->bos_page.header, page->header, page->header_len);
     vorbis->bos_page.header_len = page->header_len;
diff -ru icecast-2.3.2-ORIG/src/fserve.c icecast-2.3.2/src/fserve.c
--- icecast-2.3.2-ORIG/src/fserve.c	Tue Apr 29 06:32:10 2008
+++ icecast-2.3.2/src/fserve.c	Thu Apr 28 19:54:34 2011
@@ -727,6 +727,9 @@
                 void *tmp;
                 /* Add a new extension->type mapping */
                 mapping = malloc(sizeof(mime_type));
+		if (mapping == NULL) {
+			abort();
+		}
                 mapping->ext = strdup(ext);
                 mapping->type = strdup(type);
                 if (!avl_get_by_key (new_mimetypes, mapping, &tmp))
diff -ru icecast-2.3.2-ORIG/src/log/log.c icecast-2.3.2/src/log/log.c
--- icecast-2.3.2-ORIG/src/log/log.c	Thu Jan 24 04:10:20 2008
+++ icecast-2.3.2/src/log/log.c	Thu Apr 28 19:24:11 2011
@@ -368,6 +368,9 @@
     
     entry = calloc (1, sizeof (log_entry_t));
     entry->line = malloc (LOG_MAXLINELEN);
+    if (entry->line == NULL) {
+	abort();
+    }
     len = snprintf (entry->line, LOG_MAXLINELEN, "%s%s\n", pre,
line);
     entry->len = len;
     loglist [log_id].total += len;
@@ -402,6 +405,9 @@
     _lock_logger ();
     remain = loglist [log_id].total + 1;
     *_contents = malloc (remain);
+    if (*_contents == NULL) {
+	abort();
+    }
     **_contents= '\0';
     *_len = loglist [log_id].total;
 
diff -ru icecast-2.3.2-ORIG/src/slave.c icecast-2.3.2/src/slave.c
--- icecast-2.3.2-ORIG/src/slave.c	Tue Nov 20 06:01:27 2007
+++ icecast-2.3.2/src/slave.c	Thu Apr 28 19:55:04 2011
@@ -172,11 +172,17 @@
         unsigned len = strlen(relay->username) + strlen(relay->password)
+ 2;
 
         auth_header = malloc (len);
+	if (auth_header == NULL) {
+		abort();
+	}
         snprintf (auth_header, len, "%s:%s", relay->username,
relay->password);
         esc_authorisation = util_base64_encode(auth_header);
         free(auth_header);
         len = strlen (esc_authorisation) + 24;
         auth_header = malloc (len);
+	if (auth_header == NULL) {
+		abort();
+	}
         snprintf (auth_header, len,
                 "Authorization: Basic %s\r\n", esc_authorisation);
         free(esc_authorisation);
@@ -610,6 +616,9 @@
 
         len = strlen(username) + strlen(password) + 2;
         authheader = malloc(len);
+	if (authheader == NULL) {
+		abort();
+	}
         snprintf (authheader, len, "%s:%s", username, password);
         data = util_base64_encode(authheader);
         sock_write (mastersock,
diff -ru icecast-2.3.2-ORIG/src/source.c icecast-2.3.2/src/source.c
--- icecast-2.3.2-ORIG/src/source.c	Thu May 15 15:25:04 2008
+++ icecast-2.3.2/src/source.c	Thu Apr 28 19:55:17 2011
@@ -577,6 +577,9 @@
         strlen(":") + 6 + strlen(source->mount) + 1;
 
     listenurl = malloc (listen_url_size);
+    if (listenurl == NULL) {
+       abort();
+    }
     memset (listenurl, '\000', listen_url_size);
     snprintf (listenurl, listen_url_size, "http://%s:%d%s",
             config->hostname, config->port, source->mount);
@@ -1312,8 +1315,12 @@
         config = config_get_config();
         len  = strlen (config->webroot_dir) + strlen (mount) + 1;
         path = malloc (len);
-        if (path)
+        if (path) {
             snprintf (path, len, "%s%s", config->webroot_dir,
mount);
+	} else {
+		abort();
+	}
+		
         
         config_release_config ();
         if (path == NULL)
diff -ru icecast-2.3.2-ORIG/src/stats.c icecast-2.3.2/src/stats.c
--- icecast-2.3.2-ORIG/src/stats.c	Thu May  1 04:22:40 2008
+++ icecast-2.3.2/src/stats.c	Thu Apr 28 19:55:36 2011
@@ -327,6 +327,9 @@
     if (event)
     {
         event->value = malloc (16);
+	if (event->value == NULL) {
+		abort();
+	}
         snprintf (event->value, 16, "%ld", value);
         event->action = STATS_EVENT_ADD;
         queue_global_event (event);
@@ -339,6 +342,9 @@
     if (event)
     {
         event->value = malloc (16);
+	if (event->value == NULL) {
+		abort();
+	}
         snprintf (event->value, 16, "%ld", value);
         event->action = STATS_EVENT_SUB;
         queue_global_event (event);
@@ -462,6 +468,9 @@
                 break;
         }
         str = malloc (16);
+	if (str == NULL) {
+		abort();
+	}
         snprintf (str, 16, "%d", value);
         if (event->value == NULL)
             event->value = strdup (str);
@@ -687,6 +696,10 @@
 static stats_event_t *_make_event_from_node(stats_node_t *node, char *source)
 {
     stats_event_t *event = (stats_event_t *)malloc(sizeof(stats_event_t));
+
+    if (event == NULL) {
+	abort();
+    }
     
     if (source != NULL)
         event->source = (char *)strdup(source);
@@ -913,6 +926,9 @@
 
     /* build node */
     node = (source_xml_t *)malloc(sizeof(source_xml_t));
+    if (node == NULL) {
+	abort();
+    }
     node->mount = strdup(mount);
     node->node = xmlNewChild (root, NULL, XMLSTR("source"), NULL);
     xmlSetProp (node->node, XMLSTR("mount"), XMLSTR(mount));
diff -ru icecast-2.3.2-ORIG/src/thread/thread.c
icecast-2.3.2/src/thread/thread.c
--- icecast-2.3.2-ORIG/src/thread/thread.c	Wed Sep 14 15:04:12 2005
+++ icecast-2.3.2/src/thread/thread.c	Thu Apr 28 19:27:34 2011
@@ -169,6 +169,9 @@
     _threadtree = avl_tree_new(_compare_threads, NULL);
 
     thread = (thread_type *)malloc(sizeof(thread_type));
+    if (thread == NULL) {
+	abort();
+    } 
 
     thread->thread_id = _next_thread_id++;
     thread->line = 0;
diff -ru icecast-2.3.2-ORIG/src/util.c icecast-2.3.2/src/util.c
--- icecast-2.3.2-ORIG/src/util.c	Wed Oct 24 19:48:24 2007
+++ icecast-2.3.2/src/util.c	Thu Apr 28 19:55:57 2011
@@ -233,8 +233,11 @@
     webroot = config->webroot_dir;
 
     fullpath = malloc(strlen(uri) + strlen(webroot) + 1);
-    if (fullpath)
+    if (fullpath) {
         sprintf (fullpath, "%s%s", webroot, uri);
+    } else {
+	abort();
+    }
     config_release_config();
 
     return fullpath;
@@ -394,6 +397,9 @@
     char *hex = malloc(len*2 + 1);
     int i;
 
+    if (hex == NULL) {
+	abort();
+    }
     for(i = 0; i < len; i++) {
         hex[i*2] = hexchars[(data[i]&0xf0) >> 4];
         hex[i*2+1] = hexchars[data[i]&0x0f];
@@ -412,6 +418,9 @@
     char *result = out;
     int chunk;
 
+    if (out == NULL) {
+	abort();
+    }
     while(len > 0) {
         chunk = (len >3)?3:len;
         *out++ = base64table[(*data & 0xFC)>>2];
@@ -446,6 +455,9 @@
     char *result = out;
     signed char vals[4];
 
+    if (out == NULL) {
+	abort();
+    }
     while(len > 0) {
         if(len < 4)
         {

Philipp Schafft

2011-Apr-29 09:08 UTC

head link

[Icecast-dev] You don't check for malloc failure

reflum,

On Fri, 2011-04-29 at 09:05 +0200, Peter J. Philipp
wrote:> Hi,
> 
> You don't check for malloc failure.  I've made a patch that is
possibly
> wrong but it saves the program from SIGSEGV and replaces it with SIGABRT.
> 
> -peter
> 
Thanks for searching and finding those locations. :)

But I have a question:
Not all of them (only had a brief look at the patch) look to be in a
'safe' startup/shutdown state but code wich is run within the normal
operation. Wouldn't it be better to handle those cases in a diffrent way
because abort() will kill the process? Currently if it derefences NULL
the kernel will kill the process (very likely). This patch would improve
the situation because the behavor will be more defined wich is a big pro
already.

-- 
Philipp.
 (Rah of PH2)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 482 bytes
Desc: This is a digitally signed message part
Url :
http://lists.xiph.org/pipermail/icecast-dev/attachments/20110429/d28ed4b1/attachment.pgp

Maarten Bezemer

2011-May-07 22:39 UTC

head link

[Icecast-dev] You don't check for malloc failure

Hi,
> On Fri, 2011-04-29 at 09:05 +0200, Peter J. Philipp wrote:
>> You don't check for malloc failure.  I've made a patch that is
possibly
>> wrong but it saves the program from SIGSEGV and replaces it with
SIGABRT.
On Fri, 29 Apr 2011, Philipp Schafft wrote:> But I have a question:
> Not all of them (only had a brief look at the patch) look to be in a
> 'safe' startup/shutdown state but code wich is run within the
normal
> operation. Wouldn't it be better to handle those cases in a diffrent
way
> because abort() will kill the process? Currently if it derefences NULL
> the kernel will kill the process (very likely). This patch would improve
> the situation because the behavor will be more defined wich is a big pro
> already.
Most implementations of malloc() don't return NULL except for very obvious 
cases of bad programming.
When memory is running out, malloc() may return a valid pointer, but 
writing anything in the malloc-ed data region can still result in the 
kernel killing the process.

So, although I think checking the return values is a good thing, this is 
no guarantee at all that things will always run as expected.
(Besides, when running icecast on a Linux box, there's always the risk of 
being killed by the OOM killer even when you're not doing anything wrong.)


Just my 2 cents..

-- 
Maarten

Niv Sardi

2011-May-08 20:41 UTC

head link

[Icecast-dev] You don't check for malloc failure

Wow, I missed this patch for so long,

Thank you for the huge effort of finding all this and sending the patch.
That said, it doesn't seem like the right solution to abort() when out
of memory. what needs to be done is to return an error, and track down
the call path to handle gracefully.

in admin, return http err code,
in auth passwd, drop connection,
in auth url, drop connection,
in config probably print to stderr and abort();
in connection, drop connection,
in vorbis, close source, (that'll close clients and free a whole bunch
                          of memory)
...

that's a quick top-of-my head review, do you think you want to look a
bit deeper into that ?

Cheers,
-- 
Niv Sardi

Christian Mueller

2011-May-08 20:51 UTC

head link

[Icecast-dev] You don't check for malloc failure

probably one missing point...

being serious

> Wow, I missed this patch for so long,
>
> Thank you for the huge effort of finding all this and sending the patch.
> That said, it doesn't seem like the right solution to abort() when out
> of memory. what needs to be done is to return an error, and track down
> the call path to handle gracefully.
>
> in admin, return http err code,
> in auth passwd, drop connection,
> in auth url, drop connection,
> in config probably print to stderr and abort();
> in connection, drop connection,
> in vorbis, close source, (that'll close clients and free a whole bunch
>                           of memory)
> ...
>
> that's a quick top-of-my head review, do you think you want to look a
> bit deeper into that ?
>
> Cheers,
> --
> Niv Sardi
>
> _______________________________________________
> Icecast-dev mailing list
> Icecast-dev at xiph.org
> http://lists.xiph.org/mailman/listinfo/icecast-dev
>

Icecast dev - May 2011 - You don't check for malloc failure

[Icecast-dev] You don't check for malloc failure

[Icecast-dev] You don't check for malloc failure

[Icecast-dev] You don't check for malloc failure

[Icecast-dev] You don't check for malloc failure

[Icecast-dev] You don't check for malloc failure