ldap/servers/plugins/replication/cl5_api.c | 10 ++++-----
ldap/servers/plugins/replication/repl5_protocol.c | 2 -
ldap/servers/slapd/back-ldbm/import.c | 1
ldap/servers/slapd/daemon.c | 3 ++
ldap/servers/slapd/entry.c | 9 +++++---
ldap/servers/slapd/valueset.c | 23 ++++++++++++++++++++--
6 files changed, 37 insertions(+), 11 deletions(-)
New commits:
commit 39648c727d0b29e6f208cb9a698a54ab1c4be2fe
Author: Rich Megginson <rmeggins(a)redhat.com>
Date: Mon Jul 29 15:06:03 2013 -0600
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: <value count> <value size> <value> <value size>
<value> ..... */
-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))
{