The issue is that updating changelog config attribute with a dummy value (leading to empty value) creates a LDAPMod with empty value and crash the server
at least since 1.3.6
Crash
Should not crash
<img alt="ticket49412_tests.py" src="/389-ds-base/issue/raw/files/5fc927480ad59def02eea443cab7ef07051d6e45d316269a44b7b565a6bcefd0-ticket49412_tests.py" />
Metadata Update from @tbordaz: - Custom field component adjusted to None - Custom field origin adjusted to None - Custom field reviewstatus adjusted to None - Custom field type adjusted to None - Custom field version adjusted to None
<img alt="0001-Ticket-49412-SIGSEV-when-setting-invalid-changelog-c.patch" src="/389-ds-base/issue/raw/files/3f00b1682d2a5815b4437ec3c7e03eb0eb84d30e113b9926343b62ba59a03283-0001-Ticket-49412-SIGSEV-when-setting-invalid-changelog-c.patch" />
Metadata Update from @tbordaz: - Custom field reviewstatus adjusted to review (was: None)
I do not fully agree with the patch. It fixes the crash, I assume it is in slapi_is_duration_valid, but haven't seen the stack trace of the crash.
But i think: - slapi_is_duration_valiid() should properly handle NULL values and return as invalidould - for a couple of attributes there is a check for NULL and an error is reurned, With your fix you just ignore NULL values and do not handle it as an error
my proposal would be to either fix slapi_is_duration_valid or do the check for NULL inside the loop and return a generic error, the attr type should be available (and remove all the checking for individual attr types -
@lkrispen thank you for looking at it. You are right, I am adding debug info.
(gdb) where #0 0x00007f940eacfa13 in changelog5_config_modify (pb=0x7f93580008c0, entryBefore=0x7f9358012760, e=0x7f9358013a40, returncode=0x7f93917f93fc, returntext=0x7f93917f9460 "", arg=0x0) at <WS>/ldap/servers/plugins/replication/cl5_config.c:310 #1 0x00007f941a9c697f in dse_call_callback (pdse=0x1a0c0b0, pb=0x7f93580008c0, operation=8, flags=1, entryBefore=0x7f9358012760, entryAfter=0x7f9358013a40, returncode=0x7f93917f93fc, returntext=0x7f93917f9460 "") at <WS>/ldap/servers/slapd/dse.c:2530 #2 0x00007f941a9c4a9b in dse_modify (pb=0x7f93580008c0) at <WS>/ldap/servers/slapd/dse.c:1753 #3 0x00007f941aa03163 in op_shared_modify (pb=0x7f93580008c0, pw_change=0, old_pw=0x0) at <WS>/ldap/servers/slapd/modify.c:1013 #4 0x00007f941aa016cb in do_modify (pb=0x7f93580008c0) at <WS>/ldap/servers/slapd/modify.c:383 #5 0x0000000000416a97 in connection_dispatch_operation (conn=0x7f941afe3c10, op=0x7f93f40045c0, pb=0x7f93580008c0) at <WS>/ldap/servers/slapd/connection.c:624 #6 0x00000000004189cd in connection_threadmain () at <WS>/ldap/servers/slapd/connection.c:1761 #7 0x00007f94187a468b in _pt_root (arg=0x1e74320) at ../../../nspr/pr/src/pthreads/ptthread.c:216 #8 0x00007f941814061a in start_thread (arg=0x7f93917fa700) at pthread_create.c:334 #9 0x00007f9417c315fd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109 (gdb) list 305 for (i = 0; mods && mods[i] != NULL; i++) { 306 if (mods[i]->mod_op & LDAP_MOD_DELETE) { 307 /* We don't support deleting changelog attributes */ 308 } else { 309 int j; 310 for (j = 0; ((mods[i]->mod_values[j]) && (LDAP_SUCCESS == rc)); j++) { 311 char *config_attr, *config_attr_value; 312 config_attr = (char *)mods[i]->mod_type; 313 config_attr_value = (char *)mods[i]->mod_bvalues[j]->bv_val; 314 (gdb) print *mods[0] $1 = {mod_op = 130, mod_type = 0x7f93580111a0 "nsslapd-changelogmaxage", mod_vals = {modv_strvals = 0x0, modv_bvals = 0x0}}
The problem is before testing the validity of the value it is in the for loop that assumes it exists values.
ok. but then I would check if (mods[i]->mod_values)
before the for J loop and log and return an error
I agree, here a modified patch
<img alt="0001-Ticket-49412-SIGSEV-when-setting-invalid-changelog-c.patch" src="/389-ds-base/issue/raw/files/2203b3e2e6dcfa821f6ec4b8bb920a8c80e36f4d9fc556eea8be5b5714bcdee9-0001-Ticket-49412-SIGSEV-when-setting-invalid-changelog-c.patch" />
minor requests
Can you make the error more clear/precise? It should say something about where the error is occurring. Right now it just says an attribute is missing a value - that's not really useful :-)
We need a space after a bracket: "}else {" should be "} else {"
minor requests Can you make the error more clear/precise? It should say something about where the error is occurring. Right now it just says an attribute is missing a value - that's not really useful :-)
but this is the returntext explaining "UNWILLING TO PERFORM" to the clients MOD, so it should be clear
minor requests Can you make the error more clear/precise? It should say something about where the error is occurring. Right now it just says an attribute is missing a value - that's not really useful :-) but this is the returntext explaining "UNWILLING TO PERFORM" to the clients MOD, so it should be clear
True, I guess I was thinking it was also going to the errors log, but its not.
Metadata Update from @mreynolds: - Issue set to the milestone: 1.4 backlog
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to ack (was: review)
git push origin master
Counting objects: 11, done. Delta compression using up to 8 threads. Compressing objects: 100% (11/11), done. Writing objects: 100% (11/11), 2.09 KiB | 0 bytes/s, done. Total 11 (delta 8), reused 0 (delta 0) To ssh://git@pagure.io/389-ds-base.git be4d7e5..c0a7be7 master -> master
Metadata Update from @tbordaz: - Issue close_status updated to: fixed - Issue status updated to: Closed (was: Open)
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/2471
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.