#443 Deleting attribute present in nsslapd-allowed-to-delete-attrs returns Operations error
Closed: Fixed None Opened 7 years ago by rmeggins.

https://bugzilla.redhat.com/show_bug.cgi?id=853106 (389)

Description of problem:

See Bug 602456. Adding attribute to nsslapd-allowed-to-delete-attrs should
allow attribute to be deleted.


Version-Release number of selected component (if applicable):
389-ds-base-1.2.10.2-15.el6.x86_64 as well as 389-ds-base-1.2.11.7-2.el6.x86_64


How reproducible:
always

Steps to Reproduce:
1. Add nsslapd-require-secure-binds attribute to
nsslapd-allowed-to-delete-attrs

ldapmodify  -h localhost -p 22222 -D "cn=Directory manager" -w dirmanager <<
EOF
dn: cn=config
changetype: modify
replace: nsslapd-allowed-to-delete-attrs
nsslapd-allowed-to-delete-attrs: nsslapd-securelistenhost nsslapd-listenhost
nsslapd-require-secure-binds
EOF

2. Restart server
3. Delete nsslapd-require-secure-binds

ldapmodify  -h localhost -p 22222 -D "cn=Directory manager" -w dirmanager <<
EOF
dn: cn=config
changetype: modify
delete: nsslapd-require-secure-binds
EOF

Actual results:

modifying entry "cn=config"
ldap_modify: Operations error (1)
        additional info: nsslapd-require-secure-binds: NULL value


Expected results: Should succeed


Additional info: See Bug 602456. Automated in acceptance/basic/config as
bug602456_12

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.

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

}}}

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

Metadata Update from @rmeggins:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.2.11.30

2 years ago

Login to comment on this ticket.

Metadata