Bug 10021 - Subj: Lack of Sanity Checking in calls to malloc()/calloc()
Subj: Lack of Sanity Checking in calls to malloc()/calloc()
Status: NEW
Product: Samba 4.0
Classification: Unclassified
Component: Other
4.0.7
All All
: P5 major
: ---
Assigned To: Andrew Bartlett
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-14 20:21 UTC by Bill Parker
Modified: 2013-07-16 05:48 UTC (History)
0 users

See Also:


Attachments
This is a zip file created with 'zip -u' for all of the patches (in diff -u format) described in the bug report (9.49 KB, application/octet-stream)
2013-07-14 20:21 UTC, Bill Parker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Parker 2013-07-14 20:21:04 UTC
Created attachment 9046 [details]
This is a zip file created with 'zip -u' for all of the patches (in diff -u format) described in the bug report

Hello All,

	In reviewing various files in Samba-4.0.7, I found a number
of instances where malloc()/calloc() were called without the
checking the return value for a value of NULL, which would
indicate failure.

In directory 'samba-4.0.7/lib/ntdb/test', the following patch
files make the required checks, though in some cases, a(n) error
message to stderr might be appropriate:

In file 'api-55-transaction.c', the following test was added:

--- api-55-transaction.c.orig   2013-07-13 15:13:43.996141096 -0700
+++ api-55-transaction.c        2013-07-13 15:16:09.286141402 -0700
@@ -18,6 +18,12 @@
        NTDB_DATA data;
 
        buffer = malloc(1000);
+       if (buffer == NULL {
+           /* ummmm, we need to print some kind of error message here */
+           /* to let the user know we couldn't allocate the requested memory */
+           return -1;
+       }
+
        for (i = 0; i < 1000; i++)
                buffer[i] = i;
 
In file 'api-91-get-stats.c', the following tests were added:

--- api-91-get-stats.c.orig     2013-07-13 15:17:33.911143532 -0700
+++ api-91-get-stats.c  2013-07-13 15:20:47.592144498 -0700
@@ -27,11 +27,21 @@
                /* Force an expansion */
                data.dsize = 65536;
                data.dptr = calloc(data.dsize, 1);
+               if (data.dptr == NULL) {
+                   /* ummmm, we need to print some kind of error message here */
+                   /* to let the user know we couldn't allocate the requested memory */
+                   return -1;
+               }
                ok1(ntdb_store(ntdb, key, data, NTDB_REPLACE) == 0);
                free(data.dptr);
 
                /* Use malloc so valgrind will catch overruns. */
                attr = malloc(sizeof *attr);
+               if (attr == NULL) {
+                   /* ummmm, we need to print some kind of error message here */
+                   /* to let the user know we couldn't allocate the requested memory */
+                   return -1;
+               }
                attr->stats.base.attr = NTDB_ATTRIBUTE_STATS;
                attr->stats.size = sizeof(*attr);
 
@@ -45,6 +55,11 @@
                /* Try short one. */
                attr = malloc(offsetof(struct ntdb_attribute_stats, allocs)
                              + sizeof(attr->stats.allocs));
+               if (attr == NULL) {
+                   /* ummmm, we need to print some kind of error message here */
+                   /* to let the user know we couldn't allocate the requested memory */
+                   return -1;
+               }
                attr->stats.base.attr = NTDB_ATTRIBUTE_STATS;
                attr->stats.size = offsetof(struct ntdb_attribute_stats, allocs)
                        + sizeof(attr->stats.allocs);
						
In file 'api-firstkey-nextkey.c', the following test was added:

--- api-firstkey-nextkey.c.orig 2013-07-13 16:13:19.882140189 -0700
+++ api-firstkey-nextkey.c      2013-07-13 16:15:34.540136971 -0700
@@ -40,6 +40,11 @@
 static NTDB_DATA dup_key(NTDB_DATA key)
 {
        void *p = malloc(key.dsize);
+       if (p == NULL) {
+           /*  ummmm, do we need an error message here if  */
+           /*  we are NOT unable to allocate memory...     */
+           return NULL;
+       }
        memcpy(p, key.dptr, key.dsize);
        key.dptr = p;
        return key;
		
In file 'api-record-expand.c', the following test was added:

--- api-record-expand.c.orig    2013-07-13 16:18:33.995141901 -0700
+++ api-record-expand.c 2013-07-13 16:17:56.547141774 -0700
@@ -21,6 +21,11 @@
        NTDB_DATA data;
 
        data.dptr = malloc(MAX_SIZE);
+       if (data.dptr == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return -1;
+       }
        memset(data.dptr, 0x24, MAX_SIZE);
 
        plan_tests(sizeof(flags) / sizeof(flags[0])
		
In file 'external-agent.c', the following tests were added:

--- external-agent.c.orig       2013-07-13 16:25:05.553139727 -0700
+++ external-agent.c    2013-07-13 16:27:51.009139530 -0700
@@ -169,7 +169,11 @@
 
        if (pid != 0) {
                struct agent *agent = malloc(sizeof(*agent));
-
+               if (agent == NULL) {
+                   /* ummmm, do we need to print an error message here if  */
+                   /* we're unable to allocate memory? */
+                   return ENOMEM;
+               }
                close(command[0]);
                close(response[1]);
                agent->cmdfd = command[1];
@@ -214,7 +218,11 @@
                name = "";
        len = 1 + strlen(name) + 1;
        string = malloc(len);
-
+       if (string == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return ENOMEM;
+       }
        string[0] = op;
        strcpy(string+1, name);
 
In file 'helprun-layout.c', the following tests were added:

--- helprun-layout.c.orig       2013-07-13 16:34:51.160141055 -0700
+++ helprun-layout.c    2013-07-13 16:39:08.592141663 -0700
@@ -9,6 +9,11 @@
 struct ntdb_layout *new_ntdb_layout(void)
 {
        struct ntdb_layout *layout = malloc(sizeof(*layout));
+       if (layout == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return NULL;
+       }
        layout->num_elems = 0;
        layout->elem = NULL;
        return layout;
@@ -64,6 +69,11 @@
        NTDB_DATA ret;
        ret.dsize = key.dsize;
        ret.dptr = malloc(ret.dsize);
+       if (ret.dptr == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return NULL;
+       }
        memcpy(ret.dptr, key.dptr, ret.dsize);
        return ret;
 }
@@ -275,6 +285,11 @@
        }
 
        mem = malloc(off);
+       if (mem == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return NULL;
+       }
        /* Fill with some weird pattern. */
        memset(mem, 0x99, off);
        memcpy(mem, ntdb->file->map_ptr, hdrlen);
		
In file 'lock-tracking.c', the following test was added:

--- lock-tracking.c.orig        2013-07-13 16:31:39.421139729 -0700
+++ lock-tracking.c     2013-07-13 16:33:28.713146022 -0700
@@ -121,6 +121,11 @@
 
                if (ret == 0) {
                        new = malloc(sizeof *new);
+                       if (new == NULL) {
+                           /* ummmm, do we need to print an error message here if  */
+                           /* we're unable to allocate memory? */
+                           return ENOMEM;
+                       }
                        new->off = fl->l_start;
                        new->len = fl->l_len;
                        new->type = fl->l_type;
						
In file 'run-15-append.c', the following test was added:

--- run-15-append.c.orig        2013-07-13 16:40:38.872138696 -0700
+++ run-15-append.c     2013-07-13 16:41:35.180141052 -0700
@@ -32,6 +32,11 @@
        NTDB_DATA data;
 
        buffer = malloc(MAX_SIZE);
+       if (buffer == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return -1;
+       }
        for (i = 0; i < MAX_SIZE; i++)
                buffer[i] = i;
 
In file 'run-30-exhaust-before-expand.c', the following test was added:

--- run-15-append.c.orig        2013-07-13 16:40:38.872138696 -0700
+++ run-15-append.c     2013-07-13 16:41:35.180141052 -0700
@@ -32,6 +32,11 @@
        NTDB_DATA data;
 
        buffer = malloc(MAX_SIZE);
+       if (buffer == NULL) {
+           /* ummmm, do we need to print an error message here if  */
+           /* we're unable to allocate memory? */
+           return -1;
+       }
        for (i = 0; i < MAX_SIZE; i++)
                buffer[i] = i;
 
In file 'run-56-open-during-transaction.c', the following tests were added:

--- run-56-open-during-transaction.c.orig       2013-07-13 16:46:17.845132961 -0700
+++ run-56-open-during-transaction.c    2013-07-13 16:45:19.191140729 -0700
@@ -47,6 +47,10 @@
 
        /* over-length read serves as length check. */
        contents = malloc(snapshot_len+1);
+       if (contents == NULL) {
+           diag("Unable to allocate memory in compare_file()...\n");
+           return false;
+       }
        ret = pread(fd, contents, snapshot_len+1, 0) == snapshot_len
                && is_same(snapshot, contents, snapshot_len);
        free(contents);
@@ -61,6 +65,11 @@
 
        fstat(fd, &st);
        contents = malloc(st.st_size);
+       if (contents == NULL) {
+           diag("Unable to allocate memory in check_file_intact()...\n");
+           errors++;
+           return;
+       }
        if (pread(fd, contents, st.st_size, 0) != st.st_size) {
                diag("Read fail");
                errors++;
				
In file 'run-57-die-during-transaction.c', the following test was added:

--- run-57-die-during-transaction.c.orig        2013-07-13 16:47:29.414143359 -0700
+++ run-57-die-during-transaction.c     2013-07-13 16:48:43.953138749 -0700
@@ -26,6 +26,10 @@
        for (i = 0; i < MAX_ALLOCATIONS; i++)
                if (!allocated[i]) {
                        allocated[i] = malloc(len);
+                       if (allocated[i] == NULL) {
+                               diag("Unable to allocate memory...");
+                               abort();
+                       }
                        if (i > max_alloc) {
                                max_alloc = i;
                                diag("max_alloc: %i", max_alloc);
								
In file 'run-64-bit-tdb.c', the following test was added:

--- run-64-bit-tdb.c.orig       2013-07-13 16:50:02.769134548 -0700
+++ run-64-bit-tdb.c    2013-07-13 16:51:22.413137949 -0700
@@ -42,6 +42,10 @@
                d.dsize = ntdb->file->map_size
                        - NEW_DATABASE_HDR_SIZE(ntdb->hash_bits) - 8;
                d.dptr = malloc(d.dsize);
+               if (d.dptr == NULL) {
+                   pass("Unable to allocate memory...");
+                   return -1;
+               }
                memset(d.dptr, 0, d.dsize);
                ok1(ntdb_store(ntdb, k, d, NTDB_INSERT) == 0);
                ok1(ntdb->file->map_size == old_size);
				
In file 'run-expand-in-transaction.c', the following test was added:

--- run-expand-in-transaction.c.orig    2013-07-13 16:53:44.322140431 -0700
+++ run-expand-in-transaction.c 2013-07-13 16:53:04.875138910 -0700
@@ -29,6 +29,11 @@
                d.dsize = ntdb->file->map_size
                        - NEW_DATABASE_HDR_SIZE(ntdb->hash_bits) - 8;
                d.dptr = malloc(d.dsize);
+               if (d.dptr == NULL) {
+                   /* ummmm, do we need to print an error message here if  */
+                   /* we're unable to allocate memory? */
+                   return -1;
+               }
                memset(d.dptr, 0, d.dsize);
                ok1(ntdb_store(ntdb, k, d, NTDB_INSERT) == 0);
                ok1(ntdb->file->map_size == size);
				
In directory 'samba-4.0.7/lib/ccan/hash/test', file 'run.c', a call
to malloc(256000) is made w/out the proper check for NULL, indicating
failure, the patch file below adds a diagnostic messages and exits:

--- run.c.orig  2013-07-14 08:21:32.175104648 -0700
+++ run.c       2013-07-14 08:23:58.554101605 -0700
@@ -81,6 +81,10 @@
        for (i = 0; i < sizeof(uint32_t); i++) {
                unsigned int lowest = -1U, highest = 0;
                char *p = malloc(256000);
+               if (p == NULL) {
+                   diag("Unable to allocate memory...exiting...");
+                   return -1;
+               }
 
                memset(results, 0, sizeof(results));
 
				
In directory 'samba-4.0.7/source4/heimdal/lib/krb5', file 'crypto.c',
I found two instances where malloc was called without a check for a
return value of NULL, indicating failure.  The patch file is below:

--- crypto.c.orig       2013-07-14 10:12:50.973099390 -0700
+++ crypto.c    2013-07-14 10:14:29.273094563 -0700
@@ -1572,6 +1572,8 @@
     }
 
     p = q = malloc(len);
+    if (p == NULL)
+       return ENOMEM;
 
     for (i = 0; i < num_data; i++) {
        if (data[i].flags != KRB5_CRYPTO_TYPE_DATA &&
@@ -1651,6 +1653,8 @@
     }
 
     p = q = malloc(len);
+    if (p == NULL)
+       return ENOMEM;
 
     for (i = 0; i < num_data; i++) {
        if (data[i].flags != KRB5_CRYPTO_TYPE_DATA &&
		

In directory 'samba-4.0.7/source4/heimdal/lib/krb5', file 'pkinit.c',
I found two instances where malloc was called without a check for a
return value of NULL, indicating failure.  The patch file is below:

--- pkinit.c.orig       2013-07-14 10:27:59.799100232 -0700
+++ pkinit.c    2013-07-14 10:31:35.895098609 -0700
@@ -1206,6 +1206,12 @@
            unsigned char *ptr = malloc(content.length + ph);
            size_t l;
 
+           if (*ptr == NULL) {
+               ret = ENOMEM;
+               krb5_set_error_message(context, ret, N_("malloc: out of memory", ""));
+               return ret;
+           }
+
            memcpy(ptr + ph, content.data, content.length);
 
            ret = der_put_length_and_tag (ptr + ph - 1, ph, content.length,

In directory 'samba-4.0.7/source4/heimdal/lib/gssapi/mech', file
'pkinit.c', I found two instances where malloc was called without
a check for a return value of NULL, indicating failure.  The patch
file is below:
			
--- context.c.orig      2013-07-14 11:05:39.646098455 -0700
+++ context.c   2013-07-14 11:08:09.056097651 -0700
@@ -86,6 +86,8 @@
        if (value != mg->maj_stat || mg->maj_error.length == 0)
            break;
        string->value = malloc(mg->maj_error.length + 1);
+       if (string->value == NULL)
+           return GSS_S_BAD_STATUS;
        string->length = mg->maj_error.length;
        memcpy(string->value, mg->maj_error.value, mg->maj_error.length);
         ((char *) string->value)[string->length] = '\0';
@@ -95,6 +97,8 @@
        if (value != mg->min_stat || mg->min_error.length == 0)
            break;
        string->value = malloc(mg->min_error.length + 1);
+       if (string->value == NULL)
+           return GSS_S_BAD_STATUS;
        string->length = mg->min_error.length;
        memcpy(string->value, mg->min_error.value, mg->min_error.length);
         ((char *) string->value)[string->length] = '\0';
		 
In directory 'samba-4.0.7/source4/heimdal/lib/hcrypto', file 'dh-ltm.c'
I found a call to malloc() without a check for NULL, indicating failure.

The patch below corrects this issue:

--- dh-ltm.c.orig       2013-07-14 11:24:40.088098679 -0700
+++ dh-ltm.c    2013-07-14 11:25:38.188098229 -0700
@@ -51,6 +51,8 @@
 
     len = BN_num_bytes(bn);
     p = malloc(len);
+    if (p == NULL)
+       return;
     BN_bn2bin(bn, p);
     mp_read_unsigned_bin(s, p, len);
     free(p);
	 

In directory 'samba-4.0.7/source4/heimdal/lib/hcrypto', file 'hmac.c'
I found three calls to malloc() without checks for NULL, indicating
failure.

The patch file below corrects these issues:

--- hmac.c.orig 2013-07-14 11:30:35.207099754 -0700
+++ hmac.c      2013-07-14 11:32:47.594095062 -0700
@@ -92,6 +92,9 @@
        ctx->key_length = EVP_MD_size(ctx->md);
        ctx->buf = malloc(ctx->key_length);
     }
+    if (ctx->buf == NULL)
+       return; /* call to malloc() failed  */
+
 #if 0
     ctx->engine = engine;
 #endif
@@ -112,7 +115,11 @@
     }
 
     ctx->opad = malloc(EVP_MD_block_size(ctx->md));
+    if (ctx->opad == NULL)
+       return; /* call to malloc() failed */
     ctx->ipad = malloc(EVP_MD_block_size(ctx->md));
+    if (ctx->ipad == NULL)
+       return; /* call to malloc() failed */
     memset(ctx->ipad, 0x36, EVP_MD_block_size(ctx->md));
     memset(ctx->opad, 0x5c, EVP_MD_block_size(ctx->md));
 

In directory 'samba-4.0.7/source4/heimdal/lib/hcrypto', file 'hmac.c'
I found two calls to malloc() without checks for NULL, indicating
failure.

The patch file below corrects these issues:
 
--- rsa-ltm.c.orig      2013-07-14 11:36:49.398106373 -0700
+++ rsa-ltm.c   2013-07-14 11:39:43.688094413 -0700
@@ -70,6 +70,8 @@
 
     len = BN_num_bytes(bn);
     p = malloc(len);
+    if (p == NULL)
+       return;
     BN_bn2bin(bn, p);
     mp_read_unsigned_bin(s, p, len);
     free(p);
@@ -306,6 +308,9 @@
        return -2;
 
     p0 = p = malloc(size);
+    if (p == NULL)
+       return -3;
+
     *p++ = 0;
     *p++ = 1;
     memset(p, 0xff, size - flen - 3);
	 
I am attaching the patch files to this bug report for review.

unzip -l samba-patches.zip
 
  Length      Date    Time    Name
---------  ---------- -----   ----
      429  07-13-2013 16:56   api-55-transaction.c.patch
     1359  07-13-2013 16:56   api-91-get-stats.c.patch
      443  07-13-2013 16:57   api-firstkey-nextkey.c.patch
      439  07-13-2013 16:20   api-record-expand.c.patch
      745  07-13-2013 16:28   external-agent.c.patch
     1080  07-13-2013 16:39   helprun-layout.c.patch
      438  07-13-2013 16:33   lock-tracking.c.patch
      393  07-13-2013 16:41   run-15-append.c.patch
      571  07-13-2013 16:24   run-30-exhaust-before-expand.c.patch
      796  07-13-2013 16:46   run-56-open-during-transaction.c.patch
      443  07-13-2013 16:55   run-57-die-during-transaction.c.patch
      467  07-13-2013 16:51   run-64-bit-tdb.c.patch
      552  07-13-2013 16:54   run-expand-in-transaction.c.patch
      373  07-14-2013 08:26   run.c.patch
      467  07-14-2013 10:16   crypto.c.patch
      468  07-14-2013 10:31   pkinit.c.patch
      841  07-14-2013 11:11   context.c.patch
      279  07-14-2013 11:27   dh-ltm.c.patch
      706  07-14-2013 11:34   hmac.c.patch
      448  07-14-2013 11:40   rsa-ltm.c.patch
---------                     -------
    11737                     20 files
	
Bill Parker (wp02855 at gmail dot com)
Comment 1 Volker Lendecke 2013-07-15 09:02:42 UTC
This all looks good. Would it be possible that you upload this as a

git format-patch

style attachment? See

https://wiki.samba.org/index.php/Using_Git_for_Samba_Development

for a *very* brief intro.

Volker
Comment 2 Bill Parker 2013-07-15 16:38:04 UTC
Unfortunately, I don't have GIT available, can you guys handle just plain
'diff -u', the zip files in the tarball are all diff -u for patches...

I also have 15 additional patches which correct the same issues in other
parts of the code :)

Bill


On Mon, Jul 15, 2013 at 2:02 AM, <samba-bugs@samba.org> wrote:

> https://bugzilla.samba.org/show_bug.cgi?id=10021
>
> --- Comment #1 from Volker Lendecke <vl@samba.org> 2013-07-15 09:02:42
> UTC ---
> This all looks good. Would it be possible that you upload this as a
>
> git format-patch
>
> style attachment? See
>
> https://wiki.samba.org/index.php/Using_Git_for_Samba_Development
>
> for a *very* brief intro.
>
> Volker
>
> --
> Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>
Comment 3 Volker Lendecke 2013-07-16 05:48:21 UTC
(In reply to comment #2)
> Unfortunately, I don't have GIT available, can you guys handle just plain
> 'diff -u', the zip files in the tarball are all diff -u for patches...

Sure we can. It's just the difference between a simple "git am" and some additional steps. And as I guess you are using some kind of version control as well, I thought it might be git and I could ask. But if git is not possible for you, then sure, we can do with a verbal description as well.

Thanks,

Volker