PR#158: Address issues discovered by clang and coverity - certmonger

		`@@ -158,11 +158,18 @@`
		`_exit(status);`
		`}`
		`/* Re-open the database with modules enabled */`
		`- NSS_ShutdownContext(ctx);`
		`+ if (NSS_ShutdownContext(ctx) != SECSuccess) {`
		`+ cm_log(0, "Error shutting down NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`ctx = NSS_InitContext(entry->cm_cert_storage_location,`
		`NULL, NULL, NULL, NULL,`
		`(readwrite ? 0 : NSS_INIT_READONLY) \|`
		`NSS_INIT_NOROOTINIT);`
		`+ if (ctx == NULL) {`
		`+ cm_log(0, "Unable to initialize NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`es = util_n_fips_hook();`
		`if (es != NULL) {`
		`cm_log(1, "Error putting NSS into FIPS mode: %s\n", es);`

src/certsave-n.c

file modified

+10 -1

		`@@ -186,11 +186,18 @@`
		`} else {`
		`/* We don't try to force FIPS mode here, as it seems to get in`
		`* the way of saving the certificate. */`
		`- NSS_ShutdownContext(ctx);`
		`+ if (NSS_ShutdownContext(ctx) != SECSuccess) {`
		`+ cm_log(0, "Error shutting down NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`ctx = NSS_InitContext(entry->cm_cert_storage_location,`
		`NULL, NULL, NULL, NULL,`
		`(readwrite ? 0 : NSS_INIT_READONLY) \|`
		`NSS_INIT_NOROOTINIT);`
		`+ if (ctx == NULL) {`
		`+ cm_log(0, "Unable to initialize NSS.\n");`
		`+ _exit(1);`
		`+ }`

		`/* Allocate a memory pool. */`
		`arena = PORT_NewArena(sizeof(double));`
		`@@ -510,6 +517,8 @@`
		`CK_INVALID_HANDLE,`
		`entry->cm_cert_nickname,`
		`PR_FALSE);`
		`+ } else {`
		`+ error = SECFailure;`
		`}`
		`if (error == SECSuccess) {`
		`cm_log(1, "Imported certificate with "`

src/certsave-o.c

file modified

		`@@ -411,6 +411,7 @@`
		`free(old_key);`
		`free(old_cert);`
		`free(next_keyfile);`
		`+ free(old_keyfile);`
		`if (status != 0) {`
		`_exit(status);`
		`}`

src/csrgen-o.c

file modified

+1 -1

		`@@ -181,7 +181,7 @@`
		`BIGNUM *serialbn;`
		`char buf[LINE_MAX], s, nickname, pin, password, *filename;`
		`unsigned char extensions, upassword, bmp, name, up, uq, md[CM_DIGEST_MAX];`
		`- char spkidec, mcb64, *nows;`
		`+ char spkidec = NULL, mcb64, *nows;`
		`const char default_cn = CM_DEFAULT_CERT_SUBJECT_CN, spkihex = NULL;`
		`const unsigned char *nametmp;`
		`struct tm *now;`

src/dogtag.c

file modified

+17 -1

		`@@ -118,9 +118,10 @@`
		`const char ssldir = NULL, cainfo = NULL, *capath = NULL;`
		`const char sslcert = NULL, sslkey = NULL;`
		`const char sslpin = NULL, sslpinfile = NULL;`
		`- const char csr = NULL, serial = NULL, *template = NULL;`
		`+ const char serial = NULL, template = NULL;`
		`const char uid = NULL, pwd = NULL, *pwdfile = NULL;`
		`const char udn = NULL, pin = NULL, *pinfile = NULL;`
		`+ char csr = NULL, csre = NULL;`
		`char *poptarg;`
		`struct {`
		`char *name;`
		`@@ -226,17 +227,22 @@`
		`if (strchr(poptarg, '=') == NULL) {`
		`printf(_("Profile params (-O) must be in the form of param=value.\n"));`
		`poptPrintUsage(pctx, stdout, 0);`
		`+ free(soptions);`
		`+ free(aoptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`aoptions = realloc(aoptions,`
		`++num_aoptions * sizeof(*aoptions));`
		`if (aoptions == NULL) {`
		`printf(_("Out of memory.\n"));`
		`+ free(soptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`p = strdup(poptarg);`
		`if (p == NULL) {`
		`printf(_("Out of memory.\n"));`
		`+ free(aoptions);`
		`+ free(soptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`i = strcspn(p, "=");`
		`@@ -249,17 +255,21 @@`
		`if (strchr(poptarg, '=') == NULL) {`
		`printf(_("Submit params (-o) must be in the form of param=value.\n"));`
		`poptPrintUsage(pctx, stdout, 0);`
		`+ free(soptions);`
		`+ free(aoptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`soptions = realloc(soptions,`
		`++num_soptions * sizeof(*soptions));`
		`if (soptions == NULL) {`
		`printf(_("Out of memory.\n"));`
		`+ free(aoptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`p = strdup(poptarg);`
		`if (p == NULL) {`
		`printf(_("Out of memory.\n"));`
		`+ free(soptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`i = strcspn(p, "=");`
		`@@ -293,6 +303,8 @@`
		`}`
		`if (c != -1) {`
		`poptPrintUsage(pctx, stdout, 0);`
		`+ free(soptions);`
		`+ free(aoptions);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`

		`@@ -472,6 +484,8 @@`
		`op = op_retrieve;`
		`}`
		`params = talloc_asprintf(ctx, "requestId=%s", q);`
		`+ free(p);`
		`+ free(q);`
		`} else {`
		`params = "";`
		`}`
		`@@ -548,6 +562,7 @@`
		`"xml=true",`
		`template,`
		`csr);`
		`+ free(csr);`
		`}`
		`/* Check for creds specified as options. */`
		`for (j = 0; j < num_soptions; j++) {`
		`@@ -564,6 +579,7 @@`
		`pin = NULL;`
		`}`
		`}`
		`+ free(soptions);`
		`/* Add client creds. */`
		`if (uid != NULL) {`
		`uid = cm_submit_u_url_encode(uid);`

src/getcert.c

file modified

+6 -2

		`@@ -4065,7 +4065,7 @@`
		`if (length == 0) {`
		`goto done;`
		`}`
		`- u = malloc(length);`
		`+ u = malloc(length + 1);`
		`if (u == NULL) {`
		`goto done;`
		`}`
		`@@ -4084,8 +4084,12 @@`
		`}`
		`*t++ = '\0';`
		`}`
		`- }`
		`+ } else {`
		`+ free(t);`
		`+ t = NULL;`
		`+ }`
		`done:`
		`+ free(t);`
		`free(u);`
		`return ret;`
		`}`

src/ipa.c

file modified

+24 -1

		`@@ -175,6 +175,11 @@`
		`lmsg = ldap_next_entry(ld, lmsg)) {`
		`lbvalues = ldap_get_values_len(ld, lmsg,`
		`lncattrs[0]);`
		`+ /* There should be only one defaultNamingContext so once we`
		`+ * have a value we're done. */`
		`+ if (*basedn != NULL) {`
		`+ break;`
		`+ }`
		`if (lbvalues == NULL) {`
		`continue;`
		`}`
		`@@ -771,6 +776,7 @@`
		`printf(_("Unable to determine principal name for "`
		`"signing request.\n"));`
		`poptPrintUsage(pctx, stdout, 0);`
		`+ free(reqprinc);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`if ((profile == NULL) &&`
		`@@ -813,6 +819,9 @@`
		`CM_SUBMIT_CSR_ENV);`
		`}`
		`free(csr);`
		`+ free(profile);`
		`+ free(issuer);`
		`+ free(reqprinc);`
		`poptPrintUsage(pctx, stdout, 0);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`@@ -855,6 +864,10 @@`
		`fprintf(stderr,`
		`"Error reading password from \"%s\": %s.\n",`
		`pwdfile, strerror(errno));`
		`+ free(csr);`
		`+ free(profile);`
		`+ free(issuer);`
		`+ free(reqprinc);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`}`
		`@@ -867,6 +880,10 @@`
		`if ((uid != NULL) \|\| (pwd != NULL)) {`
		`fprintf(stderr,`
		`"Both -u and -W/-w options should be specified.\n");`
		`+ free(csr);`
		`+ free(profile);`
		`+ free(issuer);`
		`+ free(reqprinc);`
		`return CM_SUBMIT_STATUS_UNCONFIGURED;`
		`}`
		`}`
		`@@ -901,6 +918,10 @@`
		`}`
		`}`
		`free(kerr);`
		`+ free(csr);`
		`+ free(profile);`
		`+ free(issuer);`
		`+ free(reqprinc);`
		`switch (kret) {`
		`case KRB5_KDC_UNREACH:`
		`case KRB5_REALM_CANT_RESOLVE:`
		`@@ -920,10 +941,12 @@`
		`basedn, uid, pwd, csr, reqprinc, profile,`
		`issuer);`
		`free(csr);`
		`+ free(profile);`
		`+ free(issuer);`
		`+ free(reqprinc);`
		`return ret;`
		`} else`
		`if (strcasecmp(mode, CM_OP_FETCH_ROOTS) == 0) {`
		`- free(csr);`
		`return fetch_roots(server, ldap_uri_cmd, ldap_uri, host,`
		`uid, pwd, domain, basedn);`
		`}`

src/keygen-n.c

file modified

+8 -1

		`@@ -226,11 +226,18 @@`
		`break;`
		`}`
		`}`
		`- NSS_ShutdownContext(ctx);`
		`+ if (NSS_ShutdownContext(ctx) != SECSuccess) {`
		`+ cm_log(0, "Error shutting down NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`ctx = NSS_InitContext(entry->cm_key_storage_location,`
		`NULL, NULL, NULL, NULL,`
		`(readwrite ? 0 : NSS_INIT_READONLY) \|`
		`NSS_INIT_NOROOTINIT);`
		`+ if (ctx == NULL) {`
		`+ cm_log(0, "Unable to initialize NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`reason = util_n_fips_hook();`
		`if (reason != NULL) {`
		`cm_log(1, "Error putting NSS into FIPS mode: %s\n", reason);`

src/keyiread-n.c

file modified

+8 -1

		`@@ -115,11 +115,18 @@`
		`break;`
		`}`
		`}`
		`- NSS_ShutdownContext(ctx);`
		`+ if (NSS_ShutdownContext(ctx) != SECSuccess) {`
		`+ cm_log(0, "Error shutting down NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`ctx = NSS_InitContext(entry->cm_key_storage_location,`
		`NULL, NULL, NULL, NULL,`
		`(readwrite ? 0 : NSS_INIT_READONLY) \|`
		`NSS_INIT_NOROOTINIT);`
		`+ if (ctx == NULL) {`
		`+ cm_log(0, "Unable to initialize NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`reason = util_n_fips_hook();`
		`if (reason != NULL) {`
		`cm_log(1, "Error putting NSS into FIPS mode: %s\n", reason);`

src/pkcs7.c

file modified

		`@@ -38,6 +38,7 @@`
		`#include <nss.h>`
		`#include <secasn1.h>`
		`#include <secitem.h>`
		`+ #include <prerror.h>`

		`#include <talloc.h>`

src/scep.c

file modified

+2 -2

		`@@ -1039,9 +1039,9 @@`
		`rval = CM_SUBMIT_STATUS_UNREACHABLE;`
		`goto done;`
		`}`
		`- if ((recipient_nonce_length != sent_nonce_length) \|\|`
		`+ if (sent_nonce && ((recipient_nonce_length != sent_nonce_length) \|\|`
		`(memcmp(recipient_nonce, sent_nonce,`
		`- sent_nonce_length) != 0)) {`
		`+ sent_nonce_length) != 0))) {`
		`printf(_("Error: reply nonce doesn't match request.\n"));`
		`rval = CM_SUBMIT_STATUS_UNREACHABLE;`
		`goto done;`

src/scepgen-n.c

file modified

+8 -1

		`@@ -183,11 +183,18 @@`
		`break;`
		`}`
		`}`
		`- NSS_ShutdownContext(ctx);`
		`+ if (NSS_ShutdownContext(ctx) != SECSuccess) {`
		`+ cm_log(0, "Error shutting down NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`ctx = NSS_InitContext(entry->cm_key_storage_location,`
		`NULL, NULL, NULL, NULL,`
		`NSS_INIT_READONLY \|`
		`NSS_INIT_NOROOTINIT);`
		`+ if (ctx == NULL) {`
		`+ cm_log(0, "Unable to initialize NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`reason = util_n_fips_hook();`
		`if (reason != NULL) {`
		`cm_log(0, "Error putting NSS into FIPS mode: %s\n", reason);`

src/srvloc.c

file modified

		`@@ -97,6 +97,9 @@`
		`if (r < 0) {`
		`r = -r;`
		`}`
		`+ if (range == 0) {`
		`+ return 0;`
		`+ }`
		`return r % range;`
		`}`
		`#else`

src/store-files.c

file modified

		`@@ -559,6 +559,7 @@`
		`break;`
		`}`
		`free(buf);`
		`+ buf = NULL;`
		`}`
		`/* If we were reading a line, append it to the list. */`
		`if (s != NULL) {`
		`@@ -573,6 +574,7 @@`
		`lines = tlines;`
		`}`
		`}`
		`+ free(buf);`
		`return lines;`
		`}`

src/submit-d.c

file modified

		`@@ -203,6 +203,7 @@`
		`ret = malloc(sizeof(ret) `
		`(obj->nodesetval->nodeNr + 1));`
		`if (ret == NULL) {`
		`+ xmlFree(obj);`
		`return NULL;`
		`}`
		`memset(ret, 0,`
		`@@ -358,6 +359,8 @@`
		`v = cm_submit_d_text_node(parent, vobj);`
		`xmlXPathFreeObject(vobj);`
		`if ((v == NULL) \|\| (strlen(v) == 0)) {`
		`+ xmlFree(bpath1);`
		`+ xmlFree(bpath2);`
		`return NULL;`
		`}`
		`bobj1 = NULL;`
		`@@ -1270,6 +1273,7 @@`
		`printf("default: %s=%s\n", p, q);`
		`}`
		`}`
		`+ free(defaults);`
		`cm_submit_d_approve_result(hctx, result,`
		`&error_code, &error_reason,`
		`&error, &status, &requestId);`

src/submit-n.c

file modified

+8 -1

		`@@ -317,11 +317,18 @@`
		`}`
		`goto done;`
		`}`
		`- NSS_ShutdownContext(ctx);`
		`+ if (NSS_ShutdownContext(ctx) != SECSuccess) {`
		`+ cm_log(0, "Error shutting down NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`ctx = NSS_InitContext(args->entry->cm_key_storage_location,`
		`NULL, NULL, NULL, NULL,`
		`NSS_INIT_READONLY \|`
		`NSS_INIT_NOROOTINIT);`
		`+ if (ctx == NULL) {`
		`+ cm_log(0, "Unable to initialize NSS.\n");`
		`+ _exit(1);`
		`+ }`
		`reason = util_n_fips_hook();`
		`if (reason != NULL) {`
		`cm_log(1, "Error putting NSS into FIPS mode: %s\n", reason);`

src/submit-u.c

file modified

		`@@ -105,6 +105,9 @@`
		`if (csr[length-1] != '\n') {`
		`length += 1;`
		`csr = realloc(csr, length + 1);`
		`+ if (csr == NULL) {`
		`+ return NULL;`
		`+ }`
		`csr[length - 1] = '\n';`
		`csr[length] = '\0';`
		`}`

src/submit-x.c

file modified

+3 -3

		`@@ -914,9 +914,8 @@`

		`/* Maybe we need a ccache. */`
		`if (k5 \|\| (kpname != NULL) \|\| (ktname != NULL)) {`
		`- if (!make_ccache \|\|`
		`- (cm_submit_x_make_ccache(ktname, kpname, NULL) == 0)) {`
		`- k5 = TRUE;`
		`+ if (!make_ccache) {`
		`+ cm_submit_x_make_ccache(ktname, kpname, NULL);`
		`}`
		`}`

		`@@ -935,6 +934,7 @@`
		`} else {`
		`cm_submit_x_add_named_arg_s(ctx, skey, sval);`
		`}`
		`+ free(skey);`
		`}`

		`/* Submit the request. */`

src/tdbush.c

file modified

		`@@ -2129,6 +2129,7 @@`
		`}`

		`/* org.fedorahosted.certonger.ca.get_config_file_path */`
		`+ static DBusHandlerResult`
		`ca_get_config_file_path(DBusConnection conn, DBusMessage msg,`
		`struct cm_client_info ci, struct cm_context ctx)`
		`{`

src/util.c

file modified

+5 -1

		`@@ -155,7 +155,11 @@`

		`/* Skip over any whitespace after the equal sign. */`
		`line = strchr(line, '=');`
		`- line++;`
		`+ if (line == NULL) {`
		`+ free(data);`
		`+ return NULL;`
		`+ }`
		`+ line++;`
		`while (isspace((unsigned char)line) && (line != '\0'))`
		`line++;`

tests/tools/pk7env.c

file modified

		`@@ -110,6 +110,7 @@`
		`}`
		`len += j;`
		`}`
		`+ p[1][len] = '\0';`
		`close(fd);`
		`i++;`

tests/tools/prefs.c

file modified

		`@@ -35,6 +35,9 @@`
		`case cm_prefs_aes128:`
		`printf("cipher: AES128\n");`
		`break;`
		`+ case cm_prefs_aes192:`
		`+ printf("cipher: AES192\n");`
		`+ break;`
		`case cm_prefs_aes256:`
		`printf("cipher: AES256\n");`
		`break;`
		`@@ -44,6 +47,9 @@`
		`case cm_prefs_des3:`
		`printf("cipher: DES3\n");`
		`break;`
		`+ case cm_prefs_nocipher:`
		`+ printf("No cipher selected. Shouldn't happen\n");`
		`+ break;`
		`}`
		`switch (cm_prefs_preferred_digest()) {`
		`case cm_prefs_md5:`
		`@@ -61,6 +67,9 @@`
		`case cm_prefs_sha512:`
		`printf("digest: SHA512\n");`
		`break;`
		`+ case cm_prefs_nodigest:`
		`+ printf("No cipher selected. Shouldn't happen\n");`
		`+ break;`
		`}`

		`if (cm_prefs_notify_ttls(&ttls, &n_ttls) == 0) {`

rcritten commented 3 years ago

Address an array of memory leaks, dead code, uninitialized values, unchecked return values and more.

Most of the memory leaks aren't really a problem since they occur in short-lived helpers but it reduces the total number of warnings/errors reported by the scanning tools which makes finding any real problems a lot easier.

Metadata

Assignee

None

Tags

No Tags

Changes Summary 22

		`@@ -181,7 +181,7 @@`
		`BIGNUM *serialbn;`
		`char buf[LINE_MAX], s, nickname, pin, password, *filename;`
		`unsigned char extensions, upassword, bmp, name, up, uq, md[CM_DIGEST_MAX];`
		`- char spkidec, mcb64, *nows;`
		`+ char spkidec = NULL, mcb64, *nows;`
		`const char default_cn = CM_DEFAULT_CERT_SUBJECT_CN, spkihex = NULL;`
		`const unsigned char *nametmp;`
		`struct tm *now;`

certmonger

Source Code

Documentation

#158 Address issues discovered by clang and coverity Merged 3 years ago by rcritten. Opened 3 years ago by rcritten. rcritten/certmonger clang_coverity into master

Metadata

Changes Summary 22

#158 Address issues discovered by clang and coverity

Merged 3 years ago by rcritten. Opened 3 years ago by rcritten.

rcritten/certmonger clang_coverity into master