From 2d40406b245a51e0cf9c530c428a6999f60d804d Mon Sep 17 00:00:00 2001 From: Noriko Hosoi Date: Fri, 17 Feb 2012 16:28:48 -0800 Subject: [PATCH] Trac Ticket #298 - crash when replicating orphaned tombstone entry https://fedorahosted.org/389/ticket/298 Fix description: 1. The cause of the crash was freeing a to-be-added entry in tombstone_to_glue although the entry is consumed in slapi_add_entry_internal_set_pb/slapi_add_internal_pb. This patch removes the redundant slapi_entry_free from tombstone_to_glue. 2. Introducing is_suffix_dn_ext to pass is_tombstone flag for getting the proper parent sdn of a tombstoned entry. 3. Logic handling ancestor tombstone was broken. In _entryrdn_insert_key, if _entryrdn_get_tombstone_elem finds a child node, it was checking if the node is a tombstone or not immediately. It should have been done in the next loop. 4. Reducing repeated "WARNING: bad entry: ID ##" messages. --- ldap/servers/plugins/replication/urp.c | 13 +++++-- ldap/servers/plugins/replication/urp.h | 1 + ldap/servers/plugins/replication/urp_tombstone.c | 3 +- ldap/servers/slapd/add.c | 13 ++++--- ldap/servers/slapd/back-ldbm/import-threads.c | 4 +- ldap/servers/slapd/back-ldbm/import.c | 11 +++--- ldap/servers/slapd/back-ldbm/import.h | 11 ++++-- ldap/servers/slapd/back-ldbm/ldbm_add.c | 3 +- ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c | 43 ++++++++-------------- ldap/servers/slapd/back-ldbm/ldif2ldbm.c | 4 ++- 10 files changed, 53 insertions(+), 53 deletions(-) diff --git a/ldap/servers/plugins/replication/urp.c b/ldap/servers/plugins/replication/urp.c index 1328996..7000b0e 100644 --- a/ldap/servers/plugins/replication/urp.c +++ b/ldap/servers/plugins/replication/urp.c @@ -726,7 +726,7 @@ urp_fixup_add_entry (Slapi_Entry *e, const char *target_uniqueid, const char *pa */ slapi_add_entry_internal_set_pb ( newpb, - e, + e, /* entry will be consumed */ NULL, /*Controls*/ repl_get_plugin_identity ( PLUGIN_MULTIMASTER_REPLICATION ), OP_FLAG_REPLICATED | OP_FLAG_REPL_FIXUP | opflags); @@ -1260,14 +1260,15 @@ is_suffix_entry ( Slapi_PBlock *pb, Slapi_Entry *entry, Slapi_DN **parentdn ) } int -is_suffix_dn ( Slapi_PBlock *pb, const Slapi_DN *dn, Slapi_DN **parentdn ) +is_suffix_dn_ext ( Slapi_PBlock *pb, const Slapi_DN *dn, Slapi_DN **parentdn, + int is_tombstone ) { Slapi_Backend *backend; int rc; *parentdn = slapi_sdn_new(); slapi_pblock_get( pb, SLAPI_BACKEND, &backend ); - slapi_sdn_get_backend_parent (dn, *parentdn, backend); + slapi_sdn_get_backend_parent_ext (dn, *parentdn, backend, is_tombstone); /* A suffix entry doesn't have parent dn */ rc = slapi_sdn_isempty (*parentdn) ? 1 : 0; @@ -1275,6 +1276,12 @@ is_suffix_dn ( Slapi_PBlock *pb, const Slapi_DN *dn, Slapi_DN **parentdn ) return rc; } +int +is_suffix_dn ( Slapi_PBlock *pb, const Slapi_DN *dn, Slapi_DN **parentdn ) +{ + return is_suffix_dn_ext ( pb, dn, parentdn, 0 ); +} + static int mod_namingconflict_attr (const char *uniqueid, const Slapi_DN *entrysdn, const Slapi_DN *conflictsdn, CSN *opcsn) diff --git a/ldap/servers/plugins/replication/urp.h b/ldap/servers/plugins/replication/urp.h index 2ca7ad2..ec7c94d 100644 --- a/ldap/servers/plugins/replication/urp.h +++ b/ldap/servers/plugins/replication/urp.h @@ -63,6 +63,7 @@ int urp_fixup_rename_entry (Slapi_Entry *entry, const char *newrdn, int opflags) int urp_fixup_modify_entry (const char *uniqueid, const Slapi_DN *sdn, CSN *opcsn, Slapi_Mods *smods, int opflags); int is_suffix_dn (Slapi_PBlock *pb, const Slapi_DN *dn, Slapi_DN **parenddn); +int is_suffix_dn_ext (Slapi_PBlock *pb, const Slapi_DN *dn, Slapi_DN **parenddn, int is_tombstone); /* * urp_glue.c diff --git a/ldap/servers/plugins/replication/urp_tombstone.c b/ldap/servers/plugins/replication/urp_tombstone.c index 6dd9a2a..a61f1e5 100644 --- a/ldap/servers/plugins/replication/urp_tombstone.c +++ b/ldap/servers/plugins/replication/urp_tombstone.c @@ -169,7 +169,7 @@ tombstone_to_glue ( /* JCM - This DN calculation is odd. It could resolve to NULL * which won't help us identify the correct backend to search. */ - is_suffix_dn (pb, tombstonedn, &parentdn); + is_suffix_dn_ext (pb, tombstonedn, &parentdn, 1 /* is_tombstone */); parentuniqueid= slapi_entry_attr_get_charptr (tombstoneentry, SLAPI_ATTR_VALUE_PARENT_UNIQUEID); /* Allocated */ tombstone_to_glue_resolve_parent (pb, sessionid, parentdn, parentuniqueid, opcsn); @@ -201,7 +201,6 @@ tombstone_to_glue ( "%s: Can't resurrect tombstone %s to glue reason '%s', error=%d\n", sessionid, addingdn, reason, op_result); } - slapi_entry_free (addingentry); return op_result; } diff --git a/ldap/servers/slapd/add.c b/ldap/servers/slapd/add.c index 206dfc4..53fd3fc 100644 --- a/ldap/servers/slapd/add.c +++ b/ldap/servers/slapd/add.c @@ -377,7 +377,7 @@ int slapi_add_internal_set_pb (Slapi_PBlock *pb, const char *dn, LDAPMod **attrs return rc; } - +/* Note: Passed entry e is going to be consumed. */ /* Initialize a pblock for a call to slapi_add_internal_pb() */ void slapi_add_entry_internal_set_pb (Slapi_PBlock *pb, Slapi_Entry *e, LDAPControl **controls, Slapi_ComponentId *plugin_identity, int operation_flags) @@ -415,12 +415,12 @@ static int add_internal_pb (Slapi_PBlock *pb) { opresult = LDAP_PARAM_ERROR; slapi_pblock_set(pb, SLAPI_PLUGIN_INTOP_RESULT, &opresult); - return 0; + return 0; } slapi_pblock_get(pb, SLAPI_OPERATION, &op); - op->o_handler_data = &opresult; - op->o_result_handler = internal_getresult_callback; + op->o_handler_data = &opresult; + op->o_result_handler = internal_getresult_callback; slapi_pblock_set(pb, SLAPI_REQCONTROLS, controls); @@ -431,9 +431,9 @@ static int add_internal_pb (Slapi_PBlock *pb) set_config_params (pb); /* perform the add operation */ - op_shared_add (pb); + op_shared_add (pb); - slapi_pblock_set(pb, SLAPI_PLUGIN_INTOP_RESULT, &opresult); + slapi_pblock_set(pb, SLAPI_PLUGIN_INTOP_RESULT, &opresult); return 0; } @@ -731,6 +731,7 @@ done: slapi_entry_free(pse); slapi_ch_free((void **)&operation->o_params.p.p_add.parentuniqueid); slapi_entry_free(e); + slapi_pblock_set(pb, SLAPI_ADD_ENTRY, NULL); valuearray_free(&unhashed_password_vals); slapi_ch_free((void**)&pwdtype); slapi_ch_free_string(&proxydn); diff --git a/ldap/servers/slapd/back-ldbm/import-threads.c b/ldap/servers/slapd/back-ldbm/import-threads.c index 458ead0..4c1129d 100644 --- a/ldap/servers/slapd/back-ldbm/import-threads.c +++ b/ldap/servers/slapd/back-ldbm/import-threads.c @@ -2039,7 +2039,7 @@ foreman_do_entrydn(ImportJob *job, FifoItem *fi) fi->line, fi->filename); idl_free(IDL); /* skip this one */ - fi->bad = 1; + fi->bad = FIFOITEM_BAD; job->skipped++; return -1; /* skip to next entry */ } @@ -2214,7 +2214,7 @@ import_foreman(void *param) slapi_entry_get_dn(fi->entry->ep_entry), fi->line, fi->filename); /* skip this one */ - fi->bad = 1; + fi->bad = FIFOITEM_BAD; job->skipped++; goto cont; /* below */ } diff --git a/ldap/servers/slapd/back-ldbm/import.c b/ldap/servers/slapd/back-ldbm/import.c index 7f03eb3..4958dc0 100644 --- a/ldap/servers/slapd/back-ldbm/import.c +++ b/ldap/servers/slapd/back-ldbm/import.c @@ -106,14 +106,13 @@ FifoItem *import_fifo_fetch(ImportJob *job, ID id, int worker) } else { return NULL; } - if (fi->entry) { + if (fi->entry && fi->bad && (FIFOITEM_BAD == fi->bad)) { + fi->bad = FIFOITEM_BAD_PRINTED; if (worker) { - if (fi->bad) { - import_log_notice(job, "WARNING: bad entry: ID %d", id); - return NULL; - } - PR_ASSERT(fi->entry->ep_refcnt > 0); + import_log_notice(job, "WARNING: bad entry: ID %d", id); + return NULL; } + PR_ASSERT(fi->entry->ep_refcnt > 0); } return fi; } diff --git a/ldap/servers/slapd/back-ldbm/import.h b/ldap/servers/slapd/back-ldbm/import.h index bef5fa0..c9a3f8a 100644 --- a/ldap/servers/slapd/back-ldbm/import.h +++ b/ldap/servers/slapd/back-ldbm/import.h @@ -83,12 +83,15 @@ struct _import_index_info /* item on the entry FIFO */ typedef struct { struct backentry *entry; - char *filename; /* or NULL */ - int line; /* filename/line are used to report errors */ - int bad; /* foreman did not like the entry */ - size_t esize; /* entry size */ + char *filename; /* or NULL */ + int line; /* filename/line are used to report errors */ + int bad; /* foreman did not like the entry */ + size_t esize; /* entry size */ } FifoItem; +#define FIFOITEM_BAD 1 +#define FIFOITEM_BAD_PRINTED 2 + typedef struct { FifoItem *item; size_t size; /* Queue size in entries (computed in import_fifo_init). */ diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index 748ef26..d29ff5f 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -176,8 +176,7 @@ ldbm_back_add( Slapi_PBlock *pb ) } - if (!is_tombstone_operation && !is_resurect_operation) - { + if (!is_tombstone_operation) { rc= slapi_setbit_int(rc,SLAPI_RTN_BIT_FETCH_EXISTING_DN_ENTRY); } rc= slapi_setbit_int(rc,SLAPI_RTN_BIT_FETCH_EXISTING_UNIQUEID_ENTRY); diff --git a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c index a0aaa80..2a7b1e4 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c @@ -1584,8 +1584,7 @@ retry_get: slapi_ch_free(&ptr); } bail: - slapi_log_error(SLAPI_LOG_TRACE, ENTRYRDN_TAG, - "<-- _entryrdn_get_elem\n"); + slapi_log_error(SLAPI_LOG_TRACE, ENTRYRDN_TAG, "<-- _entryrdn_get_elem\n"); return rc; } @@ -2231,6 +2230,12 @@ _entryrdn_insert_key(backend *be, /* Check the direct child in the RDN array, first */ rdnidx = slapi_rdn_get_prev_ext(srdn, rdnidx, &childnrdn, FLAG_ALL_NRDNS); + if ((rdnidx < 0) || (NULL == childnrdn)) { + slapi_log_error(SLAPI_LOG_FATAL, ENTRYRDN_TAG, + "_entryrdn_insert_key: RDN list \"%s\" is broken: " + "idx(%d)\n", slapi_rdn_get_rdn(srdn), rdnidx); + goto bail; + } /* Generate a key for child tree */ /* E.g., C1 */ keybuf = slapi_ch_smprintf("%c%u", RDN_INDEX_CHILD, workid); @@ -2298,7 +2303,7 @@ _entryrdn_insert_key(backend *be, */ rc = _entryrdn_get_tombstone_elem(cursor, tmpsrdn, &key, childnrdn, &tmpelem); - if (rc || (NULL == tmpelem)) { + if (rc) { char *dn = NULL; slapi_rdn_get_dn(tmpsrdn, &dn); if (DB_NOTFOUND == rc) { @@ -2312,24 +2317,16 @@ _entryrdn_insert_key(backend *be, } slapi_ch_free_string(&dn); goto bail; - } else { - int tmpidx = slapi_rdn_get_prev_ext(srdn, rdnidx, - &childnrdn, FLAG_ALL_NRDNS); - if (childnrdn && - (0 == strncasecmp(childnrdn, SLAPI_ATTR_UNIQUEID, - sizeof(SLAPI_ATTR_UNIQUEID) - 1))) { - rdnidx = tmpidx; - } } /* Node is a tombstone. */ - slapi_ch_free((void **)&elem); - elem = tmpelem; - currid = id_stored_to_internal(elem->rdn_elem_id); - nrdn = childnrdn; - workid = currid; - slapi_ch_free((void **)&parentelem); - parentelem = elem; - elem = NULL; + if (tmpelem) { + currid = id_stored_to_internal(tmpelem->rdn_elem_id); + nrdn = childnrdn; + workid = currid; + slapi_ch_free((void **)&parentelem); + parentelem = tmpelem; + slapi_ch_free((void **)&elem); + } } } else { char *dn = NULL; @@ -2823,14 +2820,6 @@ _entryrdn_index_read(backend *be, slapi_rdn_free(&tmpsrdn); } goto bail; - } else { - int tmpidx = slapi_rdn_get_prev_ext(srdn, rdnidx, - &childnrdn, FLAG_ALL_NRDNS); - if (childnrdn && - (0 == strncasecmp(childnrdn, SLAPI_ATTR_UNIQUEID, - sizeof(SLAPI_ATTR_UNIQUEID) - 1))) { - rdnidx = tmpidx; - } } } else { slapi_ch_free((void **)&tmpelem); diff --git a/ldap/servers/slapd/back-ldbm/ldif2ldbm.c b/ldap/servers/slapd/back-ldbm/ldif2ldbm.c index 1e28070..77f7cc9 100644 --- a/ldap/servers/slapd/back-ldbm/ldif2ldbm.c +++ b/ldap/servers/slapd/back-ldbm/ldif2ldbm.c @@ -256,7 +256,9 @@ int add_op_attrs(Slapi_PBlock *pb, struct ldbminfo *li, struct backentry *ep, * If so, need to get the grandparent of the leaf. */ if (slapi_entry_flag_is_set(ep->ep_entry, - SLAPI_ENTRY_FLAG_TOMBSTONE)) { + SLAPI_ENTRY_FLAG_TOMBSTONE) && + (0 == strncasecmp(pdn, SLAPI_ATTR_UNIQUEID, + sizeof(SLAPI_ATTR_UNIQUEID) - 1))) { char *ppdn = slapi_dn_parent(pdn); slapi_ch_free_string(&pdn); if (NULL == ppdn) { -- 1.7.6.5