From 39648c727d0b29e6f208cb9a698a54ab1c4be2fe Mon Sep 17 00:00:00 2001 From: Rich Megginson Date: Jul 31 2013 00:29:45 +0000 Subject: Ticket #47455 - valgrind - value mem leaks, uninit mem usage https://fedorahosted.org/389/ticket/47455 Reviewed by: lkrispenz, nhosoi (Thanks!) Branch: master Fix Description: Make sure to use slapi_valueset_done to free valuesets to clean up everything. Make sure to free the values in valueset_set_valueset before setting them. The calculation of the size of the values to write to the changelog was wrong. It was finding the size of the length by doing sizeof(bv.bv_len) which is 8 bytes on 64-bit platforms, but using PRInt32 which is 4 bytes on all platforms to actually store the length. So each length calculation was using an extra 4 bytes, and writing this as uninitialized memory to the changelog db. In slapi_valueset_add_attr_valuearray_ext(), if an error is returned, and the PASSIN flag was used, set the vs->va[0] to NULL - this will allow the called to use both slapi_valueset_done(vs) and valuearray_free(&addvals) - addvals will still own all of the values. Cleaned up some other memory leaks and uninitialized memory uses. Platforms tested: RHEL6 x86_64 Flag Day: no Doc impact: no (cherry picked from commit 5633e125a462fa174186d1a8adba3ee00c43e3cd) --- diff --git a/ldap/servers/plugins/replication/cl5_api.c b/ldap/servers/plugins/replication/cl5_api.c index e6f2d0c..0a70d6b 100644 --- a/ldap/servers/plugins/replication/cl5_api.c +++ b/ldap/servers/plugins/replication/cl5_api.c @@ -335,7 +335,7 @@ static int _cl5GetModSize (LDAPMod *mod); static void _cl5ReadBerval (struct berval *bv, char** buff); static void _cl5WriteBerval (struct berval *bv, char** buff); static int _cl5ReadBervals (struct berval ***bv, char** buff, unsigned int size); -static int _cl5WriteBervals (struct berval **bv, char** buff, unsigned int *size); +static int _cl5WriteBervals (struct berval **bv, char** buff, u_int32_t *size); /* replay iteration */ #ifdef FOR_DEBUGGING @@ -2731,7 +2731,7 @@ static int _cl5GetModSize (LDAPMod *mod) { while (mod->mod_bvalues != NULL && mod->mod_bvalues[i] != NULL) { - size += mod->mod_bvalues[i]->bv_len + sizeof (mod->mod_bvalues[i]->bv_len); + size += (PRInt32)mod->mod_bvalues[i]->bv_len + sizeof (PRInt32); i++; } } @@ -2785,7 +2785,7 @@ static void _cl5WriteBerval (struct berval *bv, char** buff) PRUint32 net_length = 0; length = (PRUint32) bv->bv_len; - net_length = PR_htonl(length); + net_length = PR_htonl(length); memcpy(*buff, &net_length, sizeof (net_length)); *buff += sizeof (net_length); @@ -2835,7 +2835,7 @@ static int _cl5ReadBervals (struct berval ***bv, char** buff, unsigned int size) } /* data format: ..... */ -static int _cl5WriteBervals (struct berval **bv, char** buff, unsigned int *size) +static int _cl5WriteBervals (struct berval **bv, char** buff, u_int32_t *size) { PRInt32 count, net_count; char *pos; @@ -2847,7 +2847,7 @@ static int _cl5WriteBervals (struct berval **bv, char** buff, unsigned int *size *size = sizeof (count); for (count = 0; bv[count]; count ++) { - *size += sizeof (bv[count]->bv_len) + bv[count]->bv_len; + *size += (u_int32_t)(sizeof (PRInt32) + (PRInt32)bv[count]->bv_len); } /* allocate buffer */ diff --git a/ldap/servers/plugins/replication/repl5_protocol.c b/ldap/servers/plugins/replication/repl5_protocol.c index 8373ba6..34fe8a0 100644 --- a/ldap/servers/plugins/replication/repl5_protocol.c +++ b/ldap/servers/plugins/replication/repl5_protocol.c @@ -91,7 +91,7 @@ Repl_Protocol * prot_new(Repl_Agmt *agmt, int protocol_state) { Slapi_DN *replarea_sdn = NULL; - Repl_Protocol *rp = (Repl_Protocol *)slapi_ch_malloc(sizeof(Repl_Protocol)); + Repl_Protocol *rp = (Repl_Protocol *)slapi_ch_calloc(1, sizeof(Repl_Protocol)); rp->prp_incremental = rp->prp_total = rp->prp_active_protocol = NULL; if (protocol_state == STATE_PERFORMING_TOTAL_UPDATE) diff --git a/ldap/servers/slapd/back-ldbm/import.c b/ldap/servers/slapd/back-ldbm/import.c index c7b52d9..aa6c707 100644 --- a/ldap/servers/slapd/back-ldbm/import.c +++ b/ldap/servers/slapd/back-ldbm/import.c @@ -1558,6 +1558,7 @@ error: instance_set_not_busy(job->inst); import_free_job(job); + FREE(job); if (producer) FREE(producer); diff --git a/ldap/servers/slapd/daemon.c b/ldap/servers/slapd/daemon.c index dae5f84..c4b0865 100644 --- a/ldap/servers/slapd/daemon.c +++ b/ldap/servers/slapd/daemon.c @@ -1210,6 +1210,7 @@ void slapd_daemon( daemon_ports_t *ports ) } } + slapi_ch_free((void **)&listener_idxs); /* We get here when the server is shutting down */ /* Do what we have to do before death */ @@ -1822,6 +1823,8 @@ compute_idletimeout( slapdFrontendConfig_t *fecfg, Connection *conn ) } slapi_sdn_free(&anon_sdn); + } else { + idletimeout = fecfg->idletimeout; } slapi_ch_free_string(&anon_dn); } else if ( conn->c_isroot ) { diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c index 3c1c29b..4b119aa 100644 --- a/ldap/servers/slapd/entry.c +++ b/ldap/servers/slapd/entry.c @@ -1106,11 +1106,12 @@ str2entry_dupcheck( const char *rawdn, char *s, int flags, int read_stateinfo ) { /* Failure adding to value tree */ LDAPDebug( LDAP_DEBUG_ANY, "str2entry_dupcheck: unexpected failure %d adding value\n", rc, 0, 0 ); + slapi_value_free(&value); /* value not consumed - free it */ slapi_entry_free( e ); e = NULL; goto free_and_return; } - slapi_value_free(&value); + slapi_value_free(&value); /* if rc is error, value was not consumed - free it */ } /* All done with parsing. Now create the entry. */ @@ -1179,6 +1180,7 @@ str2entry_dupcheck( const char *rawdn, char *s, int flags, int read_stateinfo ) sa->sa_present_values.num, SLAPI_VALUE_FLAG_PASSIN, NULL); sa->sa_present_values.num= 0; /* The values have been consumed */ + slapi_ch_free((void **)&sa->sa_present_values.va); slapi_valueset_add_attr_valuearray_ext( *a, &(*a)->a_deleted_values, @@ -1186,6 +1188,7 @@ str2entry_dupcheck( const char *rawdn, char *s, int flags, int read_stateinfo ) sa->sa_deleted_values.num, SLAPI_VALUE_FLAG_PASSIN, NULL); sa->sa_deleted_values.num= 0; /* The values have been consumed */ + slapi_ch_free((void **)&sa->sa_deleted_values.va); if(sa->sa_attributedeletioncsn!=NULL) { attr_set_deletion_csn(*a,sa->sa_attributedeletioncsn); @@ -1231,8 +1234,8 @@ free_and_return: for ( i = 0; i < nattrs; i++ ) { slapi_ch_free((void **) &(attrs[ i ].sa_type)); - slapi_ch_free((void **) &(attrs[ i ].sa_present_values.va)); - slapi_ch_free((void **) &(attrs[ i ].sa_deleted_values.va)); + slapi_valueset_done(&attrs[ i ].sa_present_values); + slapi_valueset_done(&attrs[ i ].sa_deleted_values); attr_done( &attrs[ i ].sa_attr ); } if (tree_attr_checking) diff --git a/ldap/servers/slapd/valueset.c b/ldap/servers/slapd/valueset.c index de576ec..172882f 100644 --- a/ldap/servers/slapd/valueset.c +++ b/ldap/servers/slapd/valueset.c @@ -1044,6 +1044,15 @@ valueset_insert_value_to_sorted(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_V } +/* + * If this function returns an error, it is safe to do both + * slapi_valueset_done(vs); + * and + * valuearray_free(&addvals); + * if there is an error and the PASSIN flag is used, the addvals array will own all of the values + * vs will own none of the values - so you should do both slapi_valueset_done(vs) and valuearray_free(&addvals) + * to clean up + */ int slapi_valueset_add_attr_valuearray_ext(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value **addvals, int naddvals, unsigned long flags, int *dup_index) @@ -1116,8 +1125,12 @@ slapi_valueset_add_attr_valuearray_ext(const Slapi_Attr *a, Slapi_ValueSet *vs, if (dup < 0 ) { rc = LDAP_TYPE_OR_VALUE_EXISTS; if (dup_index) *dup_index = i; - if ( !passin) + if (passin) { + /* we have to NULL out the first value so valuearray_free won't delete values in addvals */ + (vs->va)[0] = NULL; + } else { slapi_value_free(&(vs->va)[vs->num]); + } break; } } else { @@ -1345,11 +1358,17 @@ valueset_replace_valuearray_ext(Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value * PR_ASSERT((vs->sorted == NULL) || (vs->num == 0) || ((vs->sorted[0] >= 0) && (vs->sorted[0] < vs->num))); } else { /* verify the given values are not duplicated. */ + unsigned long flags = SLAPI_VALUE_FLAG_PASSIN|SLAPI_VALUE_FLAG_DUPCHECK; Slapi_ValueSet *vs_new = slapi_valueset_new(); - rc = slapi_valueset_add_attr_valuearray_ext (a, vs_new, valstoreplace, vals_count, 0, NULL); + rc = slapi_valueset_add_attr_valuearray_ext (a, vs_new, valstoreplace, vals_count, flags, NULL); if ( rc == LDAP_SUCCESS ) { + /* used passin, so vs_new owns all of the Slapi_Value* in valstoreplace + * so tell valuearray_free_ext to start at index vals_count, which is + * NULL, then just free valstoreplace + */ + valuearray_free_ext(&valstoreplace, vals_count); /* values look good - replace the values in the attribute */ if(!valuearray_isempty(vs->va)) {