SIGSEV when setting invalid changelog config value

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49412

• Created at 2017-10-23 17:41:31 by tbordaz (@tbordaz)
• Closed at 2017-11-07 15:00:29 as fixed
• Assigned to nobody

---

Issue Description

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

Package Version and Platform

at least since 1.3.6

Steps to reproduce

1. see attached test case

Actual results

Crash

Expected results

Should not crash

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-10-23 17:43:56

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-10-23 17:43:58

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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-10-25 11:19:17

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-10-25 11:19:48

Metadata Update from @tbordaz:

• Custom field reviewstatus adjusted to review (was: None)

[389-ds-bot]

Comment from lkrispen (@elkris) at 2017-10-25 12:44:05

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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-10-25 14:44:49

@elkris 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.

[389-ds-bot]

Comment from lkrispen (@elkris) at 2017-10-25 14:56:25

ok. but then I would check
if (mods[i]->mod_values)

before the for J loop and log and return an error

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-10-25 17:48:23

I agree, here a modified patch

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-10-25 18:00:46

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

[389-ds-bot]

Comment from lkrispen (@elkris) at 2017-10-25 18:36:24

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

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-10-25 19:58:30

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.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-10-26 17:57:21

Metadata Update from @mreynolds389:

• Issue set to the milestone: 1.4 backlog

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-10-31 14:43:48

Metadata Update from @mreynolds389:

• Custom field reviewstatus adjusted to ack (was: review)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-11-07 14:58:44

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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2017-11-07 15:00:55

Metadata Update from @tbordaz:

• Issue close_status updated to: fixed
• Issue status updated to: Closed (was: Open)