From 0b67e0aeabbed65030f3651154c67f6d77531ae7 Mon Sep 17 00:00:00 2001 From: William Brown Date: Thu, 1 Dec 2016 14:02:02 +1000 Subject: [PATCH] Ticket 48894 - improve entrywsi delete Bug Description: The entrywsi cleanup code for maintaining the vs->sorted array had a crash in it due to improper handling of the valueset and the sorted array. Fix Description: This fixes the complex algorithm to be simpler, and resolves the segfault found. https://fedorahosted.org/389/ticket/48894 Author: wibrown Review by: ??? --- ldap/servers/slapd/attr.c | 2 +- ldap/servers/slapd/entrywsi.c | 4 +- ldap/servers/slapd/proto-slap.h | 2 +- ldap/servers/slapd/valueset.c | 125 +++++++++++++++++++++++++++------------- 4 files changed, 88 insertions(+), 45 deletions(-) diff --git a/ldap/servers/slapd/attr.c b/ldap/servers/slapd/attr.c index 170bdd3..76573ed 100644 --- a/ldap/servers/slapd/attr.c +++ b/ldap/servers/slapd/attr.c @@ -758,7 +758,7 @@ attr_purge_state_information(Slapi_Entry *entry, Slapi_Attr *attr, const CSN *cs { if(!valueset_isempty(&attr->a_deleted_values)) { - valueset_purge(&attr->a_deleted_values, csnUpTo); + valueset_purge(attr, &attr->a_deleted_values, csnUpTo); } } diff --git a/ldap/servers/slapd/entrywsi.c b/ldap/servers/slapd/entrywsi.c index da58cb2..94cf4ba 100644 --- a/ldap/servers/slapd/entrywsi.c +++ b/ldap/servers/slapd/entrywsi.c @@ -811,8 +811,8 @@ entry_delete_present_values_wsi_multi_valued(Slapi_Entry *e, const char *type, s * the current attr delete operation. These values need to be * preserved, all others can be removed, purging should o the job. */ - valueset_purge(&a->a_present_values, csn); - valueset_purge(&a->a_deleted_values, csn); + valueset_purge(a, &a->a_present_values, csn); + valueset_purge(a, &a->a_deleted_values, csn); if(attr_state==ATTRIBUTE_PRESENT && valueset_isempty(&a->a_present_values)) entry_present_attribute_to_deleted_attribute(e, a); } diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h index af728a6..2cdee6a 100644 --- a/ldap/servers/slapd/proto-slap.h +++ b/ldap/servers/slapd/proto-slap.h @@ -149,7 +149,7 @@ int valueset_isempty( const Slapi_ValueSet *vs); Slapi_Value *valueset_find(const Slapi_Attr *a, const Slapi_ValueSet *vs, const Slapi_Value *v); Slapi_Value *valueset_remove_value(const Slapi_Attr *a, Slapi_ValueSet *vs, const Slapi_Value *v); int valueset_remove_valuearray(Slapi_ValueSet *vs, const Slapi_Attr *a, Slapi_Value **valuestodelete, int flags, Slapi_Value ***va_out); -int valueset_purge(Slapi_ValueSet *vs, const CSN *csn); +int valueset_purge(const Slapi_Attr *a, Slapi_ValueSet *vs, const CSN *csn); Slapi_Value **valueset_get_valuearray(const Slapi_ValueSet *vs); size_t valueset_size(const Slapi_ValueSet *vs); void slapi_valueset_add_valuearray(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value **addvals); diff --git a/ldap/servers/slapd/valueset.c b/ldap/servers/slapd/valueset.c index ac2f1be..d8188fc 100644 --- a/ldap/servers/slapd/valueset.c +++ b/ldap/servers/slapd/valueset.c @@ -734,7 +734,7 @@ valueset_remove_value(const Slapi_Attr *a, Slapi_ValueSet *vs, const Slapi_Value * Remove any values older than the CSN from valueset. */ int -valueset_array_purge(Slapi_ValueSet *vs, const CSN *csn) +valueset_array_purge(const Slapi_Attr *a, Slapi_ValueSet *vs, const CSN *csn) { size_t i = 0; size_t j = 0; @@ -744,6 +744,7 @@ valueset_array_purge(Slapi_ValueSet *vs, const CSN *csn) /* Loop over all the values freeing the old ones. */ for(i = 0; i < vs->num; i++) { + /* If we have the sorted array, find the va array ref by it. */ if (vs->sorted) { j = vs->sorted[i]; } else { @@ -753,71 +754,112 @@ valueset_array_purge(Slapi_ValueSet *vs, const CSN *csn) if (vs->va[j]->v_csnset == NULL) { slapi_value_free(&vs->va[j]); vs->va[j] = NULL; + } else if (vs->va[j] != NULL) { + /* This value survived, we should count it. */ + numValues++; } } + /* Now compact the value/sorted list. */ - numValues = i; - nextValue = 0; - for(i = 0; isorted) { - j = vs->sorted[nextValue]; - } else { - j = nextValue; - } - while((nextValue < numValues) && (NULL == vs->va[j])) { - if (vs->sorted) { - j = vs->sorted[nextValue++]; - } else { - nextValue++; - } - } - if(nextValue < numValues) { - if(vs->sorted) { - vs->va[vs->sorted[i]] = vs->va[j]; - vs->sorted[i] = j; - } else { - vs->va[i] = vs->va[j]; + /* + * Because we want to preserve the sorted array, this is complicated. + * + * We have an array of values: + * [ b, a, c, NULL, e, NULL, NULL, d] + * And an array of indicies that are sorted. + * [ 1, 0, 2, 7, 4, 3, 5, 6 ] + * Were we to iterate over the sorted array, we get refs to the values in + * some order. + * The issue is now we must *remove* from both the values *and* the sorted. + * + * Previously, we just discarded this, because too hard. Now we try to keep + * it. The issue is that this is surprisingly hard to actually keep in + * sync. + * + * We can't just blindly move the values down: That breaks the sorted array + * and we would need to iterate over the sorted array multiple times to + * achieve this. + * + * It's actually going to be easier to just ditch the sorted, compact vs + * and then qsort the array. + */ + + j = 0; + while (nextValue < numValues && j < vs->num) + { + /* nextValue is what we are looking at now + * j tracks along the array getting next elements. + * + * [ b, a, c, NULL, e, NULL, NULL, d] + * ^nv ^j + * [ b, a, c, e, NULL, NULL, NULL, d] + * ^nv ^j + * [ b, a, c, e, NULL, NULL, NULL, d] + * ^nv ^j + * [ b, a, c, e, NULL, NULL, NULL, d] + * ^nv ^j + * [ b, a, c, e, NULL, NULL, NULL, d] + * ^nv ^j + * [ b, a, c, e, d, NULL, NULL, NULL] + * ^nv ^j + */ + if (vs->va[nextValue] == NULL) { + /* Advance j till we find something */ + while (vs->va[j] == NULL) { + j++; } - nextValue++; - } else { - break; + /* We have something! */ + vs->va[nextValue] = vs->va[j]; + vs->va[j] = NULL; } + nextValue++; } - - if(vs->sorted) { - vs->va[vs->sorted[i]] = NULL; - vs->sorted[i] = 0; - } else { - vs->va[i] = NULL; + /* Fix up the number of values */ + vs->num = numValues; + /* Should we re-alloc values to be smaller? */ + /* Other parts of DS are lazy. Lets clean our list */ + for (j = vs->num; j < vs->max; j++) { + vs->va[j] = NULL; } /* All the values were deleted, we can discard the whole array. */ - if(NULL == vs->va[0]) { + if(vs->num == 0) { if(vs->sorted) { slapi_ch_free ((void **)&vs->sorted); } slapi_ch_free ((void **)&vs->va); - vs->va= NULL; + vs->va = NULL; + vs->max = 0; + } else if (vs->sorted != NULL) { + /* We still have values! rebuild the sorted array */ + valueset_array_to_sorted(a, vs); + } + +#ifdef DEBUG + PR_ASSERT(vs->num == 0 || (vs->num > 0 && vs->va[0] != NULL)); + size_t index = 0; + for (; index < vs->num; index++) { + PR_ASSERT(vs->va[index] != NULL); } + for (; index < vs->max; index++) { + PR_ASSERT(vs->va[index] == NULL); + } +#endif /* return the number of remaining values */ - return i; + return numValues; } /* * Remove any values older than the CSN. */ int -valueset_purge(Slapi_ValueSet *vs, const CSN *csn) +valueset_purge(const Slapi_Attr *a, Slapi_ValueSet *vs, const CSN *csn) { int r= 0; if(!valuearray_isempty(vs->va)) { - r= valueset_array_purge(vs, csn); + r= valueset_array_purge(a, vs, csn); vs->num = r; - if (vs->va == NULL) { - /* va was freed */ - vs->max = 0; - } PR_ASSERT((vs->sorted == NULL) || (vs->num < VALUESET_ARRAY_SORT_THRESHOLD) || ((vs->num >= VALUESET_ARRAY_SORT_THRESHOLD) && (vs->sorted[0] < vs->num))); } return 0; @@ -1207,12 +1249,13 @@ valueset_add_string(const Slapi_Attr *a, Slapi_ValueSet *vs, const char *s, CSNT void valueset_set_valueset(Slapi_ValueSet *vs1, const Slapi_ValueSet *vs2) { - int i; + size_t i; if (vs1 && vs2) { int oldmax = vs1->max; /* pre-condition - vs1 empty - otherwise, existing data is overwritten */ PR_ASSERT(vs1->num == 0); + if (vs2->va) { /* need to copy valuearray */ if (vs2->max == 0) { -- 1.8.3.1