Bug Description: Even if setting a config parameter to nsslapd-
allowed-to-delete-attrs, the value failed to delete if the type
was on|off or integer.

Fix Description: Store all the initial config param values in
ConfigList. If the attribute value is deleted, reset the initial
value.

git patch file (master)
0001-Trac-Ticket-443-Deleting-attribute-present-in.patch

Reviewed by Rich (Thank you!!)

Pushed to master.

commit 90dd9bb

==16727== Thread 43:
==16727== Invalid read of size 4
==16727== at 0x4C93712: config_set_onoff (libglobs.c:3252)
==16727== by 0x4C9386E: config_set_schemacheck (libglobs.c:3302)
==16727== by 0x4C99E51: config_set (libglobs.c:7209)
==16727== by 0x11ED75: modify_config_dse (configdse.c:456)
==16727== by 0x4C7402F: dse_call_callback (dse.c:2421)
==16727== by 0x4C72652: dse_modify (dse.c:1860)
==16727== by 0x4CAE3C6: op_shared_modify (modify.c:1078)
==16727== by 0x4CAC9C4: do_modify (modify.c:416)
==16727== by 0x1208AC: connection_dispatch_operation (connection.c:660)
==16727== by 0x12291F: connection_threadmain (connection.c:2534)
==16727== by 0x690FC75: _pt_root (ptthread.c:204)
==16727== by 0x6F4CD14: start_thread (pthread_create.c:308)
==16727== by 0x746A53C: clone (clone.S:114)
==16727== Address 0xdbacc10 is 0 bytes inside a block of size 1 alloc'd
==16727== at 0x4A0887C: malloc (vg_replace_malloc.c:270)
==16727== by 0x5B572A4: ber_memalloc_x (memory.c:228)
==16727== by 0x5B5760B: ber_dupbv_x (memory.c:506)
==16727== by 0x4CFD314: normalize_mods2bvals (util.c:773)
==16727== by 0x4CAC8FD: do_modify (modify.c:405)
==16727== by 0x1208AC: connection_dispatch_operation (connection.c:660)
==16727== by 0x12291F: connection_threadmain (connection.c:2534)
==16727== by 0x690FC75: _pt_root (ptthread.c:204)
==16727== by 0x6F4CD14: start_thread (pthread_create.c:308)
==16727== by 0x746A53C: clone (clone.S:114)

Description: commit 90dd9bb introduced
an Invalid read. If an empty value is set to on/off type config param,
it cannot be distinguished from the internal OFF value '0'. When checking
the value other than "on" and "off", the pointer casted to an integer and
evaluated the value. When an empty value is given, it is an empty string
"", which size is sizeof(char), not sizeof(int), which caused the Invalid
read.

This patch re-typedef slapi_onoff_t with PRInt8 or char, which restricts
to access sizeof(char), which prevents the Invalid read even if an empty
string is passed.

git patch file (master) -- fixing invalid read when an empty string is set to on/off type config param
0003-Ticket-443-Deleting-attribute-present-in-nsslapd-all.patch

It turned out PR_AtomicSet takes PRInt32 * and there is no other AtomicSet taking other types in NSPR... I have to nack my proposal by myself... :(
../ldap/servers/slapd/libglobs.c:3275:3: warning: passing argument 1 of 'PR_AtomicSet' from incompatible pointer type [enabled by default]
/usr/include/nspr4/pratom.h:51:19: note: expected 'PRInt32 ' but argument is of type 'slapi_onoff_t '

Description: commit 90dd9bb introduced
an Invalid read/write. The commit meant to allow "on" and "off" as well
as integer 0 and 1 in on/off type of config parameters. But it caused
the type mismatch. This patch strictly limits the value to "on" or "off"
(in the case insensitive manner).

git patch file (master; take 2) -- fixing invalid read when an empty string is set to on/off type config param
0001-Ticket-443-Deleting-attribute-present-in-nsslapd-all.patch

You can get rid of char *init_nagle;

define DEFAULT_CONNECTION_BUFFER "on" / LDAP_ON /

connection_buffer is an int, not an on/off value

Looks like the problem is here:
{{{
if ((NULL == values) &&
config_allowed_to_delete_attrs(cgas->attr_name)) {
if (cgas->setfunc) {
retval = (cgas->setfunc)(cgas->attr_name, cgas->initvalue,
errorbuf, apply);
} else if (cgas->logsetfunc) {
retval = (cgas->logsetfunc)(cgas->attr_name, cgas->initvalue,
cgas->whichlog, errorbuf, apply);
} else {
LDAPDebug1Arg(LDAP_DEBUG_ANY,
"config_set: the attribute %s is read only; "
"ignoring setting NULL value\n", attr);
}
}
}}}

The problem being that setfunc expects a char value as the second parameter. So, why not just convert cgas->initvalue to a string? e.g. something like this:
{{{
if ((NULL == values) &&
config_allowed_to_delete_attrs(cgas->attr_name)) {
char initvalbuf[64];
char initval = NULL;
initval = config_initvalue_to_string(cgas, initvalbuf, sizeof(initvalbuf));
if (cgas->setfunc) {
retval = (cgas->setfunc)(cgas->attr_name, initval,
errorbuf, apply);
} else if (cgas->logsetfunc) {
retval = (cgas->logsetfunc)(cgas->attr_name, initval,
cgas->whichlog, errorbuf, apply);
} else {
LDAPDebug1Arg(LDAP_DEBUG_ANY,
"config_set: the attribute %s is read only; "
"ignoring setting NULL value\n", attr);
}
}
}}}

then
{{{
static char
config_initvalue_to_string(struct config_get_and_set cgas, char initvalbuf, size_t initvalbufsize)
{
char retval;
if (isInt(cgas->config_var_type)) {
int ival = (int )(intptr_t)cgas->initvalue)
PR_snprintf(initvalbuf, initvalbufsize, "%d", ival);
retval = initvalbuf;
} else if (cgas->config_var_type == CONFIG_LONG) {
long lval = (long )(intptr_t)cgas->initvalue;
PR_snprintf(initvalbuf, initvalbufsize, "%ld", lval);
retval = initvalbuf;
} else {
retval = cgas->initvalue;
}
return retval;
}}}

Thanks, Rich.

In the current code, the init value stores integer 0 or 1 (not "0" or "1")... E.g.,
init_schemacheck = cfg->schemacheck = LDAP_ON;

Probably, if we change config_set_onoff like this, then it'd work...? I'm trying...
{{{
static int
config_set_onoff ( const char attrname, char value, int configvalue,
char errorbuf, int apply )
...
if ( strcasecmp ( value, "on" ) != 0 &&
strcasecmp ( value, "off") != 0 &&
/ initializing the value /
((int )value != LDAP_ON) && ==> strcasecmp ( value, "1" ) != 0 )
((int )value != LDAP_OFF) ==> strcasecmp ( value, "0" ) != 0 ) {
...
if ( strcasecmp ( value, "on" ) == 0 ) {
newval = LDAP_ON;
} else if ( strcasecmp ( value, "off" ) == 0 ) {
newval = LDAP_OFF;
+ } else if ( strcasecmp ( value, "1" ) == 0 ) {
+ newval = LDAP_ON;
+ } else if ( strcasecmp ( value, "0" ) == 0 ) {
+ newval = LDAP_OFF;
} else { / assume it is an integer /
newval = (slapi_onoff_t )value;
}
}}}

or
{{{
static char
config_initvalue_to_string(struct config_get_and_set cgas, char initvalbuf, size_t initvalbufsize)
{
char retval;
if (cgas->config_var_type == CONFIG_ON_OFF) {
slapi_onoff_t ival = (slapi_onoff_t )(intptr_t)cgas->initvalue;
PR_snprintf(initvalbuf, initvalbufsize, "%s", (ival && ival) ? "on" : "off");
retval = initvalbuf;
} else if (isInt(cgas->config_var_type)) {
int ival = (int )(intptr_t)cgas->initvalue)
PR_snprintf(initvalbuf, initvalbufsize, "%d", ival ? ival : 0);
retval = initvalbuf;
} else if (cgas->config_var_type == CONFIG_LONG) {
long lval = (long )(intptr_t)cgas->initvalue;
PR_snprintf(initvalbuf, initvalbufsize, "%ld", lval ? *lval: 0);
retval = initvalbuf;
} else {
retval = cgas->initvalue;
}
return retval;
}
}}}

and if this still doesn't catch all of the types, then perhaps it is better to just refactor the function config_set_value() which handles all of the config value types already. All of the code that does value conversion based on type would be moved into config_value_to_string, and the code in config_set_value() would then just call slapi_entry_attr_set_charptr() with the string value, instead of calling slapi_entry_attr_set_int/long.

Well, I've finished testing a patch prior to your comment 17. It passed fine. I'm updating it with your comment 17 and trying again...

git patch file (master; take 3) -- fixing invalid read when an empty string is set to on/off type config param
0001-Ticket-443-Deleting-attribute-present-in-nsslapd-all.2.patch

Do all of the config parameters have an initvalue? If not, then this will cause a crash:
{{{

7146                    retval = initvalbuf; 
7147            } else if (isInt(cgas->config_var_type)) { 
7148                    int *ival = (int *)(intptr_t)cgas->initvalue; 
7149                    PR_snprintf(initvalbuf, initvalbufsize, "%d", *ival); 
7150                    retval = initvalbuf; 
7151            } else if (cgas->config_var_type == CONFIG_LONG) { 
7152                    long *lval = (long *)(intptr_t)cgas->initvalue; 
7153                    PR_snprintf(initvalbuf, initvalbufsize, "%ld", *lval); 
7154                    retval = initvalbuf;

}}}

See https://fedorahosted.org/389/ticket/443#comment:18 e.g.
{{{
PR_snprintf(initvalbuf, initvalbufsize, "%d", ival ? ival : 0);
...
PR_snprintf(initvalbuf, initvalbufsize, "%ld", lval ? lval: 0);
}}}

All right. Since the invalid read/write problem is limited in the onoff type, I'm going to convert to string just onoff values as something like this... All onoff types have the initvalue.

{{{
@@ -7143,6 +7136,18 @@ config_get_listen_backlog_size()
return retVal;
}

+static char
+config_initvalue_to_onoff(struct config_get_and_set cgas, char initvalbuf, size_t initvalbufsize)
+{
+ char retval = NULL;
+ if (cgas->config_var_type == CONFIG_ON_OFF) {
+ slapi_onoff_t ival = (slapi_onoff_t )(intptr_t)cgas->initvalue;
+ PR_snprintf(initvalbuf, initvalbufsize, "%s", (ival && ival) ? "on" : "off");
+ retval = initvalbuf;
+ }
+ return retval;
+}
+
/
* This function is intended to be used from the dse code modify callback. It
* is "optimized" for that case because it takes a berval of values, which is
@@ -7191,12 +7196,15 @@ config_set(const char *attr, struct berval values, char errorbuf, int apply)
default:
if ((NULL == values) &&
config_allowed_to_delete_attrs(cgas->attr_name)) {
+ char initvalbuf[64];
+ void initval = cgas->initvalue;
+ if (cgas->config_var_type == CONFIG_ON_OFF) {
+ initval = (void *)config_initvalue_to_string(cgas, initvalbuf, sizeof(initvalbuf));
+ }
if (cgas->setfunc) {
- retval = (cgas->setfunc)(cgas->attr_name, cgas->initvalue,
- errorbuf, apply);
+ retval = (cgas->setfunc)(cgas->attr_name, initval, errorbuf, apply);
} else if (cgas->logsetfunc) {
- retval = (cgas->logsetfunc)(cgas->attr_name, cgas->initvalue,
- cgas->whichlog, errorbuf, apply);
+ retval = (cgas->logsetfunc)(cgas->attr_name, initval, cgas->whichlog, errorbuf, apply)
} else {
LDAPDebug1Arg(LDAP_DEBUG_ANY,
"config_set: the attribute %s is read only; "

}}}

ack

Reviewed by Rich (Thank you!!)

Pushed to master:
020c163..c52987d master -> master
commit c52987d

Pushed to 389-ds-base-1.3.2:
cb67295..4266657 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 4266657

Pushed to 389-ds-base-1.3.1:
8937cd7..d68dc32 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit d68dc32

Backported to 389-ds-base-1.2.11:
502629a..2b08517 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 77cb32c
commit 2b08517

Fixed a regression in 1.2.11 introduced by the backport.
19e4ea6..06dfa1d 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 06dfa1d

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/443

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.