From ce896d538052d20f56f440d1a23fd99da950ed07 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 19 Dec 2018 12:34:13 +0000 Subject: [PATCH 1/2] Fix segfault in nops when used with membersof overlay Allow problematic variables to be defined on heap so modifications to modlist can happen in other overlay modules Signed-off-by: Noel Power --- servers/slapd/overlays/memberof.c | 64 +++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/servers/slapd/overlays/memberof.c b/servers/slapd/overlays/memberof.c index 40ad6e2..358d5f9 100644 --- a/servers/slapd/overlays/memberof.c +++ b/servers/slapd/overlays/memberof.c @@ -355,10 +355,13 @@ memberof_value_modify( unsigned long opid = op->o_opid; SlapReply rs2 = { REP_RESULT }; slap_callback cb = { NULL, slap_null_cb, NULL, NULL }; - Modifications mod[ 2 ] = { { { 0 } } }, *ml; - struct berval values[ 4 ], nvalues[ 4 ]; + Modifications *mod[2] = {NULL, NULL}; + Modifications *ml = NULL; int mcnt = 0; + mod[0] = (Modifications*)ch_calloc( sizeof(Modifications), 1 ); + mod[1] = (Modifications*)ch_calloc( sizeof(Modifications), 1 ); + op2.o_tag = LDAP_REQ_MODIFY; op2.o_req_dn = *ndn; @@ -375,13 +378,17 @@ memberof_value_modify( op2.o_dont_replicate = 1; if ( !BER_BVISNULL( &mo->mo_ndn ) ) { - ml = &mod[ mcnt ]; + ml = mod[ mcnt ]; ml->sml_numvals = 1; - ml->sml_values = &values[ 0 ]; - ml->sml_values[ 0 ] = mo->mo_dn; + ml->sml_values = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + ber_dupbv(&ml->sml_values[ 0 ], &mo->mo_ndn); BER_BVZERO( &ml->sml_values[ 1 ] ); - ml->sml_nvalues = &nvalues[ 0 ]; - ml->sml_nvalues[ 0 ] = mo->mo_ndn; + ml->sml_nvalues = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + ber_dupbv(&ml->sml_nvalues[ 0 ], &mo->mo_ndn); BER_BVZERO( &ml->sml_nvalues[ 1 ] ); ml->sml_desc = slap_schema.si_ad_modifiersName; ml->sml_type = ml->sml_desc->ad_cname; @@ -393,11 +400,17 @@ memberof_value_modify( mcnt++; } - ml = &mod[ mcnt ]; + ml = mod[ mcnt ]; ml->sml_numvals = 1; - ml->sml_values = &values[ 2 ]; + ml->sml_values = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + BER_BVZERO( &ml->sml_values[ 0 ] ); BER_BVZERO( &ml->sml_values[ 1 ] ); - ml->sml_nvalues = &nvalues[ 2 ]; + ml->sml_nvalues = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + BER_BVZERO( &ml->sml_nvalues[ 0 ] ); BER_BVZERO( &ml->sml_nvalues[ 1 ] ); ml->sml_desc = ad; ml->sml_type = ml->sml_desc->ad_cname; @@ -412,11 +425,13 @@ memberof_value_modify( assert( !BER_BVISNULL( new_dn ) ); assert( !BER_BVISNULL( new_ndn ) ); - ml = &mod[ mcnt ]; + ml = mod[ mcnt ]; ml->sml_op = LDAP_MOD_ADD; - ml->sml_values[ 0 ] = *new_dn; - ml->sml_nvalues[ 0 ] = *new_ndn; + ber_memfree(ml->sml_values[ 0 ].bv_val); + ber_memfree(ml->sml_nvalues[ 0 ].bv_val); + ber_dupbv(&ml->sml_values[ 0 ], new_dn); + ber_dupbv(&ml->sml_nvalues[ 0 ], new_dn); oex.oe_key = (void *)&memberof; LDAP_SLIST_INSERT_HEAD(&op2.o_extra, &oex, oe_next); @@ -433,18 +448,18 @@ memberof_value_modify( op->o_log_prefix, buf, 0 ); } - assert( op2.orm_modlist == &mod[ mcnt ] ); - assert( mcnt == 0 || op2.orm_modlist->sml_next == &mod[ 0 ] ); + assert( op2.orm_modlist == mod[ mcnt ] ); + assert( mcnt == 0 || op2.orm_modlist->sml_next == mod[ 0 ] ); ml = op2.orm_modlist->sml_next; if ( mcnt == 1 ) { - assert( ml == &mod[ 0 ] ); + assert( ml == mod[ 0 ] ); ml = ml->sml_next; } if ( ml != NULL ) { slap_mods_free( ml, 1 ); } - mod[ 0 ].sml_next = NULL; + mod[ 0 ]->sml_next = NULL; } if ( old_ndn != NULL ) { @@ -454,11 +469,13 @@ memberof_value_modify( assert( !BER_BVISNULL( old_dn ) ); assert( !BER_BVISNULL( old_ndn ) ); - ml = &mod[ mcnt ]; + ml = mod[ mcnt ]; ml->sml_op = LDAP_MOD_DELETE; - - ml->sml_values[ 0 ] = *old_dn; - ml->sml_nvalues[ 0 ] = *old_ndn; + + ber_memfree(ml->sml_values[ 0 ].bv_val); + ber_memfree(ml->sml_nvalues[ 0 ].bv_val); + ber_dupbv(&ml->sml_values[ 0 ], old_dn); + ber_dupbv(&ml->sml_nvalues[ 0 ], old_dn); oex.oe_key = (void *)&memberof; LDAP_SLIST_INSERT_HEAD(&op2.o_extra, &oex, oe_next); @@ -475,10 +492,10 @@ memberof_value_modify( op->o_log_prefix, buf, 0 ); } - assert( op2.orm_modlist == &mod[ mcnt ] ); + assert( op2.orm_modlist == mod[ mcnt ] ); ml = op2.orm_modlist->sml_next; if ( mcnt == 1 ) { - assert( ml == &mod[ 0 ] ); + assert( ml == mod[ 0 ] ); ml = ml->sml_next; } if ( ml != NULL ) { @@ -488,6 +505,7 @@ memberof_value_modify( /* restore original opid */ op->o_opid = opid; + slap_mods_free( op2.orm_modlist, 1 ); /* FIXME: if old_group_ndn doesn't exist, both delete __and__ * add will fail; better split in two operations, although * not optimal in terms of performance. At least it would -- 2.16.4 From 6bd3ce920e750c21cedf4a118027043d37056950 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 19 Dec 2018 15:51:37 +0000 Subject: [PATCH 2/2] Remove asserts to allow nops to process. The asserts present seem to: a) ensure that only additions can happen to the modlist b) that we only delete Modifications that have been added These asserts are bound to the assumption that no other overlay will delete Modifications from the modlist and additionally are there to protect illegal deletion of Modifications allocated on the stack. These changes allow Modifications to be deleted via other overlay modules that could be called. Additionally since now the modlist elements are allocated on the heap we can delete them freely now. Signed-off-by: Noel Power --- servers/slapd/overlays/memberof.c | 129 +++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 72 deletions(-) diff --git a/servers/slapd/overlays/memberof.c b/servers/slapd/overlays/memberof.c index 358d5f9..d49f2d3 100644 --- a/servers/slapd/overlays/memberof.c +++ b/servers/slapd/overlays/memberof.c @@ -334,6 +334,55 @@ memberof_isGroupOrMember( Operation *op, memberof_cbinfo_t *mci ) return LDAP_SUCCESS; } +static Modifications* +memberof_value_modlist( + Operation *op2, + memberof_t *mo, + AttributeDescription *ad) +{ + Modifications *ml = NULL; + Modifications *result = NULL; + if ( !BER_BVISNULL( &mo->mo_ndn ) ) { + ml = (Modifications*)ch_calloc( sizeof(Modifications), 1 ); + ml->sml_numvals = 1; + ml->sml_values = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + ber_dupbv(&ml->sml_values[ 0 ], &mo->mo_ndn); + BER_BVZERO( &ml->sml_values[ 1 ] ); + ml->sml_nvalues = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + ber_dupbv(&ml->sml_nvalues[ 0 ], &mo->mo_ndn); + BER_BVZERO( &ml->sml_nvalues[ 1 ] ); + ml->sml_desc = slap_schema.si_ad_modifiersName; + ml->sml_type = ml->sml_desc->ad_cname; + ml->sml_op = LDAP_MOD_REPLACE; + ml->sml_flags = SLAP_MOD_INTERNAL; + ml->sml_next = result; + result = ml; + } + + ml = (Modifications*)ch_calloc( sizeof(Modifications), 1 ); + ml->sml_numvals = 1; + ml->sml_values = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + BER_BVZERO( &ml->sml_values[ 0 ] ); + BER_BVZERO( &ml->sml_values[ 1 ] ); + ml->sml_nvalues = + (BerVarray)ch_malloc( + (ml->sml_numvals + 1) * sizeof( struct berval )); + BER_BVZERO( &ml->sml_nvalues[ 0 ] ); + BER_BVZERO( &ml->sml_nvalues[ 1 ] ); + ml->sml_desc = ad; + ml->sml_type = ml->sml_desc->ad_cname; + ml->sml_flags = SLAP_MOD_INTERNAL; + ml->sml_next = result; + result = ml; + return result; +} + /* * response callback that adds memberof values when a group is modified. */ @@ -355,12 +404,7 @@ memberof_value_modify( unsigned long opid = op->o_opid; SlapReply rs2 = { REP_RESULT }; slap_callback cb = { NULL, slap_null_cb, NULL, NULL }; - Modifications *mod[2] = {NULL, NULL}; Modifications *ml = NULL; - int mcnt = 0; - - mod[0] = (Modifications*)ch_calloc( sizeof(Modifications), 1 ); - mod[1] = (Modifications*)ch_calloc( sizeof(Modifications), 1 ); op2.o_tag = LDAP_REQ_MODIFY; @@ -377,47 +421,6 @@ memberof_value_modify( op2.orm_no_opattrs = 1; op2.o_dont_replicate = 1; - if ( !BER_BVISNULL( &mo->mo_ndn ) ) { - ml = mod[ mcnt ]; - ml->sml_numvals = 1; - ml->sml_values = - (BerVarray)ch_malloc( - (ml->sml_numvals + 1) * sizeof( struct berval )); - ber_dupbv(&ml->sml_values[ 0 ], &mo->mo_ndn); - BER_BVZERO( &ml->sml_values[ 1 ] ); - ml->sml_nvalues = - (BerVarray)ch_malloc( - (ml->sml_numvals + 1) * sizeof( struct berval )); - ber_dupbv(&ml->sml_nvalues[ 0 ], &mo->mo_ndn); - BER_BVZERO( &ml->sml_nvalues[ 1 ] ); - ml->sml_desc = slap_schema.si_ad_modifiersName; - ml->sml_type = ml->sml_desc->ad_cname; - ml->sml_op = LDAP_MOD_REPLACE; - ml->sml_flags = SLAP_MOD_INTERNAL; - ml->sml_next = op2.orm_modlist; - op2.orm_modlist = ml; - - mcnt++; - } - - ml = mod[ mcnt ]; - ml->sml_numvals = 1; - ml->sml_values = - (BerVarray)ch_malloc( - (ml->sml_numvals + 1) * sizeof( struct berval )); - BER_BVZERO( &ml->sml_values[ 0 ] ); - BER_BVZERO( &ml->sml_values[ 1 ] ); - ml->sml_nvalues = - (BerVarray)ch_malloc( - (ml->sml_numvals + 1) * sizeof( struct berval )); - BER_BVZERO( &ml->sml_nvalues[ 0 ] ); - BER_BVZERO( &ml->sml_nvalues[ 1 ] ); - ml->sml_desc = ad; - ml->sml_type = ml->sml_desc->ad_cname; - ml->sml_flags = SLAP_MOD_INTERNAL; - ml->sml_next = op2.orm_modlist; - op2.orm_modlist = ml; - if ( new_ndn != NULL ) { BackendInfo *bi = op2.o_bd->bd_info; OpExtra oex; @@ -425,7 +428,9 @@ memberof_value_modify( assert( !BER_BVISNULL( new_dn ) ); assert( !BER_BVISNULL( new_ndn ) ); - ml = mod[ mcnt ]; + ml = memberof_value_modlist(&op2, mo, ad); + op2.orm_modlist = ml; + ml->sml_op = LDAP_MOD_ADD; ber_memfree(ml->sml_values[ 0 ].bv_val); @@ -447,19 +452,7 @@ memberof_value_modify( Debug( LDAP_DEBUG_ANY, "%s: %s\n", op->o_log_prefix, buf, 0 ); } - - assert( op2.orm_modlist == mod[ mcnt ] ); - assert( mcnt == 0 || op2.orm_modlist->sml_next == mod[ 0 ] ); - ml = op2.orm_modlist->sml_next; - if ( mcnt == 1 ) { - assert( ml == mod[ 0 ] ); - ml = ml->sml_next; - } - if ( ml != NULL ) { - slap_mods_free( ml, 1 ); - } - - mod[ 0 ]->sml_next = NULL; + slap_mods_free( op2.orm_modlist, 1 ); } if ( old_ndn != NULL ) { @@ -469,7 +462,9 @@ memberof_value_modify( assert( !BER_BVISNULL( old_dn ) ); assert( !BER_BVISNULL( old_ndn ) ); - ml = mod[ mcnt ]; + ml = memberof_value_modlist(&op2, mo, ad); + op2.orm_modlist = ml; + ml->sml_op = LDAP_MOD_DELETE; ber_memfree(ml->sml_values[ 0 ].bv_val); @@ -491,21 +486,11 @@ memberof_value_modify( Debug( LDAP_DEBUG_ANY, "%s: %s\n", op->o_log_prefix, buf, 0 ); } - - assert( op2.orm_modlist == mod[ mcnt ] ); - ml = op2.orm_modlist->sml_next; - if ( mcnt == 1 ) { - assert( ml == mod[ 0 ] ); - ml = ml->sml_next; - } - if ( ml != NULL ) { - slap_mods_free( ml, 1 ); - } + slap_mods_free( op2.orm_modlist, 1 ); } /* restore original opid */ op->o_opid = opid; - slap_mods_free( op2.orm_modlist, 1 ); /* FIXME: if old_group_ndn doesn't exist, both delete __and__ * add will fail; better split in two operations, although * not optimal in terms of performance. At least it would -- 2.16.4