Bug 10025 - Lack of Sanity Checking in calls to malloc()/calloc()
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-16 16:29 UTC by Bill Parker
Modified: 2013-07-23 14:34 UTC (History)
1 user (show)

See Also:


Attachments
diff -u patch files all rolled into single zip file (8.60 KB, application/octet-stream)
2013-07-16 16:29 UTC, Bill Parker
no flags Details
git-am version of the fixes. (16.29 KB, patch)
2013-07-17 17:18 UTC, Jeremy Allison
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-16 16:29:00 UTC
Created attachment 9053 [details]
diff -u patch files all rolled into single zip file

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/nsswitch', file 'nsstest.c', the
existing code calls malloc() without checking the return
value from malloc(). The following patch file(s) make the
required checks for the return value from calls to malloc(),
though in some cases, a(n) error message to stderr might be
appropriate:

Here is the patch file:

--- nsstest.c.orig      2013-07-15 13:20:43.815038138 -0700
+++ nsstest.c   2013-07-15 13:22:01.395040113 -0700
@@ -371,6 +371,11 @@
        NSS_STATUS status;
 
        groups = (gid_t *)malloc(sizeof(gid_t) * size);
+       if (groups == NULL) {
+           printf("Unable to allocate memory for groups\n");
+           return;
+       }
+
        groups[0] = gid;
 
        status = nss_initgroups(name, gid, &groups, &start, &size);		

In directory 'samba-4.0.7/lib/iniparser/src', file 'dictionary.c',
the existing code calls calloc() without checking the return
value from calloc() for NULL, indicating failure. The following patch
file(s) make the required checks for the return value from calls to
calloc(), though in some cases, a(n) error message to stderr might be
appropriate:

Here is the patch file:

--- dictionary.c.orig   2013-07-15 13:32:46.662039628 -0700
+++ dictionary.c        2013-07-15 13:35:16.127039091 -0700
@@ -53,6 +53,8 @@
     void    *   newptr ;
  
     newptr = calloc(2*size, 1);
+    if (newptr == NULL)
+       return NULL;
     memcpy(newptr, ptr, size);
     free(ptr);
     return newptr ;
@@ -117,10 +119,23 @@
        if (!(d = (dictionary *)calloc(1, sizeof(dictionary)))) {
                return NULL;
        }
-       d->size = size ;
+
+       d->size = size;
        d->val  = (char **)calloc(size, sizeof(char*));
+       if (d-val == NULL) {
+               return NULL;
+       }
+
        d->key  = (char **)calloc(size, sizeof(char*));
+       if (d->key == NULL) {
+               return NULL;
+       }
+
        d->hash = (unsigned int *)calloc(size, sizeof(unsigned));
+       if (d->hash == NULL) {
+               return NULL;
+       }
+
        return d ;
 }

In directory 'samba-4.0.7/lib/ntdb/tools', file 'growtdb-bench.c',
the existing code calls calloc() without checking the return
value from calloc() for NULL, indicating failure. The following patch
file(s) make the required checks for the return value from calls to
calloc(), though in some cases, a(n) error message to stderr might be
appropriate:

Here is the patch file:

--- growtdb-bench.c.orig        2013-07-15 13:39:17.634039264 -0700
+++ growtdb-bench.c     2013-07-15 13:43:31.954038804 -0700
@@ -48,12 +48,24 @@
        idxkey.dsize = strlen("User index");
        idxdata.dsize = 51;
        idxdata.dptr = calloc(idxdata.dsize, 1);
+       if (idxdata.dptr == NULL) {
+               fprintf(stderr, "Unable to allocate memory for idxdata.dptr\n");
+               return -1;
+       }
 
        /* Create users. */
        k.dsize = 48;
        k.dptr = calloc(k.dsize, 1);
+       if (k.dptr == NULL) {
+               fprintf(stderr, "Unable to allocate memory for k.dptr\n");
+               return -1;
+       }
        d.dsize = 64;
        d.dptr = calloc(d.dsize, 1);
+       if (d.dptr == NULL) {
+               fprintf(stderr, "Unable to allocate memory for d.dptr\n");
+               return -1;
+       }
 
        ntdb_transaction_start(ntdb);
        for (i = 0; i < users; i++) {
@@ -79,6 +91,10 @@
         * a group. */
        gk.dsize = 48;
        gk.dptr = calloc(k.dsize, 1);
+       if (gk.dptr == NULL) {
+               fprintf(stderr, "Unable to allocate memory for gk.dptr\n");
+               return -1;
+       }
        gk.dptr[gk.dsize-1] = 1;
 
        d.dsize = 32;

In directory 'samba-4.0.7/lib/ntdb/tools', file 'ntdbtorture.c',
the existing code calls malloc() without checking the return
value from malloc() for NULL, indicating failure. The following patch
file(s) make the required checks for the return value from calls to
malloc(), though in some cases, a(n) error message to stderr might be
appropriate:

Here is the patch file:
		
--- ntdbtorture.c.orig  2013-07-13 15:09:18.835144785 -0700
+++ ntdbtorture.c       2013-07-13 15:10:54.550134834 -0700
@@ -96,6 +96,10 @@
        char *buf;
        int i;
        buf = (char *)malloc(len+1);
+       if (buf == NULL) {
+           perror("randbuf: unable to allocate memory for buffer...\n");
+           exit(1);
+       }
 
        for (i=0;i<len;i++) {
                buf[i] = 'a' + (rand() % 26);

In directory 'samba-4.0.7/lib/popt', file 'popt.c', the existing
code calls calloc() without checking the return value from calloc()
for NULL, indicating failure. The following patch file(s) makes the
required checks for the return value from calls to calloc(), though
in some cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- popt.c.orig 2013-07-15 14:00:49.778037510 -0700
+++ popt.c      2013-07-15 14:02:36.653037460 -0700
@@ -171,6 +171,7 @@
 
     con->leftovers = (const char **)calloc( (argc + 1),
                                            sizeof(*con->leftovers) );
+    if (con->leftovers == NULL) return NULL;   /* XXX should never happen? */
     /*@-dependenttrans -assignexpose@*/        /* FIX: W2DO? */
     con->options = options;
     /*@=dependenttrans =assignexpose@*/
@@ -182,6 +183,7 @@
     con->finalArgvAlloced = argc * 2;
     con->finalArgv = (const char **)calloc( con->finalArgvAlloced,
                                            sizeof(*con->finalArgv) );
+    if (con->finalArgv == NULL) return NULL;   /* XXX should never happen? */
     con->execAbsolute = 1;
     con->arg_strip = NULL;

In directory 'samba-4.0.7/lib/ccan/htable/tools', file 'speed.c',
the existing code calls calloc() without checking the return value
from calloc() for NULL, indicating failure. The following patch
file(s) makes the required checks for the return value from calls
to calloc(), though in some cases, a(n) error message to stderr
might be appropriate:

Here is the patch file:

--- speed.c.orig        2013-07-15 14:06:11.849033917 -0700
+++ speed.c     2013-07-15 14:07:11.006037728 -0700
@@ -145,6 +145,11 @@
        }
        num = argv[1] ? atoi(argv[1]) : 1000000;
        objs = calloc(num, sizeof(objs[0]));
+       if (objs == NULL) {
+               printf("Unable to allocate memory for objs...exiting...\n");
+               fflush(stdout);
+               return -1;
+       }
 
        for (i = 0; i < num; i++) {
                objs[i].key = i;

In directory 'samba-4.0.7/lib/ccan/list/test', file 'helper.c',
the existing code calls calloc() without checking the return value
from calloc() for NULL, indicating failure. The following patch
file(s) makes the required checks for the return value from calls
to calloc(), though in some cases, a(n) error message to stderr
might be appropriate:

Here is the patch file:

--- helper.c.orig       2013-07-15 14:11:24.627039485 -0700
+++ helper.c    2013-07-15 14:12:18.430038327 -0700
@@ -19,6 +19,9 @@
 struct opaque *create_opaque_blob(void)
 {
   struct opaque *blob = calloc(1, sizeof(struct opaque));
+  if (*blob == NULL) {
+    return NULL;
+  }
 
   if (not_randomized) {
     srandom((int)time(NULL));

In directory 'samba-4.0.7/lib/ccan/read_write_all/test', file
'run-read_all.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- run-read_all.c.orig 2013-07-15 14:16:22.954038018 -0700
+++ run-read_all.c      2013-07-15 14:17:28.159037958 -0700
@@ -32,6 +32,10 @@
        pid_t child;
 
        buffer = calloc(BUFSZ, 2);
+       if (buffer == NULL) {
+               printf("Unable to allocate memory for buffer...\n");
+               exit(-1);
+       }
        plan_tests(6);
 
        /* We fork and torture parent. */

In directory 'samba-4.0.7/lib/tdb/tools', file 'tdbtorture.c',
the existing code calls calloc() without checking the return
value from calloc() for NULL, indicating failure. The following
patch file(s) makes the required checks for the return value
from calls to calloc(), though in some cases, a(n) error message
to stderr might be appropriate:

Here is the patch file:

--- tdbtorture.c.orig   2013-07-15 14:21:03.051037499 -0700
+++ tdbtorture.c        2013-07-15 14:22:19.847036948 -0700
@@ -342,7 +342,15 @@
        }
 
        pids = (pid_t *)calloc(sizeof(pid_t), num_procs);
+       if (pids == NULL) {
+               perror("Unable to allocate memory for pids");
+               exit(1);
+       }
        done = (int *)calloc(sizeof(int), num_procs);
+       if (done == NULL) {
+               perror("Unable to allocate memory for done");
+               exit(1);
+       }
 
        if (pipe(pfds) != 0) {
                perror("Creating pipe");

In directory 'samba-4.0.7/lib/tdb/test', file 'run-transaction-expand.c',
the existing code calls calloc() without checking the return
value from calloc() for NULL, indicating failure. The following
patch file(s) makes the required checks for the return value
from calls to calloc(), though in some cases, a(n) error message
to stderr might be appropriate:

Here is the patch file:

--- run-transaction-expand.c.orig       2013-07-15 14:26:34.657034965 -0700
+++ run-transaction-expand.c    2013-07-15 14:28:02.422037760 -0700
@@ -73,6 +73,11 @@
 
        data.dsize = 0;
        data.dptr = calloc(1000, getpagesize());
+       if (data.dptr == NULL) {
+               diag("Unable to allocate memory for data.dptr");
+               tdb_close(tdb);
+               return -1;
+       }
 
        /* Simulate a slowly growing record. */
        for (i = 0; i < 1000; i++)

In directory 'samba-4.0.7/lib/replace', file 'getifaddrs.c',
the existing code calls calloc() without checking the return
value from calloc() for NULL, indicating failure. The following
patch file(s) makes the required checks for the return value
from calls to calloc(), though in some cases, a(n) error message
to stderr might be appropriate:

Here is the patch file:

--- getifaddrs.c.orig   2013-07-15 14:33:32.400037343 -0700
+++ getifaddrs.c        2013-07-15 14:35:21.475038407 -0700
@@ -117,6 +117,10 @@
                }
 
                curif = calloc(1, sizeof(struct ifaddrs));
+               if (curif == NULL)
+                       freeifaddrs(*ifap);
+                       return -;1
+               }
                curif->ifa_name = strdup(ifr[i].ifr_name);
                curif->ifa_flags = ifr[i].ifr_flags;
                curif->ifa_dstaddr = NULL;

In directory 'samba-4.0.7/source4/heimdal/lib/krb5', file
'config_file.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- config_file.c.orig  2013-07-15 14:46:47.069038367 -0700
+++ config_file.c       2013-07-15 14:49:20.982035887 -0700
@@ -581,6 +581,11 @@
 
     while (c) {
        d = calloc(1, sizeof(*d));
+       if (d == NULL) {
+           krb5_abortx(context,
+                       "Unable to allocate memory (%d) in krb5_config_copy",
+                       -1);
+       }
 
        if (*head == NULL)
            *head = d;

In directory 'samba-4.0.7/source4/heimdal/lib/krb5', file
'init_creds_pw.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- init_creds_pw.c.orig        2013-07-15 14:56:08.037039815 -0700
+++ init_creds_pw.c     2013-07-15 14:57:52.385029901 -0700
@@ -1203,6 +1203,10 @@
        unsigned flag;
 
        paid = calloc(1, sizeof(*paid));
+       if (paid == NULL) {
+           krb5_set_error_message(context, ENOMEM, N_("malloc: out of memory", ""));
+           return ENOMEM;
+       }
 
        paid->etype = KRB5_ENCTYPE_NULL;
        ppaid = process_pa_info(context, creds->client, a, paid, in_md);

In directory 'samba-4.0.7/source4/heimdal/lib/hdb', file
'dbinfo.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- dbinfo.c.orig       2013-07-15 15:08:09.571036518 -0700
+++ dbinfo.c    2013-07-15 15:09:30.647036376 -0700
@@ -139,6 +139,10 @@
     if(databases == NULL) {
        /* if there are none specified, create one and use defaults */
        di = calloc(1, sizeof(*di));
+       if (di == NULL) {
+           krb5_set_error_message(context, ENOMEM, "malloc: out of memory");
+           return ENOMEM;
+       }
        databases = di;
        di->label = strdup("default");
     }

In directory 'samba-4.0.7/source4/heimdal/lib/roken', file
'resolve.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- resolve.c.orig      2013-07-14 15:08:18.476089589 -0700
+++ resolve.c   2013-07-15 15:14:40.807037679 -0700
@@ -723,6 +723,8 @@
        return NULL;
 
     rr = calloc(1, sizeof(*rr));
+    if (rr == NULL)
+       return NULL;
 
     rr->domain = strdup(pRec->pName);
     rr->type = pRec->wType;
@@ -787,6 +789,11 @@
        len = strnlen(pRec->Data.TXT.pStringArray[0], DNS_MAX_TEXT_STRING_LENGTH);
 
        rr->u.txt = (char *)malloc(len + 1);
+       if (rr->u.txt == NULL) {
+           dns_free_rr(rr);
+           return NULL;
+       }
+
        strcpy_s(rr->u.txt, len + 1, pRec->Data.TXT.pStringArray[0]);
 
        break;
@@ -900,6 +907,8 @@
            return NULL;
 
        r = calloc(1, sizeof(*r));
+       if (r == NULL)
+           return NULL;
        r->q.domain = strdup(domain);
        r->q.type = type;
        r->q.class = 0;

In directory 'samba-4.0.7/source4/heimdal/lib/hx509', file
'ks_mem.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- ks_mem.c.orig       2013-07-15 15:26:38.743036017 -0700
+++ ks_mem.c    2013-07-15 15:27:45.459035354 -0700
@@ -163,6 +163,11 @@
     for (i = 0; mem->keys && mem->keys[i]; i++)
        ;
     *keys = calloc(i + 1, sizeof(**keys));
+    if (*keys == NULL) {
+       hx509_set_error_string(context, 0, ENOMEM, "out of memory");
+       return ENOMEM;
+    }
+
     for (i = 0; mem->keys && mem->keys[i]; i++) {
        (*keys)[i] = _hx509_private_key_ref(mem->keys[i]);
        if ((*keys)[i] == NULL) {
		
In directory 'samba-4.0.7/source4/heimdal/lib/hcrypto', file
'dsa.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- dsa.c.orig  2013-07-15 15:33:21.773035947 -0700
+++ dsa.c       2013-07-15 15:33:58.434035054 -0700
@@ -47,6 +47,8 @@
 DSA_new(void)
 {
     DSA *dsa = calloc(1, sizeof(*dsa));
+    if (*dsa == NULL)
+       return NULL;
     dsa->meth = rk_UNCONST(DSA_get_default_method());
     dsa->references = 1;
     return dsa;

In directory 'samba-4.0.7/source4/heimdal/lib/hcrypto', file
'engine.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- engine.c.orig       2013-07-15 15:35:56.081030960 -0700
+++ engine.c    2013-07-15 15:36:27.662038005 -0700
@@ -62,6 +62,9 @@
     ENGINE *engine;
 
     engine = calloc(1, sizeof(*engine));
+    if (engine == NULL)
+       return NULL;
+
     engine->references = 1;
 
     return engine;

In directory 'samba-4.0.7/source4/librpc/rpc', file
'pyrpc_util.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- pyrpc_util.c.orig   2013-07-15 15:41:38.872035763 -0700
+++ pyrpc_util.c        2013-07-15 15:42:32.990032577 -0700
@@ -245,6 +245,8 @@
        for (i = 0; mds[i].name; i++) {
                PyObject *ret;
                struct wrapperbase *wb = (struct wrapperbase *)calloc(sizeof(struct wrapperbase), 1);
+               if (*wb == NULL)
+                       return false;
 
                wb->name = discard_const_p(char, mds[i].name);
                wb->flags = PyWrapperFlag_KEYWORDS;

In directory 'samba-4.0.7/source4/torture', file
'gentest.c', the existing code calls calloc() without
checking the return value from calloc() for NULL, indicating
failure. The following patch file(s) makes the required checks
for the return value from calls to calloc(), though in some
cases, a(n) error message to stderr might be appropriate:

Here is the patch file:

--- gentest.c.orig      2013-07-15 15:44:46.196034030 -0700
+++ gentest.c   2013-07-15 15:46:32.079035052 -0700
@@ -3068,9 +3068,17 @@
 
        /* allocate the open_handles array */
        open_handles = calloc(options.max_open_handles, sizeof(open_handles[0]));
+       if (open_handles == NULL) {
+               printf("Unable to allocate memory for open_handles array...\n");
+               exit(1);
+       }
 
        srandom(options.seed);
        op_parms = calloc(options.numops, sizeof(op_parms[0]));
+       if (op_parms == NULL) {
+               printf("Unable to allocate memory for op_parms...\n");
+               exit(1);
+       }
 
        /* generate the seeds - after this everything is deterministic */
        if (options.use_preset_seeds) {
		
I am attaching the patch file(s) to this email.

This bug report should complete all checks for malloc()/calloc()/realloc()
calls with regards to sanity checks on the return value from these calls.

Archive:  even-more-samba-patches.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
      382  07-15-2013 13:23   nsstest.c.patch
      757  07-15-2013 13:37   dictionary.c.patch
      985  07-15-2013 13:44   growtdb-bench.c.patch
      352  07-13-2013 15:11   ntdbtorture.c.patch
      719  07-15-2013 14:04   popt.c.patch
      383  07-15-2013 14:07   speed.c.patch
      329  07-15-2013 14:13   helper.c.patch
      332  07-15-2013 14:19   run-read_all.c.patch
      467  07-15-2013 15:53   tdbtorture.c.patch
      406  07-15-2013 14:29   run-transaction-expand.c.patch
      367  07-15-2013 14:35   getifaddrs.c.patch
      339  07-15-2013 14:51   config_file.c.patch
      426  07-15-2013 15:55   init_creds_pw.c.patch
      424  07-15-2013 15:10   dbinfo.c.patch
      747  07-15-2013 15:16   resolve.c.patch
      468  07-15-2013 15:29   ks_mem.c.patch
      310  07-15-2013 15:35   dsa.c.patch
      279  07-15-2013 15:37   engine.c.patch
      401  07-15-2013 15:43   pyrpc_util.c.patch
      649  07-15-2013 15:47   gentest.c.patch
---------                     -------
     9522                     20 files

Bill Parker (wp02855 at gmail dot com)
Comment 1 Jeremy Allison 2013-07-17 17:18:57 UTC
Created attachment 9057 [details]
git-am version of the fixes.

Here is a git-am version of the fixes.

Please check and confirm it works for you.

Jeremy.
Comment 2 Bill Parker 2013-07-17 17:28:30 UTC
That looks good from this end...thanks for the additional code checks for
calls to free(), you'll probably want to take a look at #10022/#10021 as
well...

Bill


On Wed, Jul 17, 2013 at 10:18 AM, <samba-bugs@samba.org> wrote:

> https://bugzilla.samba.org/show_bug.cgi?id=10025
>
> Jeremy Allison <jra@samba.org> changed:
>
>            What    |Removed                     |Added
>
> ----------------------------------------------------------------------------
>    Attachment #9053 [details]|0                           |1
>         is obsolete|                            |
>
> --- Comment #1 from Jeremy Allison <jra@samba.org> 2013-07-17 17:18:57
> UTC ---
> Created attachment 9057 [details]
>   --> https://bugzilla.samba.org/attachment.cgi?id=9057
> git-am version of the fixes.
>
> Here is a git-am version of the fixes.
>
> Please check and confirm it works for you.
>
> Jeremy.
>
> --
> Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>