#51077 Issue 51076 - prevent unnecessary duplication of the target entry
Closed 3 years ago by spichugi. Opened 3 years ago by mreynolds.
mreynolds/389-ds-base issue51076  into  master

@@ -2165,9 +2165,8 @@ 

                  if (e && free_entry) {

                      slapi_entry_free(e);

                  }

- 

-                 slapi_search_internal_get_entry(sdn, 0, &e, mep_get_plugin_id());

-                 free_entry = 1;

+                 slapi_pblock_get(pb, SLAPI_ENTRY_PRE_OP, &e);

+                 free_entry = 0;

              }

  

              if (e && mep_is_managed_entry(e)) {

Bug Description:

For any update operation the MEP plugin was calling slapi_search_internal_get_entry() which duplicates the entry it returns. In this case the entry is just read from and discarded, but this entry is already in the pblock (the PRE OP ENTRY).

In modify.c we were also duplicating an entry to check for password policy attributes/objectclasses.

Fix Description:

Just grab the PRE OP ENTRY from the pblock and use that to read the attribute values from. This saves two entry duplications for every update operation from MEP, and two duplications in op_shared_modify().

So this patch reduces the total number of entry duplications from 8 down to 4 per modify.

fixes: https://pagure.io/389-ds-base/issue/51076

rebased onto 0549ff0525373c58485abed69fc85220b28bc32e

3 years ago

Changes made to include the extra dups in op_shared_modify(). This also passes ASAN tests

That's pretty nice! I assume it passes the MEP tests too?

Good catch !

I agree with change in mep.c as it is a betxn but in modify.c, SLAPI_ENTRY_PRE_OP is not set yet. I would expect checking/transformation on password related stuff to be skipped.

Good catch !
I agree with change in mep.c as it is a betxn but in modify.c, SLAPI_ENTRY_PRE_OP is not set yet

It is set. In all my tests, the preop entry is always present - it is never NULL in op_shared_modify().

Doing some more checking we call slapi_search_internal_get_entry() in a lot of places (especially plugins). I'm going to see if I caneliminate some of these as well:

servers/plugins/acctpolicy/acct_config.c:    rc = slapi_search_internal_get_entry(config_sdn, NULL, &config_entry,
servers/plugins/acctpolicy/acct_util.c:    ldrc = slapi_search_internal_get_entry(sdn, NULL, &policy_entry,
servers/plugins/acctpolicy/acct_plugin.c:    ldrc = slapi_search_internal_get_entry(sdn, NULL, &target_entry,
servers/plugins/acctpolicy/acct_plugin.c:        ldrc = slapi_search_internal_get_entry(sdn, NULL, &target_entry,
servers/plugins/acctpolicy/acct_plugin.c:                slapi_search_internal_get_entry(sdn, 0, &e, get_identity());
servers/plugins/automember/automember.c:    rc = slapi_search_internal_get_entry(group_sdn, NULL, &group_entry, automember_get_plugin_id());
servers/plugins/automember/automember.c:                slapi_search_internal_get_entry(sdn, 0, &e, automember_get_plugin_id());
servers/plugins/memberof/memberof.c:    slapi_search_internal_get_entry(op_to_sdn, config->groupattrs,
servers/plugins/memberof/memberof.c:    slapi_search_internal_get_entry(sdn, config->groupattrs,
servers/plugins/memberof/memberof_config.c:        slapi_search_internal_get_entry(config_sdn, NULL, &e, memberof_get_plugin_id());
servers/plugins/memberof/memberof_config.c:            slapi_search_internal_get_entry(config_sdn, NULL, &config_entry, memberof_get_plugin_id());
servers/plugins/memberof/memberof_config.c:                                    rc = slapi_search_internal_get_entry(config_sdn, NULL, &config_entry,
servers/plugins/mep/mep.c:        slapi_search_internal_get_entry(entry->template_sdn, 0,
servers/plugins/mep/mep.c:                slapi_search_internal_get_entry(sdn, 0, &e, mep_get_plugin_id());
servers/plugins/mep/mep.c:                    slapi_search_internal_get_entry(sdn, 0, &e, mep_get_plugin_id());
servers/plugins/mep/mep.c:                slapi_search_internal_get_entry(sdn, 0, &e, mep_get_plugin_id());
servers/plugins/mep/mep.c:                        slapi_search_internal_get_entry(origin_sdn, 0,
servers/plugins/mep/mep.c:            if (slapi_search_internal_get_entry(managed_sdn, 0,
servers/plugins/dna/dna.c:        slapi_search_internal_get_entry(sdn, attrs, &shared_e, getPluginID());
servers/plugins/dna/dna.c:        slapi_search_internal_get_entry(config_dn, attrs, &e, getPluginID());
servers/plugins/dna/dna.c:        slapi_search_internal_get_entry(replica_sdn, attrs, &e, getPluginID());
servers/plugins/dna/dna.c:                    slapi_search_internal_get_entry(bind_group_sdn, attrs, &bind_group_entry, getPluginID());
servers/plugins/uiduniq/uid.c:    err = slapi_search_internal_get_entry(sdn, NULL, &e, plugin_identity);
servers/plugins/linkedattrs/linked_attrs.c:                slapi_search_internal_get_entry(tmp_dn, 0, &e, linked_attrs_get_plugin_id());
servers/plugins/referint/referint.c:            result = slapi_search_internal_get_entry(config_sdn, NULL, &e, referint_plugin_identity);
servers/plugins/referint/referint.c:        result = slapi_search_internal_get_entry(config_sdn, NULL, &e, referint_plugin_identity);
servers/plugins/referint/referint.c:            rc = slapi_search_internal_get_entry(config_sdn, NULL, &e, referint_plugin_identity);
servers/plugins/replication/windows_protocol_util.c:    rc = slapi_search_internal_get_entry(sdn, NULL, &entry, repl_get_plugin_identity(PLUGIN_MULTIMASTER_REPLICATION));
servers/plugins/replication/windows_protocol_util.c:    slapi_search_internal_get_entry((Slapi_DN *)local_dn, NULL, &new_entry,
servers/plugins/replication/windows_private.c:    int rc = slapi_search_internal_get_entry(agmtdn, NULL, &agmte,
servers/plugins/replication/windows_private.c:    rc = slapi_search_internal_get_entry(sdn, NULL, &entry,
servers/plugins/replication/repl5_tot_protocol.c:        rc = slapi_search_internal_get_entry(area_sdn, NULL, &suffix, repl_get_plugin_identity(PLUGIN_MULTIMASTER_REPLICATION));
servers/plugins/posix-winsync/posix-winsync.c:        rc = slapi_search_internal_get_entry(childparent, nisDomainAttr, &entry,
servers/plugins/posix-winsync/posix-group-func.c:    int rc = slapi_search_internal_get_entry(udn_sdn, attrs, &result, posix_winsync_get_plugin_identity());
servers/plugins/chainingdb/cb_instance.c:    if (LDAP_SUCCESS == (slapi_search_internal_get_entry(aDn, NULL, &anEntry, inst->backend_type->identity))) {
servers/plugins/pam_passthru/pam_ptpreop.c:            slapi_search_internal_get_entry(sdn, 0, &e,
servers/plugins/pam_passthru/pam_ptconfig.c:                    slapi_search_internal_get_entry(bind_sdn, NULL, &test_e,
servers/plugins/pam_passthru/pam_ptimpl.c:    int rc = slapi_search_internal_get_entry((Slapi_DN *)bindsdn, attrs, &entry,
servers/plugins/http/http_impl.c:    rc = slapi_search_internal_get_entry(sdn, NULL, &entry, plugin_id);
servers/plugins/views/views.c:            slapi_search_internal_get_entry(sdn, NULL, &e, view_get_plugin_identity());
servers/slapd/pw_retry.c:    search_result = slapi_search_internal_get_entry(target_sdn, NULL,
servers/slapd/test-plugins/testbind.c:        rc = slapi_search_internal_get_entry(sdn, attrs, &e,
servers/slapd/daemon.c:                ret = slapi_search_internal_get_entry(
servers/slapd/slapi-plugin.h: * slapi_search_internal_get_entry() finds an entry given a dn.  It returns
servers/slapd/slapi-plugin.h:int slapi_search_internal_get_entry(Slapi_DN *dn, char **attrlist, Slapi_Entry **ret_entry, void *caller_identity);
servers/slapd/passwd_extop.c:    if ((search_result = slapi_search_internal_get_entry(&sdn, NULL, e2,
servers/slapd/schema.c:    if (slapi_search_internal_get_entry(&sdn, NULL, &entry, plugin_get_default_component_id()) == LDAP_SUCCESS) {
servers/slapd/resourcelimit.c:        (void)slapi_search_internal_get_entry(dn, attrs, &e, reslimit_componentid);
servers/slapd/snmp_collator.c:    slapi_search_internal_get_entry(&sdn, NULL, e,
servers/slapd/back-ldbm/ldbm_attrcrypt.c:    slapi_search_internal_get_entry(&sdn, NULL, e2,
servers/slapd/sasl_map.c:    slapi_search_internal_get_entry(&sdn, NULL, e2,
servers/slapd/modify.c:        slapi_search_internal_get_entry(&sdn, NULL, &e, (void *)plugin_get_default_component_id());
servers/slapd/ssl.c:    slapi_search_internal_get_entry(&sdn, NULL, e2,

It's probably worth doing these in small batches OR as a set of small commits to make bisect easier if something goes wrong when we go to test it :)

It's probably worth doing these in small batches OR as a set of small commits to make bisect easier if something goes wrong when we go to test it :)

Of course, and I suspect most of these we still need to call. It's not like we can remove all of these functions, but in cases where it's getting the current target entry and we are only reading it then we can skip it. I already confirmed we need those functions in attribute uniqueness and memberof. I will look at the reset and I will open a new PR if I find any that can be removed.

So this PR is "complete" and still needs final review/ack.

rebased onto bc094c70b2a20a854b4bb6dbd0ac45e8a509024c

3 years ago

It's probably worth doing these in small batches OR as a set of small commits to make bisect easier if something goes wrong when we go to test it :)

Of course, and I suspect most of these we still need to call. It's not like we can remove all of these functions, but in cases where it's getting the current target entry and we are only reading it then we can skip it. I already confirmed we need those functions in attribute uniqueness and memberof. I will look at the reset and I will open a new PR if I find any that can be removed.
So this PR is "complete" and still needs final review/ack.

I narrowed that list down to 5,and I will open a new PR for those, but they are less critical than this PR.

Good catch !
I agree with change in mep.c as it is a betxn but in modify.c, SLAPI_ENTRY_PRE_OP is not set yet

It is set. In all my tests, the preop entry is always present - it is never NULL in op_shared_modify().

On master+patch + ldapmodify test_user, 'slapi_pblock_get(pb, SLAPI_ENTRY_PRE_OP, &e);' returns e=NULL instead of entry_test_user.

I only found ldbm callbacks setting SLAPI_ENTRY_PRE_OP.

rebased onto cee9774be44ea6240276d253a43744e9bfda738f

3 years ago

rebased onto ed24259dadf98671e0550290fa1d17a372055153

3 years ago

Good catch !
I agree with change in mep.c as it is a betxn but in modify.c, SLAPI_ENTRY_PRE_OP is not set yet
It is set. In all my tests, the preop entry is always present - it is never NULL in op_shared_modify().

On master+patch + ldapmodify test_user, 'slapi_pblock_get(pb, SLAPI_ENTRY_PRE_OP, &e);' returns e=NULL instead of entry_test_user.
I only found ldbm callbacks setting SLAPI_ENTRY_PRE_OP.

Okay I was wrong. Ugh so we can not remove those entry duplicates. So until password policy becomes a plugin(post op), we need to keep the current behavior. I've reviewed all places where we duplicate entries, and I do not see that they can be optimized, so its just the MEP plugin.

Please review one last time

Yeah, no issues from me here, but @tbordaz should check too :)

For sure you have my ACK it is a nice catch :)

Regarding the remaining 5 suspects calls to slapi_search_internal_get_entry (https://pagure.io/389-ds-base/pull-request/51077#comment-119223). Do you want to open a PR or a Ticket ? IMHO I would prefer a ticket or even a ticket by fixed plugins.

Well done @mreynolds, thanks @vashirov sharing all these results

For sure you have my ACK it is a nice catch :)
Regarding the remaining 5 suspects calls to slapi_search_internal_get_entry (https://pagure.io/389-ds-base/pull-request/51077#comment-119223). Do you want to open a PR or a Ticket ? IMHO I would prefer a ticket or even a ticket by fixed plugins.

No we need all of those. We could get rid of two more but that would requires handing the password policy stuff later in the operation after we set the preop entry in the pblock. But I do wonder what @vashirov's results would look like if we could get rid of those calls from modify.c, but as of today we can not get rid of them.

rebased onto bc789a9

3 years ago

Pull-Request has been merged by mreynolds

3 years ago

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4130

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago
Metadata