looking at latest sources from "master" in git, I've found a few problems: In lib/util/debug.c if (logfile && (*logfile) == '/') { fname = strdup(logfile); } else { asprintf(&fname, "%s/%s.log", dyn_LOGFILEBASE, state.prog_name); } if (fname) { Upon failed asprintf, fname is undefined, so must not be used at all. Instead, do something like this: if (logfile && (*logfile) == '/') { fname = strdup(logfile); } else { if (asprintf(&fname, "%s/%s.log", dyn_LOGFILEBASE, state.prog_name) < 0) fname = NULL; } if (fname) { Or better still, use an asprintf wrapper that does that for you. There's similar code in that same file: asprintf(&s, "[%s, %d %s:%s()]\n", t, level, location, func); talloc_free(t); if (!s) return; --------------------------------------- Looking at the PIDL generated code in librpc/gen_ndr/ndr_dcerpc.c, if (asprintf(&idx_0, "[%d]", cntr_transfer_syntaxes_0) != -1) { ndr_print_ndr_syntax_id(ndr, "transfer_syntaxes", &r->transfer_syntaxes[cntr_transfer_syntaxes_0]); free(idx_0); I noticed that the asprintf result is unused, so technically, removing all of those many asprintf calls would cause no change in semantics. It allocates memory into idx_0, but then simply frees it with no intervening use. However, that appears to be accidental. I think the following is what was intended: if (asprintf(&idx_0, "transfer_syntaxes[%d]", cntr_transfer_syntaxes_0) != -1) { ndr_print_ndr_syntax_id(ndr, idx_0, &r->transfer_syntaxes[cntr_transfer_syntaxes_0]); free(idx_0); So, it looks like pidl's Parser.pm can be adjusted to give better output or to avoid the asprintf call altogether. --------------------------- In nsswitch/winbind_nss_irix.c there is some asprintf abuse. There are numerous uses like this: rlen = asprintf(&result, "%s\n", response.data.winsresp); if (rlen == 0 || result == NULL) { return NSD_ERROR; } They should be like this: rlen = asprintf(&result, "%s\n", response.data.winsresp); if (rlen < 0) { return NSD_ERROR; } --------------------------- In source3/lib/afs.c there is an unchecked asprintf call: asprintf(&result, "%s\n%u\n%s\n%u\n%u\n%u\n%s\n", cell, ct->AuthHandle, base64_key, ct->ViceId, ct->BeginTimestamp, ct->EndTimestamp, base64_ticket); DEBUG(10, ("Got ticket string:\n%s\n", result)); ---------------------------------- There are two like this in source3/lib/sysquotas_4A.c asprintf("a_file,"/%s/%s%s",path, QUOTAFILENAME,USERQUOTAFILE_EXTENSION); if (quota_file == NULL) { source3/smbd/sesssetup.c: asprintf(&host_princ_s, "%s$@%s", global_myname( ), lp_realm()); source3/smbd/sesssetup.c- if (!host_princ_s) { source4/client/client.c- if (finfo->attrib & FILE_ATTRIBUTE_DIRECTORY) source4/client/client.c: asprintf(&quest, "Get directory %s? ",fi nfo->name); source4/client/client.c- else source4/client/client.c: asprintf(&quest, "Get file %s? ",finfo-> name); source4/client/client.c- source4/client/client.c- if (ctx->prompt && !yesno(quest)) return; source4/client/client.c- source4/client/client.c- SAFE_FREE(quest); source4/client/client.c: asprintf(&rname, "%s%s",ctx->remote_cur_ dir,finfo->name); source4/client/client.c- do_get(ctx, rname, finfo->name, false); source4/client/client.c- SAFE_FREE(rname); source4/client/client.c: asprintf(&dirmask, "%s%*s*", rl_ctx->rem ote_cur_dir, i-1, text); source4/client/client.c- } else source4/client/client.c: asprintf(&dirmask, "%s*", rl_ctx->remote _cur_dir); source4/client/client.c- source4/client/client.c- if (smbcli_list(rl_ctx->cli->tree, dirmask, source4/client/smbmount.c: asprintf(&smbmnt_path, "%s/smbmnt", dyn_ BINDIR); source4/client/smbmount.c- source4/client/smbmount.c- if (file_exist(smbmnt_path)) { ------------------------------------------ i stopped there. Here's a summary of the remaining easily-spotted abuses: [btw, I ran this: git grep -C3 -w asprintf|grep -v 'if.*asprintf'|grep ':[ ]*asprintf' source4/heimdal/kdc/digest.c: asprintf(&s, "%s-%s:%s", r.u.initReply.nonce, source4/heimdal/kdc/digest.c: asprintf(r.u.initReply.identifier, "%02X", identifier & 0xff); source4/heimdal/kdc/digest.c: asprintf(&r.u.error.reason, "Unsupported digest type %s", source4/heimdal/kdc/log.c: asprintf(&s, "0-1/FILE:%s/%s", hdb_db_dir(context), KDC_LOG_FILE); source4/heimdal/kdc/pkinit.c: asprintf(&fn, "%s/pki-mapping", hdb_db_dir(context)); source4/heimdal/kpasswd/kpasswd.c: asprintf(&msg, "New password: "); source4/heimdal/kpasswd/kpasswd.c: asprintf(&msg, "New password for %s: ", name); source4/heimdal/kuser/kinit.c: asprintf(&name, "ntlm-key-%s", domain); source4/heimdal/kuser/kinit.c: asprintf (&prompt, N_("%s's Password: ", ""), p); source4/heimdal/lib/asn1/asn1_gen.c: asprintf(&fnout, "%s.out", bname); source4/heimdal/lib/asn1/gen.c: asprintf(&header, "%s.h", headerbase); source4/heimdal/lib/asn1/gen.c: asprintf(&fn, "%s_files", base); source4/heimdal/lib/asn1/gen.c: asprintf (&filename, "%s_%s.x", STEM, name); source4/heimdal/lib/asn1/gen.c: asprintf (&n, "%s:1", m->gen_name); source4/heimdal/lib/asn1/gen.c: asprintf (&n, "*%s", m->gen_name); source4/heimdal/lib/asn1/gen.c: asprintf (&n, "*%s", m->gen_name); source4/heimdal/lib/asn1/gen_copy.c: asprintf (&fs, "%s(%s)->%s%s", source4/heimdal/lib/asn1/gen_copy.c: asprintf (&ts, "%s(%s)->%s%s", source4/heimdal/lib/asn1/gen_copy.c: asprintf(&f, "&(%s)->val[(%s)->len]", from, to); source4/heimdal/lib/asn1/gen_copy.c: asprintf(&T, "&(%s)->val[(%s)->len]", to, to); source4/heimdal/lib/asn1/gen_decode.c: asprintf (&s, "%s(%s)->%s", m->optional ? "" : "&", source4/heimdal/lib/asn1/gen_decode.c: asprintf (&s, "%s(%s)->%s", m->optional ? "" : "&", name, m->gen_name); source4/heimdal/lib/asn1/gen_decode.c: asprintf (&s, "%s->%s", name, m->gen_name); source4/heimdal/lib/asn1/gen_decode.c: asprintf (&n, "&(%s)->val[(%s)->len]", name, name); source4/heimdal/lib/asn1/gen_decode.c: asprintf (&sname, "%s_s_of", tmpstr); source4/heimdal/lib/asn1/gen_decode.c: asprintf (&tname, "%s_Tag", tmpstr); source4/heimdal/lib/asn1/gen_decode.c: asprintf (&s, "%s(%s)->u.%s", m->optional ? "" : "&", source4/heimdal/lib/asn1/gen_encode.c: asprintf (&s, "%s(%s)->%s", m->optional ? "" : "&", name, m->gen_name); source4/heimdal/lib/asn1/gen_encode.c: asprintf (&n, "&(%s)->val[i]", name); source4/heimdal/lib/asn1/gen_encode.c: asprintf (&sname, "%s_S_Of", tmpstr); source4/heimdal/lib/asn1/gen_encode.c: asprintf (&tname, "%s_tag", tmpstr); source4/heimdal/lib/asn1/gen_encode.c: asprintf (&s, "(%s)", name); source4/heimdal/lib/asn1/gen_encode.c: asprintf(&s2, "%s(%s)->u.%s", m->optional ? "" : "&", source4/heimdal/lib/asn1/gen_free.c: asprintf (&s, "%s(%s)->%s%s", source4/heimdal/lib/asn1/gen_free.c: asprintf (&n, "&(%s)->val[(%s)->len-1]", name, name); source4/heimdal/lib/asn1/gen_length.c: asprintf (&s, "%s(%s)->%s%s", source4/heimdal/lib/asn1/gen_length.c: asprintf (&n, "&(%s)->val[i]", name); source4/heimdal/lib/asn1/gen_length.c: asprintf (&sname, "%s_S_Of", tmpstr); source4/heimdal/lib/asn1/gen_length.c: asprintf(&tname, "%s_tag", tmpstr); source4/heimdal/lib/asn1/parse.c: asprintf(&m->label, "%s_%s", prefix, m->gen_name); source4/heimdal/lib/asn1/parse.c: asprintf(&p, "choice_%s", s->gen_name); source4/heimdal/lib/asn1/parse.y: asprintf(&m->label, "%s_%s", prefix, m->gen_name); source4/heimdal/lib/asn1/parse.y: asprintf(&p, "choice_%s", s->gen_name); source4/heimdal/lib/com_err/parse.c: asprintf (&prefix, "%s_", (yyvsp[(2) - (2)].string)); source4/heimdal/lib/com_err/parse.c: asprintf (&ec->name, "%s%s", prefix, (yyvsp[(2) - (4)].string)); source4/heimdal/lib/com_err/parse.y: asprintf (&prefix, "%s_", $2); source4/heimdal/lib/com_err/parse.y: asprintf (&ec->name, "%s%s", prefix, $2); source4/heimdal/lib/gssapi/krb5/accept_sec_context.c: asprintf(&p, "FILE:%s", identity); source4/heimdal/lib/gssapi/krb5/add_cred.c: asprintf(&type_name, "%s:%s", type, name); source4/heimdal/lib/gssapi/krb5/display_status.c: asprintf(&buf, "%s", source4/heimdal/lib/gssapi/krb5/display_status.c: asprintf (&buf, "%s %s", source4/heimdal/lib/gssapi/krb5/display_status.c: asprintf(&buf, "unknown mech error-code %u", source4/heimdal/lib/gssapi/mech/gss_display_status.c: asprintf(&buf, "%s", supplementary_error( source4/heimdal/lib/gssapi/mech/gss_display_status.c: asprintf (&buf, "%s %s", source4/heimdal/lib/gssapi/mech/gss_display_status.c: asprintf (&buf, "unknown mech-code %lu for mech %.*s", source4/heimdal/lib/gssapi/spnego/accept_sec_context.c: asprintf(&str, "host@%s", hostname); source4/heimdal/lib/hdb/db.c: asprintf(&old, "%s.db", db->hdb_name); source4/heimdal/lib/hdb/db.c: asprintf(&new, "%s.db", new_name); source4/heimdal/lib/hdb/db.c: asprintf(&fn, "%s.db", db->hdb_name); source4/heimdal/lib/hdb/dbinfo.c: asprintf(&di->mkey_file, "%s.mkey", di->dbname); source4/heimdal/lib/hdb/dbinfo.c: asprintf(&di->mkey_file, "%.*s.mkey", source4/heimdal/lib/hdb/ndbm.c: asprintf(&new_lock, "%s.lock", new_name); source4/heimdal/lib/hdb/ndbm.c: asprintf(&old_dir, "%s.dir", db->hdb_name); source4/heimdal/lib/hdb/ndbm.c: asprintf(&old_pag, "%s.pag", db->hdb_name); source4/heimdal/lib/hdb/ndbm.c: asprintf(&new_dir, "%s.dir", new_name); source4/heimdal/lib/hdb/ndbm.c: asprintf(&new_pag, "%s.pag", new_name); source4/heimdal/lib/hdb/ndbm.c: asprintf(&lock_file, "%s.lock", (char*)db->hdb_name); source4/heimdal/lib/hx509/ca.c: asprintf(&tstr, "ts-%lu", (unsigned long)t); source4/heimdal/lib/hx509/cert.c: asprintf(&buf, "%d", _hx509_cert_get_version(_hx509_get_cert(cert))); source4/heimdal/lib/hx509/cms.c: asprintf(str, "certificate issued by %s with serial number %s", source4/heimdal/lib/hx509/cms.c: asprintf(str, "certificate with id %s", keyid); source4/heimdal/lib/hx509/cms.c: asprintf(str, "certificate have unknown CMSidentifier type"); source4/heimdal/lib/hx509/ks_p11.c: asprintf(&slot->name, "%.*s", source4/heimdal/lib/hx509/ks_p11.c: asprintf(&str, "PIN code for %s: ", slot->name); source4/heimdal/lib/hx509/ks_p11.c: asprintf(&str, "%.*s", source4/heimdal/lib/krb5/acache.c: asprintf(str, "API:%s", name->data); source4/heimdal/lib/krb5/cache.c: asprintf(&append, "%u", (unsigned)getuid()); source4/heimdal/lib/krb5/fcache.c: asprintf (&file, "%sXXXXXX", KRB5_DEFAULT_CCFILE_ROOT); source4/heimdal/lib/krb5/init_creds_pw.c: asprintf (&p, "%s%s", str, ctime(&now)); source4/heimdal/lib/krb5/init_creds_pw.c: asprintf (&p, "%s: %.*s\n", source4/heimdal/lib/krb5/init_creds_pw.c: asprintf (&q, "%s's Password: ", p); source4/heimdal/lib/krb5/krbhst.c: asprintf(&host, "%s.%s.", serv_string, kd->realm); source4/heimdal/lib/krb5/krbhst.c: asprintf(&host, "%s-%d.%s.", source4/heimdal/lib/krb5/mcache.c: asprintf(&m->name, "%p", m); source4/heimdal/lib/krb5/plugin.c: asprintf(&path, "%s/%s", *di, entry->d_name); source4/heimdal/lib/krb5/replay.c: asprintf(&name, "FILE:rc_%s_%u", tmp, (unsigned)geteuid()); source4/heimdal/lib/krb5/replay.c: asprintf(&name, "FILE:rc_%s", tmp); source4/heimdal/lib/krb5/send_to_kdc.c: asprintf(&request, "GET %s%s HTTP/1.0\r\n\r\n", prefix, str); source4/heimdal/lib/krb5/send_to_kdc.c: asprintf(&prefix, "http://%s/", hi->hostname); source4/heimdal/lib/roken/roken.h.in: asprintf (char **, const char *, ...) source4/heimdal/lib/roken/roken_gethostby.c: asprintf(&dns_req, "http://%s:%d%s", dns_host, dns_port, dns_path); source4/heimdal/lib/roken/roken_gethostby.c: asprintf(&dns_req, "%s", dns_path); source4/heimdal/lib/roken/roken_gethostby.c: asprintf(&request, "GET %s?%s HTTP/1.0\r\n\r\n", dns_req, hostname); source4/lib/registry/dir.c: asprintf(&thispath, "%s/%s", dk->path, e->d_name); source4/lib/registry/tools/regshell.c: asprintf(&matches[0], "%s%s", base_n, matches[1]); source4/lib/registry/tools/regshell.c: asprintf(&matches[0], "%s%s", base_n, source4/lib/registry/tools/regshell.c: asprintf(&prompt, "%s> ", ctx->path); source4/libcli/clideltree.c: asprintf(&s, "%s%s", n, finfo->name); source4/libcli/clideltree.c: asprintf(&s2, "%s\\*", s); source4/libcli/clideltree.c: asprintf(&mask, "%s\\*", dname); source4/librpc/rpc/dcerpc_util.c: asprintf(&name, "%s/rpclog/%s-%u.%d.%s", source4/ntvfs/nbench/vfs_nbench.c: asprintf(&logname, "/tmp/nbenchlog%d.%u", ntvfs->depth, getpid()); source4/ntvfs/simple/svfs_util.c: asprintf(&full_name, "%s/%s", dir->unix_dir, dir->files[i].name); source4/ntvfs/simple/svfs_util.c: asprintf(&fd_path, "/proc/self/%d", fd); source4/ntvfs/simple/vfs_simple.c: asprintf(&pattern, "%s:*", unix_path); source4/param/loadparm.c: asprintf(&vfskey, "%s:%s", type, option); source4/scripting/python/modules.c: asprintf(&newpath, "%s/python:%s/../scripting/python:%s", bindir, bindir, Py_GetPath()); source4/smbd/pidfile.c: asprintf(&pidFile, "%s/%s.pid", piddir, name); source4/smbd/pidfile.c: asprintf(&pidFile, "%s/%s.pid", piddir, name); source4/smbd/process_thread.c: asprintf(&s, "thread[%u][%s]:\n", source4/torture/basic/base.c: asprintf(&control_char_fname, "\\readonly.afile"); source4/torture/basic/delete.c: asprintf(&fullname, "\\%s%s", dname, fname); source4/torture/basic/delete.c: asprintf(&fullname, "\\%s%s", dname, fname); source4/torture/basic/dir.c: asprintf(&fname, "\\%x", (int)random()); source4/torture/basic/dir.c: asprintf(&fname, "\\%x", (int)random()); source4/torture/basic/dir.c: asprintf(&fname, "\\LISTDIR\\f%d", i); source4/torture/basic/dir.c: asprintf(&fname, "\\LISTDIR\\d%d", i); source4/torture/basic/misc.c: asprintf(&fname, "\\torture.%u", n); source4/torture/basic/misc.c: asprintf(&fname, "\\maxfid\\fid%d", i/1000); source4/torture/basic/misc.c: asprintf(&fname, MAXFID_TEMPLATE, i/1000, i,(int)getpid()); source4/torture/basic/misc.c: asprintf(&fname, MAXFID_TEMPLATE, i/1000, i,(int)getpid()); source4/torture/basic/misc.c: asprintf(&fname, MAXFID_TEMPLATE, (maxfid-i)/1000, maxfid-i,(int)getpid()); source4/torture/locktest2.c: asprintf(&path, "%s%s", nfs, fname); source4/torture/nbench/nbench.c: asprintf(&cname, "client%d", client+1); source4/torture/raw/setfileinfo.c: asprintf(&path_fname, BASEDIR "\\fname_test_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&path_fname_new, BASEDIR "\\fname_test_new_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&fnum_fname, BASEDIR "\\fnum_test_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&fnum_fname_new, BASEDIR "\\fnum_test_new_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&path_fname, BASEDIR "\\fname_test_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&path_fname_new, BASEDIR "\\fname_test_new_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&fnum_fname, BASEDIR "\\fnum_test_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&fnum_fname_new, BASEDIR "\\fnum_test_new_%d.txt", n); source4/torture/raw/setfileinfo.c: asprintf(&path_dname, BASEDIR "\\dname_test_%d", n); source4/torture/raw/setfileinfo.c: asprintf(&path_dname_new, BASEDIR "\\dname_test_new_%d", n); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/rpc/samlogon.c: asprintf(error_string, "Expected error: %s, got %s", nt_errstr(samlogon_state->expected_error), nt_errstr(nt_status)); source4/torture/smbtorture.c: asprintf(&name, "%s-%s", prefix, c->name); source4/torture/smbtorture.c: asprintf(&name, "%s-%s", prefix, t->name); source4/utils/ntlm_auth.c: asprintf(unix_name, source4/utils/ntlm_auth.c: asprintf(&user, "%s%c%s", opt_domain, *lp_winbind_separator(cmdline_lp_ctx), opt_username); source4/web_server/wsgi.c: asprintf(&name, "HTTP_%s", hdr->name);
All asprintf have proper return code checks in samba3. samba4 folks, can you check you have proper return code checks for those as well ?
Jelmer, would take over the review of my proposed patchset: http://repo.or.cz/w/Samba/mdw.git/shortlog/refs/heads/asprintf
I've fixed and controlled the plain s4 "asprintf"s (the torture tests don't matter so I've ignored them). They should be okay now. Well and regarding the Heimdal Kerberos code parts I assign this up to Love - the maintainer.
Hi Matthias, Thanks for working on this. In general these patches seem alright, couple of things that I notice: * please fail when memory allocation failures occur rather than ignoring that (source4/web_server/wsgi.c). Otherwise we can end up with very weird and inconsistent behaviour. In Python, when you encounter a memory allocation failure, call "PyErr_NoMemory();" to make sure the right exception is set and then return NULL. * Don't exit() from helper functions, only from top-level functions. Otherwise it's impossible for a caller to get different behaviour and it makes it impossible to test particular behaviour as the test program would exit.
Hi Jelmer, (In reply to comment #4) > Hi Matthias, > > Thanks for working on this. In general these patches seem alright, couple of > things that I notice: > > * please fail when memory allocation failures occur rather than ignoring that > (source4/web_server/wsgi.c). Otherwise we can end up with very weird and > inconsistent behaviour. In Python, when you encounter a memory allocation > failure, call "PyErr_NoMemory();" to make sure the right exception is set and > then return NULL. I've fixed this - thanks for pointing this out. And what is about "bool py_update_path(const char *bindir)"? Is the return of "false" the right behaviour? Or should I also insert a "PyErr_NoMemory" before? > * Don't exit() from helper functions, only from top-level functions. Otherwise > it's impossible for a caller to get different behaviour and it makes it > impossible to test particular behaviour as the test program would exit. Do you mean in "void pidfile_create(const char *piddir, const char *name)"? There is no other return possibility - please look at the code. Matthias
I've got now also a confirmation from Love that Heimdal is now "asprintf" safe. Therefore we should be done now - marking this as "FIXED".