From: Jonathan McDowell Date: Sat, 9 Nov 2013 16:34:08 +0000 (-0800) Subject: Fix issues found by llvm scan-build static analysis X-Git-Tag: onak-0.4.3~32 X-Git-Url: https://the.earth.li/gitweb/?a=commitdiff_plain;h=83ae316a7b14e55418349e87d1a1942a0627ae14;p=onak.git Fix issues found by llvm scan-build static analysis A mixture of fixes: some variables set but never used (fix by either removing or correctly checking error codes), some unlikely but possible memory leaks, some invalid casting. --- diff --git a/decodekey.c b/decodekey.c index b4f7ceb..aeb7c62 100644 --- a/decodekey.c +++ b/decodekey.c @@ -61,11 +61,11 @@ int parse_subpackets(unsigned char *data, uint64_t *keyid, time_t *creation) } else if (packetlen == 255) { packetlen = data[offset++]; packetlen <<= 8; - packetlen = data[offset++]; + packetlen |= data[offset++]; packetlen <<= 8; - packetlen = data[offset++]; + packetlen |= data[offset++]; packetlen <<= 8; - packetlen = data[offset++]; + packetlen |= data[offset++]; } switch (data[offset] & 0x7F) { case OPENPGP_SIGSUB_CREATION: diff --git a/getcgi.c b/getcgi.c index 2035841..b36ad9b 100644 --- a/getcgi.c +++ b/getcgi.c @@ -200,14 +200,14 @@ char **getcgivars(int argc, char *argv[]) for(i=0; cgiinput[i]; i++) if (cgiinput[i]=='+') cgiinput[i] = ' '; /* First, split on "&" to extract the name-value pairs into pairlist */ - pairlist=(char **) malloc(256*sizeof(char **)); + pairlist= malloc(256*sizeof(char *)); paircount=0; nvpair=strtok(cgiinput, "&"); while (nvpair) { pairlist[paircount++]= strdup(nvpair) ; if (!(paircount%256)) { - pairlist=(char **) realloc(pairlist, - (paircount+256)*sizeof(char **)); + pairlist= realloc(pairlist, + (paircount+256)*sizeof(char *)); } nvpair=strtok(NULL, "&") ; } @@ -216,7 +216,7 @@ char **getcgivars(int argc, char *argv[]) /* Then, from the list of pairs, extract the names and values */ - cgivars=(char **) malloc((paircount*2+1)*sizeof(char **)); + cgivars= malloc((paircount*2+1)*sizeof(char *)); for (i=0; iendtrans(dbctx); - intrans = false; } if (sendsync && keys != NULL) { diff --git a/keydb_db4.c b/keydb_db4.c index 9d5dd90..6a21fab 100644 --- a/keydb_db4.c +++ b/keydb_db4.c @@ -176,16 +176,18 @@ static int db4_upgradedb(struct onak_db4_dbctx *privctx) logthing(LOGTHING_NOTICE, "Upgrading DB4 database"); ret = db_env_create(&privctx->dbenv, 0); - privctx->dbenv->set_errcall(privctx->dbenv, &db4_errfunc); - privctx->dbenv->remove(privctx->dbenv, config.db_dir, 0); - privctx->dbenv = NULL; + if (ret == 0) { + privctx->dbenv->set_errcall(privctx->dbenv, &db4_errfunc); + privctx->dbenv->remove(privctx->dbenv, config.db_dir, 0); + privctx->dbenv = NULL; + } for (i = 0; i < privctx->numdbs; i++) { ret = db_create(&curdb, NULL, 0); if (ret == 0) { snprintf(buf, sizeof(buf) - 1, "%s/keydb.%d.db", config.db_dir, i); logthing(LOGTHING_DEBUG, "Upgrading %s", buf); - ret = curdb->upgrade(curdb, buf, 0); + curdb->upgrade(curdb, buf, 0); curdb->close(curdb, 0); } else { logthing(LOGTHING_ERROR, "Error upgrading DB %s : %s", @@ -198,7 +200,7 @@ static int db4_upgradedb(struct onak_db4_dbctx *privctx) if (ret == 0) { snprintf(buf, sizeof(buf) - 1, "%s/worddb", config.db_dir); logthing(LOGTHING_DEBUG, "Upgrading %s", buf); - ret = curdb->upgrade(curdb, buf, 0); + curdb->upgrade(curdb, buf, 0); curdb->close(curdb, 0); } else { logthing(LOGTHING_ERROR, "Error upgrading DB %s : %s", @@ -210,7 +212,7 @@ static int db4_upgradedb(struct onak_db4_dbctx *privctx) if (ret == 0) { snprintf(buf, sizeof(buf) - 1, "%s/id32db", config.db_dir); logthing(LOGTHING_DEBUG, "Upgrading %s", buf); - ret = curdb->upgrade(curdb, buf, 0); + curdb->upgrade(curdb, buf, 0); curdb->close(curdb, 0); } else { logthing(LOGTHING_ERROR, "Error upgrading DB %s : %s", @@ -222,7 +224,7 @@ static int db4_upgradedb(struct onak_db4_dbctx *privctx) if (ret == 0) { snprintf(buf, sizeof(buf) - 1, "%s/skshashdb", config.db_dir); logthing(LOGTHING_DEBUG, "Upgrading %s", buf); - ret = curdb->upgrade(curdb, buf, 0); + curdb->upgrade(curdb, buf, 0); curdb->close(curdb, 0); } else { logthing(LOGTHING_ERROR, "Error upgrading DB %s : %s", @@ -234,7 +236,7 @@ static int db4_upgradedb(struct onak_db4_dbctx *privctx) if (ret == 0) { snprintf(buf, sizeof(buf) - 1, "%s/subkeydb", config.db_dir); logthing(LOGTHING_DEBUG, "Upgrading %s", buf); - ret = curdb->upgrade(curdb, buf, 0); + curdb->upgrade(curdb, buf, 0); curdb->close(curdb, 0); } else { logthing(LOGTHING_ERROR, "Error upgrading DB %s : %s", @@ -270,6 +272,10 @@ static uint64_t db4_getfullkeyid(struct onak_dbctx *dbctx, uint64_t keyid) &cursor, 0); /* flags */ + if (ret != 0) { + return 0; + } + shortkeyid = keyid & 0xFFFFFFFF; memset(&key, 0, sizeof(key)); @@ -292,7 +298,7 @@ static uint64_t db4_getfullkeyid(struct onak_dbctx *dbctx, uint64_t keyid) } } - ret = cursor->c_close(cursor); + cursor->c_close(cursor); cursor = NULL; } @@ -444,6 +450,11 @@ static int db4_fetch_key_text(struct onak_dbctx *dbctx, const char *search, &cursor, 0); /* flags */ + if (ret != 0) { + db4_endtrans(dbctx); + break; + } + memset(&key, 0, sizeof(key)); memset(&data, 0, sizeof(data)); key.data = curword->object; @@ -488,7 +499,7 @@ static int db4_fetch_key_text(struct onak_dbctx *dbctx, const char *search, free(data.data); data.data = NULL; } - ret = cursor->c_close(cursor); + cursor->c_close(cursor); cursor = NULL; firstpass = 0; db4_endtrans(dbctx); @@ -530,6 +541,10 @@ static int db4_fetch_key_skshash(struct onak_dbctx *dbctx, &cursor, 0); /* flags */ + if (ret != 0) { + return 0; + } + memset(&key, 0, sizeof(key)); memset(&data, 0, sizeof(data)); key.data = (void *) hash->hash; @@ -550,7 +565,7 @@ static int db4_fetch_key_skshash(struct onak_dbctx *dbctx, } } - ret = cursor->c_close(cursor); + cursor->c_close(cursor); cursor = NULL; return db4_fetch_key_id(dbctx, keyid, publickey, false); @@ -600,7 +615,7 @@ static int db4_delete_key(struct onak_dbctx *dbctx, wordlist = makewordlist(wordlist, uids[i]); } - ret = privctx->worddb->cursor(privctx->worddb, + privctx->worddb->cursor(privctx->worddb, privctx->txn, &cursor, 0); /* flags */ @@ -651,44 +666,46 @@ static int db4_delete_key(struct onak_dbctx *dbctx, } } } - ret = cursor->c_close(cursor); + cursor->c_close(cursor); cursor = NULL; ret = privctx->skshashdb->cursor(privctx->skshashdb, privctx->txn, &cursor, 0); /* flags */ - get_skshash(publickey, &hash); + if (ret == 0) { + get_skshash(publickey, &hash); - memset(&key, 0, sizeof(key)); - memset(&data, 0, sizeof(data)); - key.data = hash.hash; - key.size = sizeof(hash.hash); - data.data = &keyid; - data.size = sizeof(keyid); + memset(&key, 0, sizeof(key)); + memset(&data, 0, sizeof(data)); + key.data = hash.hash; + key.size = sizeof(hash.hash); + data.data = &keyid; + data.size = sizeof(keyid); - ret = cursor->c_get(cursor, - &key, - &data, - DB_GET_BOTH); + ret = cursor->c_get(cursor, + &key, + &data, + DB_GET_BOTH); - if (ret == 0) { - ret = cursor->c_del(cursor, 0); - } + if (ret == 0) { + ret = cursor->c_del(cursor, 0); + } - if (ret != 0) { - logthing(LOGTHING_ERROR, - "Problem deleting skshash: %s " - "(0x%016" PRIX64 ")", - db_strerror(ret), - keyid); - if (ret == DB_LOCK_DEADLOCK) { - deadlock = true; + if (ret != 0) { + logthing(LOGTHING_ERROR, + "Problem deleting skshash: %s " + "(0x%016" PRIX64 ")", + db_strerror(ret), + keyid); + if (ret == DB_LOCK_DEADLOCK) { + deadlock = true; + } } - } - ret = cursor->c_close(cursor); - cursor = NULL; + cursor->c_close(cursor); + cursor = NULL; + } /* * Free our UID and word lists. @@ -705,7 +722,7 @@ static int db4_delete_key(struct onak_dbctx *dbctx, } if (!deadlock) { - ret = privctx->id32db->cursor(privctx->id32db, + privctx->id32db->cursor(privctx->id32db, privctx->txn, &cursor, 0); /* flags */ @@ -791,9 +808,8 @@ static int db4_delete_key(struct onak_dbctx *dbctx, free(subkeyids); subkeyids = NULL; } - ret = cursor->c_close(cursor); + cursor->c_close(cursor); cursor = NULL; - } if (!deadlock) { @@ -1129,6 +1145,10 @@ static int db4_iterate_keys(struct onak_dbctx *dbctx, &cursor, 0); /* flags */ + if (ret != 0) { + continue; + } + memset(&dbkey, 0, sizeof(dbkey)); memset(&data, 0, sizeof(data)); ret = cursor->c_get(cursor, &dbkey, &data, DB_NEXT); @@ -1159,7 +1179,7 @@ static int db4_iterate_keys(struct onak_dbctx *dbctx, db_strerror(ret)); } - ret = cursor->c_close(cursor); + cursor->c_close(cursor); cursor = NULL; } @@ -1371,8 +1391,10 @@ struct onak_dbctx *keydb_db4_init(bool readonly) "Error opening db environment: %s (%s)", config.db_dir, db_strerror(ret)); - privctx->dbenv->close(privctx->dbenv, 0); - privctx->dbenv = NULL; + if (privctx->dbenv != NULL) { + privctx->dbenv->close(privctx->dbenv, 0); + privctx->dbenv = NULL; + } } } diff --git a/keydb_fs.c b/keydb_fs.c index 9e072ba..a152945 100644 --- a/keydb_fs.c +++ b/keydb_fs.c @@ -452,7 +452,7 @@ static struct ll *internal_get_key_by_word(char *word, struct ll *mct) d = opendir(buffer); logthing(LOGTHING_DEBUG, "Scanning for word %s in dir %s", word, buffer); - if (d) + if (d) { do { de = readdir(d); if (de && de->d_name[0] != '.') { @@ -470,7 +470,8 @@ static struct ll *internal_get_key_by_word(char *word, struct ll *mct) } } } while (de); - closedir(d); + closedir(d); + } return keys; } @@ -640,10 +641,9 @@ struct onak_dbctx *keydb_fs_init(bool readonly) privctx->lockfile_fd = open(buffer, O_RDWR | O_CREAT, 0600); } chdir(config.db_dir); - if (privctx->lockfile_fd == -1) - privctx->lockfile_fd = open(buffer, - (privctx->lockfile_readonly) ? - O_RDONLY : O_RDWR); + privctx->lockfile_fd = open(buffer, + (privctx->lockfile_readonly) ? + O_RDONLY : O_RDWR); if (privctx->lockfile_fd == -1) privctx->lockfile_fd = open(buffer, O_RDWR | O_CREAT, 0600); if (privctx->lockfile_fd == -1) { diff --git a/keydb_hkp.c b/keydb_hkp.c index 7c7b31a..8bb8c02 100644 --- a/keydb_hkp.c +++ b/keydb_hkp.c @@ -52,7 +52,7 @@ static int hkp_parse_url(struct onak_hkp_dbctx *privctx, const char *url) &port); if (matched < 2) { proto[0] = 0; - matched = sscanf(url, "%256[a-zA-Z0-9.]:%u", host, &port); + sscanf(url, "%256[a-zA-Z0-9.]:%u", host, &port); } if (host[0] == 0) { diff --git a/keydb_keyd.c b/keydb_keyd.c index 5fd0843..1d10928 100644 --- a/keydb_keyd.c +++ b/keydb_keyd.c @@ -489,7 +489,6 @@ static void keyd_cleanupdb(struct onak_dbctx *dbctx) logthing(LOGTHING_NOTICE, "Error closing down socket: %d", errno); } - keyd_fd = -1; free(dbctx); @@ -559,6 +558,13 @@ struct onak_dbctx *keydb_keyd_init(bool readonly) } count = read(keyd_fd, &reply, sizeof(reply)); + if (count != sizeof(reply)) { + logthing(LOGTHING_CRITICAL, + "Error! Unexpected keyd version " + "length: %d != %d", + count, sizeof(reply)); + exit(EXIT_FAILURE); + } logthing(LOGTHING_DEBUG, "keyd protocol version %d", reply); diff --git a/keydctl.c b/keydctl.c index 47492c5..ef942db 100644 --- a/keydctl.c +++ b/keydctl.c @@ -154,10 +154,17 @@ static void keyd_status(void) uint32_t reply; struct keyd_stats stats; - keyd_do_command(KEYD_CMD_VERSION, &reply, sizeof(reply)); + if (keyd_do_command(KEYD_CMD_VERSION, &reply, sizeof(reply)) == -1) { + printf("Got failure asking for keyd version.\n"); + return; + } printf("Using keyd protocol version %d.\n", reply); - keyd_do_command(KEYD_CMD_STATS, &stats, sizeof(stats)); + if (keyd_do_command(KEYD_CMD_STATS, &stats, sizeof(stats)) == -1) { + printf("Got failure asking for keyd statistics.\n"); + return; + } + printf("keyd running since %s", ctime(&stats.started)); printf("%d client connections received\n", stats.connects); diff --git a/maxpath.c b/maxpath.c index fbd5a17..0932df3 100644 --- a/maxpath.c +++ b/maxpath.c @@ -91,12 +91,16 @@ int main(int argc, char *argv[]) while ((optchar = getopt(argc, argv, "c:")) != -1 ) { switch (optchar) { case 'c': + if (configfile != NULL) { + free(configfile); + } configfile = strdup(optarg); break; } } readconfig(configfile); + free(configfile); initlogthing("maxpath", config.logfile); dbctx = config.dbinit(true); if (dbctx != NULL) { diff --git a/onak.c b/onak.c index dd39bb3..7a5d693 100644 --- a/onak.c +++ b/onak.c @@ -166,7 +166,6 @@ int main(int argc, char *argv[]) int i; bool ishex = false; bool isfp = false; - bool verbose = false; bool update = false; bool binary = false; bool fingerprint = false; @@ -194,7 +193,6 @@ int main(int argc, char *argv[]) update = true; break; case 'v': - verbose = true; setlogthreshold(LOGTHING_INFO); break; } diff --git a/parsekey.c b/parsekey.c index f780520..ed61e24 100644 --- a/parsekey.c +++ b/parsekey.c @@ -194,7 +194,6 @@ onak_status_t read_openpgp_stream(int (*getchar_func)(void *ctx, size_t count, struct openpgp_packet_list *curpacket = NULL, **packetend = NULL; onak_status_t rc = ONAK_E_OK; int keys = 0; - bool inpacket = false; if (packets == NULL) return ONAK_E_INVALID_PARAM; @@ -206,14 +205,12 @@ onak_status_t read_openpgp_stream(int (*getchar_func)(void *ctx, size_t count, } } - while (!rc && (maxnum == 0 || keys < maxnum) && + while (rc == ONAK_E_OK && (maxnum == 0 || keys < maxnum) && !getchar_func(ctx, 1, &curchar)) { - if (!inpacket && (curchar & 0x80)) { + if (curchar & 0x80) { /* - * New packet. Record the fact we're in a packet and - * allocate memory for it. + * New packet. Allocate memory for it. */ - inpacket = true; if (curpacket != NULL) { curpacket->next = malloc(sizeof (*curpacket)); packetend = &curpacket->next; @@ -236,7 +233,10 @@ onak_status_t read_openpgp_stream(int (*getchar_func)(void *ctx, size_t count, */ if (curpacket->packet->newformat) { curpacket->packet->tag = (curchar & 0x3F); - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length = curchar; if (curpacket->packet->length > 191 && curpacket->packet->length < 224) { @@ -255,43 +255,76 @@ onak_status_t read_openpgp_stream(int (*getchar_func)(void *ctx, size_t count, * 5 byte length; ie 255 followed by 3 * bytes of MSB length. */ - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length = curchar; curpacket->packet->length <<= 8; - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += curchar; curpacket->packet->length <<= 8; - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += curchar; curpacket->packet->length <<= 8; - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += curchar; } } else { curpacket->packet->tag = (curchar & 0x3C) >> 2; switch (curchar & 3) { case 0: - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length = curchar; break; case 1: - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length = curchar; curpacket->packet->length <<= 8; - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += curchar; break; case 2: - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length = (curchar << 24); - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += (curchar << 16); - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += (curchar << 8); - rc = getchar_func(ctx, 1, &curchar); + if (getchar_func(ctx, 1, &curchar)) { + rc = ONAK_E_INVALID_PKT; + break; + } curpacket->packet->length += curchar; break; case 3: @@ -318,7 +351,6 @@ onak_status_t read_openpgp_stream(int (*getchar_func)(void *ctx, size_t count, curpacket->packet->data); } } - inpacket = false; } else { rc = ONAK_E_INVALID_PKT; } diff --git a/sixdegrees.c b/sixdegrees.c index a842230..b806c3e 100644 --- a/sixdegrees.c +++ b/sixdegrees.c @@ -119,7 +119,7 @@ void sixdegrees(struct onak_dbctx *dbctx, uint64_t keyid) * if it's signed by the key we're looking at. */ initcolour(false); - degree = countdegree(dbctx, keyinfo, true, 7); + countdegree(dbctx, keyinfo, true, 7); puts("\t\tSigned by\t\tSigns"); for (loop = 1; loop < 7; loop++) { @@ -143,6 +143,9 @@ int main(int argc, char *argv[]) while ((optchar = getopt(argc, argv, "c:")) != -1 ) { switch (optchar) { case 'c': + if (configfile != NULL) { + free(configfile); + } configfile = strdup(optarg); break; } @@ -153,6 +156,7 @@ int main(int argc, char *argv[]) } readconfig(configfile); + free(configfile); initlogthing("sixdegrees", config.logfile); dbctx = config.dbinit(true); if (dbctx != NULL) {