#47834 Tombstone_to_glue: if parents are also converted to glue, the target entry's DN must be adjusted.
Closed: Fixed None Opened 5 years ago by nhosoi.

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.


{{{
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

Noriko, This fix triggers or reveal a crash condition with the test case ticket47808_test.py. It crashes systematically in a double free: {{{ (gdb) where #0 0x0000003bee035935 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 #1 0x0000003bee0370e8 in __GI_abort () at abort.c:91 #2 0x0000003bee074e8b in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x3bee178928 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:198 #3 0x0000003bee07c00e in malloc_printerr (ptr=0x7ffcfc001200, str=0x3bee1767b5 "free(): invalid pointer", action=3) at malloc.c:5027 #4 _int_free (av=0x3bee3b0720, p=0x7ffcfc0011f0, have_lock=0) at malloc.c:3948 #5 0x00007ffd49bf35c0 in slapi_ch_free (ptr=0x7ffcfc001460) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/ch_malloc.c:363 #6 0x00007ffd49beaa57 in attr_done (a=0x7ffcfc001460) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/attr.c:463 #7 0x00007ffd49beaa1b in slapi_attr_free (ppa=0x7ffd2eff87d0) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/attr.c:451 #8 0x00007ffd49bebd2c in attrlist_free (alist=0x7ffcfc001210) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/attrlist.c:53 #9 0x00007ffd49c0b0ab in slapi_entry_free (e=0x7ffcfc000f10) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/entry.c:2050 #10 0x00007ffd49be8245 in op_shared_add (pb=0x7ffd2effcae0) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/add.c:798 #11 0x00007ffd49be6f14 in do_add (pb=0x7ffd2effcae0) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/add.c:258 #12 0x0000000000415d1c in connection_dispatch_operation (conn=0x7ffd3afae560, op=0x299c3c0, pb=0x7ffd2effcae0) at ../workspaces/389-master-branch/ds/ldap/servers/slapd/connection.c:645 #13 0x0000000000417d4d in connection_threadmain () at ../workspaces/389-master-branch/ds/ldap/servers/slapd/connection.c:2534 #14 0x0000003c53028b46 in _pt_root (arg=0x2959b30) at ../../../nspr/pr/src/pthreads/ptthread.c:204 #15 0x0000003bee407d14 in start_thread (arg=0x7ffd2effd700) at pthread_create.c:309 #16 0x0000003bee0f168d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115 }}} thanks thierry

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

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

Reviewed by Mark (Thank you!!)

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.

Metadata Update from @tbordaz:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.3.2.24

2 years ago

Login to comment on this ticket.

Metadata