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)
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
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. >
(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