Bug description: tombstone_to_glue resolves parents recursively if they are also a tombstone entry, which may change the DN of the target entry, but the DN change was not adjusted.
Fix description: Added a new argument newparentdn to tombstone_to_glue_resolve_parent to return the new parent DN if it has been updated. If a new parent DN is returned, set it to the target DN.
This patch also adds a cache debugging function: check_entry_cache, which is disabled by default (in #ifdef CACHE_DEBUG)
Note: this bug is a left over of ticket #47750.
git patch file (master) 0001-Ticket-47834-Tombstone_to_glue-if-parents-are-also-c.patch
{{{ 229 slapi_log_error (slapi_log_urp, repl_plugin_name, 248 slapi_log_error (/slapi_log_urp/SLAPI_LOG_FATAL, repl_plugin_name, 230 249 "%s: Resurrected tombstone %s to glue reason '%s'\n", sessionid, addingdn, reason); 231 250 } 232 251 else if (LDAP_ALREADY_EXISTS == op_result) 233 252 { 234 slapi_log_error(slapi_log_urp, repl_plugin_name, 253 slapi_log_error(/slapi_log_urp/SLAPI_LOG_FATAL, repl_plugin_name, }}} If the log level changes to FATAL, is this going to log a lot of stuff that the customer will be concerned about?
{{{
126 if (slapi_pblock_get(pb, SLAPI_CONN_ID, &conn_id) < 0) { 127 conn_id = 0; /* connection is NULL */ 128 }
}}} Does this mean it is an internal operation? Do we use -1 for conn_id for internal ops?
{{{ } 1979 *e_in_cache = 0; 1942 1980 if (orig_ec_in_cache) { }}} Indentation is off.
2485 slapi_sdn_get_rdn_ext(const Slapi_DN *sdn, Slapi_RDN *rdn, int is_tombstone) 2486 { 2487 slapi_rdn_set_dn_ext(rdn, slapi_sdn_get_dn(sdn), is_tombstone); 2488 }
}}} Is slapi_sdn_get_x calling slapi_rdn_set_x? Did you mean slapi_rdn_get_dn_ext?
Thanks, Rich! I've fixed all of your comments (in comment 2) except the last one. It looks the function is not being used now. :p I wanted to have a function to generate Slapi_RDN from Slapi_DN supporting the tombstone DN format. To implement it, I borrowed slapi_rdn_set_dn_ext that generates Slapi_RDN from string DN. But since I don't see slapi_sdn_get_rdn_ext any other places, it's not needed any more. I'm going to just delete it...
git patch file (389-ds-base-1.3.2) -- additional fixes following the comments by Rich (comment #2) 0001-Ticket-47834-Tombstone_to_glue-if-parents-are-also-c.2.patch
Reviewed by Rich (Thank you!!)
Pushed to master: d897574..708a56b master -> master commit 708a56b
Pushed to 389-ds-base-1.3.2: 60d8bf9..513788c 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 513788c
Thank you, Thierry. Reopening...
git patch file (master) -- undo the change made in op_shared_add (add.c), which broke the CI testcase 47808. 0001-Ticket-47834-Tombstone_to_glue-if-parents-are-also-c.3.patch
Thierry, thanks for pointing it out!
Pushed to master: 0bfe1ee..6d38125 master -> master commit 6d38125
Pushed to 389-ds-base-1.3.2: 107722f..88aa59f 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 88aa59f
I'm going to run the conflict test this weekend, and if it turns out it's successful, I'm closing this ticket...
Valgrind reported invalid read/write. Regressed. ==12273== Invalid read of size 4 ==12273== at 0x102E75C8: ldbm_back_delete (ldbm_delete.c:1311) ==12273== by 0x4C6A091: op_shared_delete (delete.c:364) ==12273== by 0x4C69862: do_delete (delete.c:128) ==12273== by 0x120C08: connection_dispatch_operation (connection.c:650) ==12273== by 0x122CBF: connection_threadmain (connection.c:2534) ==12273== by 0x68F9C85: _pt_root (ptthread.c:204) ==12273== by 0x6F36D14: start_thread (pthread_create.c:308) ==12273== by 0x745453C: clone (clone.S:114) ==12273== Address 0x234e5fb8 is 24 bytes inside a block of size 96 free'd ==12273== at 0x4A077E6: free (vg_replace_malloc.c:446) ==12273== by 0x4C6478F: slapi_ch_free (ch_malloc.c:363) ==12273== by 0x102983A5: backentry_free (backentry.c:62) ==12273== by 0x1029A9A9: entrycache_return (cache.c:1160) ==12273== by 0x1029A84C: cache_return (cache.c:1133) ==12273== by 0x103014D3: ldbm_back_next_search_entry_ext (ldbm_search.c:1496) ==12273== by 0x103010D7: ldbm_back_next_search_entry (ldbm_search.c:1390) ==12273== by 0x4CBB41A: iterate (opshared.c:1284) ==12273== by 0x4CBBD57: send_results_ext (opshared.c:1692) ==12273== by 0x4CBAA6A: op_shared_search (opshared.c:856) ==12273== by 0x13D9E5: do_search (search.c:378) ==12273== by 0x120D22: connection_dispatch_operation (connection.c:684) ==12273== by 0x122CBF: connection_threadmain (connection.c:2534) ==12273== by 0x68F9C85: _pt_root (ptthread.c:204) ==12273== by 0x6F36D14: start_thread (pthread_create.c:308) ==12273== by 0x745453C: clone (clone.S:114)
Memory leak in add, as well. ==12214== 66,957,435 (887,136 direct, 66,070,299 indirect) bytes in 9,241 blocks are definitely lost in loss record 2,388 of 2,388 ==12214== at 0x4A06B6F: calloc (vg_replace_malloc.c:593) ==12214== by 0x4C6449F: slapi_ch_calloc (ch_malloc.c:243) ==12214== by 0x10298428: backentry_init (backentry.c:92) ==12214== by 0x102D56FE: ldbm_back_add (ldbm_add.c:627) ==12214== by 0x4C591BD: op_shared_add (add.c:735) ==12214== by 0x4C580E3: do_add (add.c:258) ==12214== by 0x120BE6: connection_dispatch_operation (connection.c:645) ==12214== by 0x122CBF: connection_threadmain (connection.c:2534) ==12214== by 0x68F9C85: _pt_root (ptthread.c:204) ==12214== by 0x6F36D14: start_thread (pthread_create.c:308) ==12214== by 0x745453C: clone (clone.S:114)
These lines
if (slapi_pblock_get(pb, SLAPI_CONN_ID, &conn_id) < 0) { conn_id = 0; /* connection is NULL */ }
cause an error message for every internal op, we either should change this or do not log as ANY in pblock.c
git patch file (master) -- additional patch 0001-Ticket-47834-Tombstone_to_glue-if-parents-are-also-c.4.patch
Looks good, ack. Just curious, did you run this under valgrind and jenkins?
Replying to [comment:19 mreynolds]:
Looks good, ack. Just curious, did you run this under valgrind and jenkins? Thank you, Mark! I ran both. Valgrind result looks good. Regarding jenkins, compile is good.
Reviewed by Mark (Thank you!!)
Pushed to master: cb7ccc2..6333936 master -> master commit 6333936
Pushed to 389-ds-base-1.3.2: 9398383..d9c8b1f 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit d9c8b1f
The patch(es) broke CItest 47815. Reopening...
git patch file (master) -- fixing ldbm_back_add that broke the CI test 47815. 0001-Ticket-47834-Tombstone_to_glue-if-parents-are-also-c.5.patch
Pushed to master: 4f11606..7db4fa9 master -> master commit 7db4fa9
Pushed to 389-ds-base-1.3.3: 8c82941..78fdd61 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 78fdd61
Pushed to 389-ds-base-1.3.2: 5b6d60e..6eec63f 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 6eec63f
Hello Noriko,
About the original fix I have a question regarding the ldbm_back_modify function in the retry section:
{{{ @@ -515,14 +516,22 @@ ldbm_back_modify( Slapi_PBlock pb ) slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods); ldap_mods_free(mods, 1); slapi_pblock_set(pb, SLAPI_MODIFY_MODS, copy_mods(mods_original)); - / ec is not really added to the cache until cache_replace, so we - don't have to worry about the cache here / - backentry_free(&ec); - slapi_pblock_set( pb, SLAPI_MODIFY_EXISTING_ENTRY, original_entry->ep_entry ); - ec = original_entry; - if ( (original_entry = backentry_dup( ec )) == NULL ) { - ldap_result_code= LDAP_OPERATIONS_ERROR; - goto error_return; + + / reset ec set cache in id2entry_add_ext / + if (ec) { + / must duplicate ec before returning it to cache, + * which could free the entry. */ + if ( (tmpentry = backentry_dup( ec )) == NULL ) { + ldap_result_code= LDAP_OPERATIONS_ERROR; + goto error_return; + } + if (cache_is_in_cache(&inst->inst_cache, ec)) { + CACHE_REMOVE(&inst->inst_cache, ec); + } + CACHE_RETURN(&inst->inst_cache, &ec); + ec = original_entry; + original_entry = tmpentry; + tmpentry = NULL; }
}}}
In this section, SLAPI_MODIFY_EXISTING_ENTRY is no longer set. Was it intentional. I wonder if it could be related to issue like https://fedorahosted.org/389/ticket/47889.
Note: his setting still exists in for ldbm_back_modrdn (SLAPI_MODRDN_EXISTING_ENTRY)
A good catch! Thank you soooooo much, Thierry! I did not intend to removed the line... Please resume it. {{{ CACHE_RETURN(&inst->inst_cache, &ec); slapi_pblock_set( pb, SLAPI_MODIFY_EXISTING_ENTRY, original_entry->ep_entry );
ec = original_entry; }}}
P.S., I made the ticket 47889 to point the same bz1080186.
Fix cherry=pick on 1.3.1 0001-Ticket-47834-Fix-cherry-pick-for-1.3.1.patch
Metadata Update from @tbordaz: - Issue assigned to nhosoi - Issue set to the milestone: 1.3.2.24
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1165
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.