Skip to content

Commit

Permalink
Fix many unlikely memory leaks
Browse files Browse the repository at this point in the history 
These are on error paths and often require allocation failures, so are
unlikely to be issues in practice.  Reported by Coverity and cppcheck.
  • Loading branch information
@frozencemetery @greghudson
frozencemetery authored and greghudson committed Jul 1, 2021
1 parent cddd5c5 commit a06945b
Show file tree
Hide file tree
Showing 24 changed files with 198 additions and 158 deletions.
19 changes: 11 additions & 8 deletions src/kadmin/dbutil/kadm5_create.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,21 +185,24 @@ static int add_admin_princs(void *handle, krb5_context context, char *realm)
int add_admin_princ(void *handle, krb5_context context,
char *name, char *realm, int attrs, int lifetime)
{
char *fullname;
char *fullname = NULL;
krb5_error_code ret;
kadm5_principal_ent_rec ent;
long flags;
int fret;

memset(&ent, 0, sizeof(ent));

if (asprintf(&fullname, "%s@%s", name, realm) < 0) {
com_err(progname, ENOMEM, _("while appending realm to principal"));
return ERR;
fret = ERR;
goto cleanup;
}
ret = krb5_parse_name(context, fullname, &ent.principal);
if (ret) {
com_err(progname, ret, _("while parsing admin principal name"));
return(ERR);
fret = ERR;
goto cleanup;
}
ent.max_life = lifetime;
ent.attributes = attrs;
Expand All @@ -210,13 +213,13 @@ int add_admin_princ(void *handle, krb5_context context,
ret = kadm5_create_principal(handle, &ent, flags, NULL);
if (ret && ret != KADM5_DUP) {
com_err(progname, ret, _("while creating principal %s"), fullname);
krb5_free_principal(context, ent.principal);
free(fullname);
return ERR;
fret = ERR;
goto cleanup;
}

fret = OK;
cleanup:
krb5_free_principal(context, ent.principal);
free(fullname);

return OK;
return fret;
}
23 changes: 12 additions & 11 deletions src/kadmin/dbutil/kdb5_create.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ add_principal(context, princ, op, pblock)
struct realm_info *pblock;
{
krb5_error_code retval;
krb5_db_entry *entry;
krb5_db_entry *entry = NULL;
krb5_kvno mkey_kvno;
krb5_timestamp now;
struct iterate_args iargs;
Expand All @@ -410,20 +410,20 @@ add_principal(context, princ, op, pblock)
entry->expiration = pblock->expiration;

if ((retval = krb5_copy_principal(context, princ, &entry->princ)))
goto error_out;
goto cleanup;

if ((retval = krb5_timeofday(context, &now)))
goto error_out;
goto cleanup;

if ((retval = krb5_dbe_update_mod_princ_data(context, entry,
now, &db_create_princ)))
goto error_out;
goto cleanup;

switch (op) {
case MASTER_KEY:
if ((entry->key_data=(krb5_key_data*)malloc(sizeof(krb5_key_data)))
== NULL)
goto error_out;
goto cleanup;
memset(entry->key_data, 0, sizeof(krb5_key_data));
entry->n_key_data = 1;

Expand All @@ -435,7 +435,7 @@ add_principal(context, princ, op, pblock)
if ((retval = krb5_dbe_encrypt_key_data(context, pblock->key,
&master_keyblock, NULL,
mkey_kvno, entry->key_data)))
return retval;
goto cleanup;
/*
* There should always be at least one "active" mkey so creating the
* KRB5_TL_ACTKVNO entry now so the initial mkey is active.
Expand All @@ -445,11 +445,11 @@ add_principal(context, princ, op, pblock)
/* earliest possible time in case system clock is set back */
actkvno.act_time = 0;
if ((retval = krb5_dbe_update_actkvno(context, entry, &actkvno)))
return retval;
goto cleanup;

/* so getprinc shows the right kvno */
if ((retval = krb5_dbe_update_mkvno(context, entry, mkey_kvno)))
return retval;
goto cleanup;

break;
case TGT_KEY:
Expand All @@ -464,10 +464,11 @@ add_principal(context, princ, op, pblock)
1,
tgt_keysalt_iterate,
(krb5_pointer) &iargs)))
return retval;
goto cleanup;
break;
case NULL_KEY:
return EOPNOTSUPP;
retval = EOPNOTSUPP;
goto cleanup;
default:
break;
}
Expand All @@ -479,7 +480,7 @@ add_principal(context, princ, op, pblock)

retval = krb5_db_put_principal(context, entry);

error_out:
cleanup:
krb5_db_free_principal(context, entry);
return retval;
}
3 changes: 2 additions & 1 deletion src/kadmin/ktutil/ktutil_funcs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,8 @@ krb5_error_code ktutil_add(context, list, princ_str, fetch, kvno,
*last = lp;

cleanup:
krb5_kt_free_entry(context, entry);
krb5_free_keytab_entry_contents(context, entry);
free(entry);
zapfree(password.data, password.length);
krb5_free_data_contents(context, &salt);
krb5_free_data_contents(context, &params);
Expand Down
27 changes: 13 additions & 14 deletions src/kdc/do_tgs_req.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,17 +162,14 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
}

errcode = kdc_make_rstate(kdc_active_realm, &state);
if (errcode !=0) {
krb5_free_kdc_req(kdc_context, request);
return errcode;
}
if (errcode != 0)
goto cleanup;

/* Initialize audit state. */
errcode = kau_init_kdc_req(kdc_context, request, from, &au_state);
if (errcode) {
krb5_free_kdc_req(kdc_context, request);
return errcode;
}
if (errcode)
goto cleanup;

/* Seed the audit trail with the request ID and basic information. */
kau_tgs_req(kdc_context, TRUE, au_state);

Expand Down Expand Up @@ -733,11 +730,13 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
if (errcode)
emsg = krb5_get_error_message (kdc_context, errcode);

au_state->status = status;
if (!errcode)
au_state->reply = &reply;
kau_tgs_req(kdc_context, errcode ? FALSE : TRUE, au_state);
kau_free_kdc_req(au_state);
if (au_state != NULL) {
au_state->status = status;
if (!errcode)
au_state->reply = &reply;
kau_tgs_req(kdc_context, errcode ? FALSE : TRUE, au_state);
kau_free_kdc_req(au_state);
}

log_tgs_req(kdc_context, from, request, &reply, cprinc,
sprinc, altcprinc, authtime,
Expand All @@ -747,7 +746,7 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
emsg = NULL;
}

if (errcode) {
if (errcode && state != NULL) {
int got_err = 0;
if (status == 0) {
status = krb5_get_error_message (kdc_context, errcode);
Expand Down
26 changes: 11 additions & 15 deletions src/kprop/kpropd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ krb5_error_code
do_iprop()
{
kadm5_ret_t retval;
krb5_principal iprop_svc_principal;
krb5_principal iprop_svc_principal = NULL;
void *server_handle = NULL;
char *iprop_svc_princstr = NULL, *primary_svc_princstr = NULL;
unsigned int pollin, backoff_time;
Expand All @@ -652,35 +652,30 @@ do_iprop()
if (pollin == 0)
pollin = 10;

if (primary_svc_princstr == NULL) {
retval = kadm5_get_kiprop_host_srv_name(kpropd_context, realm,
&primary_svc_princstr);
if (retval) {
com_err(progname, retval,
_("%s: unable to get kiprop host based "
"service name for realm %s\n"),
progname, realm);
return retval;
}
retval = kadm5_get_kiprop_host_srv_name(kpropd_context, realm,
&primary_svc_princstr);
if (retval) {
com_err(progname, retval, _("%s: unable to get kiprop host based "
"service name for realm %s\n"),
progname, realm);
goto done;
}

retval = sn2princ_realm(kpropd_context, NULL, KIPROP_SVC_NAME, realm,
&iprop_svc_principal);
if (retval) {
com_err(progname, retval,
_("while trying to construct host service principal"));
return retval;
goto done;
}

retval = krb5_unparse_name(kpropd_context, iprop_svc_principal,
&iprop_svc_princstr);
if (retval) {
com_err(progname, retval,
_("while canonicalizing principal name"));
krb5_free_principal(kpropd_context, iprop_svc_principal);
return retval;
goto done;
}
krb5_free_principal(kpropd_context, iprop_svc_principal);

reinit:
/*
Expand Down Expand Up @@ -995,6 +990,7 @@ do_iprop()
done:
free(iprop_svc_princstr);
free(primary_svc_princstr);
krb5_free_principal(kpropd_context, iprop_svc_principal);
krb5_free_default_realm(kpropd_context, def_realm);
kadm5_destroy(server_handle);
krb5_db_fini(kpropd_context);
Expand Down
25 changes: 17 additions & 8 deletions src/kprop/kproplog.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,28 +398,36 @@ print_update(kdb_hlog_t *ulog, uint32_t entry, uint32_t ulogentries,
}
}

/* Return a read-only mmap of the ulog, or NULL on failure. Assumes fd is
* released on process exit. */
/* Return a read-only mmap of the ulog, or NULL on failure. */
static kdb_hlog_t *
map_ulog(const char *filename)
map_ulog(const char *filename, int *fd_out)
{
int fd;
struct stat st;
kdb_hlog_t *ulog;
kdb_hlog_t *ulog = MAP_FAILED;

*fd_out = -1;

fd = open(filename, O_RDONLY);
if (fd == -1)
return NULL;
if (fstat(fd, &st) < 0)
if (fstat(fd, &st) < 0) {
close(fd);
return NULL;
}
ulog = mmap(0, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
return (ulog == MAP_FAILED) ? NULL : ulog;
if (ulog == MAP_FAILED) {
close(fd);
return NULL;
}
*fd_out = fd;
return ulog;
}

int
main(int argc, char **argv)
{
int c;
int c, ulog_fd = -1;
unsigned int verbose = 0;
bool_t headeronly = FALSE, reset = FALSE;
uint32_t entry = 0;
Expand Down Expand Up @@ -480,7 +488,7 @@ main(int argc, char **argv)
goto done;
}

ulog = map_ulog(params.iprop_logfile);
ulog = map_ulog(params.iprop_logfile, &ulog_fd);
if (ulog == NULL) {
fprintf(stderr, _("Unable to map log file %s\n\n"),
params.iprop_logfile);
Expand Down Expand Up @@ -546,6 +554,7 @@ main(int argc, char **argv)
printf("\n");

done:
close(ulog_fd);
kadm5_free_config_params(context, &params);
krb5_free_context(context);
return 0;
Expand Down
6 changes: 2 additions & 4 deletions src/lib/gssapi/spnego/spnego_mech.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3485,11 +3485,9 @@ get_mech_set(OM_uint32 *minor_status, unsigned char **buff_in,

major_status = gss_add_oid_set_member(minor_status,
temp, &returned_mechSet);
if (major_status == GSS_S_COMPLETE) {
if (major_status == GSS_S_COMPLETE)
set_length += returned_mechSet->elements[i].length +2;
if (generic_gss_release_oid(minor_status, &temp))
map_errcode(minor_status);
}
generic_gss_release_oid(minor_status, &temp);
}

return (returned_mechSet);
Expand Down
11 changes: 8 additions & 3 deletions src/lib/kadm5/srv/pwqual_dict.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,16 @@ init_dict(dict_moddata dict, const char *dict_file)
close(fd);
return errno;
}
if ((dict->word_block = malloc(sb.st_size + 1)) == NULL)
dict->word_block = malloc(sb.st_size + 1);
if (dict->word_block == NULL) {
(void)close(fd);
return ENOMEM;
if (read(fd, dict->word_block, sb.st_size) != sb.st_size)
}
if (read(fd, dict->word_block, sb.st_size) != sb.st_size) {
(void)close(fd);
return errno;
(void) close(fd);
}
(void)close(fd);
dict->word_block[sb.st_size] = '\0';

p = dict->word_block;
Expand Down
5 changes: 4 additions & 1 deletion src/lib/kdb/kdb5.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1449,8 +1449,11 @@ krb5_db_setup_mkey_name(krb5_context context, const char *keyname,
if (asprintf(&fname, "%s%s%s", keyname, REALM_SEP_STRING, realm) < 0)
return ENOMEM;

if ((retval = krb5_parse_name(context, fname, principal)))
retval = krb5_parse_name(context, fname, principal);
if (retval) {
free(fname);
return retval;
}
if (fullname)
*fullname = fname;
else
Expand Down

0 comments on commit a06945b

Please sign in to comment.