389-ds-base

Clone
Source Code
GIT

#50621 Issue 50620 - Fix regressions from 50506 (slapi_enry_attr_get_ref)
Closed 5 years ago by spichugi. Opened 6 years ago by mreynolds.
mreynolds/389-ds-base attrRef  into  master

file modified
+4 -2 
@@ -584,7 +584,7 @@ 
 

      }
 

  

      /* Load the filter */
 

-     value = (char *)slapi_entry_attr_get_ref(e, AUTOMEMBER_FILTER_TYPE);
 

+     value = slapi_entry_attr_get_charptr(e, AUTOMEMBER_FILTER_TYPE);
 

      if (value) {
 

          /* Convert to a Slapi_Filter to improve performance. */
 

          if (NULL == (entry->filter = slapi_str2filter(value))) {
 
@@ -595,6 +595,7 @@ 
 

                            AUTOMEMBER_FILTER_TYPE, entry->dn, value);
 

              ret = -1;
 

          }
 

+         slapi_ch_free_string(&value);
 

          if (ret != 0) {
 

              goto bail;
 

          }
 
@@ -993,7 +994,7 @@ 
 

                    "--> automember_parse_regex_entry\n");
 

  

      /* Make sure the target group was specified. */
 

-     target_group = (char *)slapi_entry_attr_get_ref(e, AUTOMEMBER_TARGET_GROUP_TYPE);
 

+     target_group = slapi_entry_attr_get_charptr(e, AUTOMEMBER_TARGET_GROUP_TYPE);
 

      if (!target_group) {
 

          slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,
 

                        "automember_parse_regex_entry - The %s config "
 
@@ -1142,6 +1143,7 @@ 
 

      }
 

  

  bail:
 

+     slapi_ch_free_string(&target_group);
 

      slapi_log_err(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,
 

                    "<-- automember_parse_regex_entry\n");
 

  }

file modified
+33 -15 
@@ -988,9 +988,10 @@ 
 

                        "----------> %s [%s]\n", DNA_TYPE, entry->types[i]);
 

      }
 

  

-     value = (char *)slapi_entry_attr_get_ref(e, DNA_NEXTVAL);
 

+     value = slapi_entry_attr_get_charptr(e, DNA_NEXTVAL);
 

      if (value) {
 

          entry->nextval = strtoull(value, 0, 0);
 

+         slapi_ch_free_string(&value);
 

      } else {
 

          slapi_log_err(SLAPI_LOG_ERR, DNA_PLUGIN_SUBSYSTEM,
 

                        "dna_parse_config_entry - The %s config "
 
@@ -1023,9 +1024,10 @@ 
 

      entry->interval = 1;
 

  

  #ifdef DNA_ENABLE_INTERVAL
 

-     value = (char *)slapi_entry_attr_get_ref(e, DNA_INTERVAL);
 

+     value = slapi_entry_attr_get_charptr(e, DNA_INTERVAL);
 

      if (value) {
 

          entry->interval = strtoull(value, 0, 0);
 

+         slapi_ch_free_string(&value);
 

      }
 

  

      slapi_log_err(SLAPI_LOG_CONFIG, DNA_PLUGIN_SUBSYSTEM,
 
@@ -1121,9 +1123,10 @@ 
 

  

      /* optional, if not specified set -1 which is converted to the max unsigned
 

       * value */
 

-     value = (char *)slapi_entry_attr_get_ref(e, DNA_MAXVAL);
 

+     value = slapi_entry_attr_get_charptr(e, DNA_MAXVAL);
 

      if (value) {
 

          entry->maxval = strtoull(value, 0, 0);
 

+         slapi_ch_free_string(&value);
 

      } else {
 

          entry->maxval = -1;
 

      }
 
@@ -1246,7 +1249,7 @@ 
 

                        entry->shared_cfg_base);
 

      }
 

  

-     value = (char *)slapi_entry_attr_get_ref(e, DNA_THRESHOLD);
 

+     value = slapi_entry_attr_get_charptr(e, DNA_THRESHOLD);
 

      if (value) {
 

          entry->threshold = strtoull(value, 0, 0);
 

  
@@ -1258,6 +1261,8 @@ 
 

              slapi_log_err(SLAPI_LOG_ERR, DNA_PLUGIN_SUBSYSTEM,
 

                            "----------> %s too low, setting to [%s]\n", DNA_THRESHOLD, value);
 

          }
 

+ 

+         slapi_ch_free_string(&value);
 

      } else {
 

          entry->threshold = 1;
 

      }
 
@@ -1266,9 +1271,10 @@ 
 

                    "dna_parse_config_entry - %s [%" PRIu64 "]\n", DNA_THRESHOLD,
 

                    entry->threshold);
 

  

-     value = (char *)slapi_entry_attr_get_ref(e, DNA_RANGE_REQUEST_TIMEOUT);
 

+     value = slapi_entry_attr_get_charptr(e, DNA_RANGE_REQUEST_TIMEOUT);
 

      if (value) {
 

          entry->timeout = strtoull(value, 0, 0);
 

+         slapi_ch_free_string(&value);
 

      } else {
 

          entry->timeout = DNA_DEFAULT_TIMEOUT;
 

      }
 
@@ -1277,7 +1283,7 @@ 
 

                    "dna_parse_config_entry - %s [%" PRIu64 "]\n", DNA_RANGE_REQUEST_TIMEOUT,
 

                    entry->timeout);
 

  

-     value = (char *)slapi_entry_attr_get_ref(e, DNA_NEXT_RANGE);
 

+     value = slapi_entry_attr_get_charptr(e, DNA_NEXT_RANGE);
 

      if (value) {
 

          char *p = NULL;
 

  
@@ -1323,6 +1329,8 @@ 
 

                            DNA_NEXT_RANGE, entry->dn);
 

              ret = DNA_FAILURE;
 

          }
 

+ 

+         slapi_ch_free_string(&value);
 

      }
 

  

      /* If we were only called to validate config, we can
 
@@ -2287,7 +2295,7 @@ 
 

      int multitype = 0;
 

      int result, status;
 

      PRUint64 tmpval, sval, i;
 

-     const char *strval = NULL;
 

+     char *strval = NULL;
 

  

      /* check if the config is already out of range */
 

      if (config_entry->nextval > config_entry->maxval) {
 
@@ -2411,7 +2419,7 @@ 
 

           * type from the list of types directly. */
 

          sval = 0;
 

          for (i = 0; NULL != entries[i]; i++) {
 

-             strval = slapi_entry_attr_get_ref(entries[i], config_entry->types[0]);
 

+             strval = slapi_entry_attr_get_charptr(entries[i], config_entry->types[0]);
 

              errno = 0;
 

              sval = strtoull(strval, 0, 0);
 

              if (errno) {
 
@@ -2419,6 +2427,7 @@ 
 

                  status = LDAP_OPERATIONS_ERROR;
 

                  goto cleanup;
 

              }
 

+             slapi_ch_free_string(&strval);
 

  

              if (tmpval != sval)
 

                  break;
 
@@ -2444,6 +2453,7 @@ 
 

  

  cleanup:
 

      slapi_ch_free_string(&filter);
 

+     slapi_ch_free_string(&strval);
 

      slapi_free_search_results_internal(pb);
 

      slapi_pblock_destroy(pb);
 

  
@@ -2964,7 +2974,7 @@ 
 

      char *attrs[6];
 

      char *filter = NULL;
 

      char *bind_cred = NULL;
 

-     const char *transport = NULL;
 

+     char *transport = NULL;
 

      Slapi_Entry **entries = NULL;
 

      int ret = LDAP_OPERATIONS_ERROR;
 

  
@@ -3049,7 +3059,7 @@ 
 

          *bind_dn = slapi_entry_attr_get_charptr(entries[0], DNA_REPL_BIND_DN);
 

          *bind_method = slapi_entry_attr_get_charptr(entries[0], DNA_REPL_BIND_METHOD);
 

          bind_cred = slapi_entry_attr_get_charptr(entries[0], DNA_REPL_CREDS);
 

-         transport = slapi_entry_attr_get_ref(entries[0], DNA_REPL_TRANSPORT);
 

+         transport = slapi_entry_attr_get_charptr(entries[0], DNA_REPL_TRANSPORT);
 

          *port = slapi_entry_attr_get_int(entries[0], DNA_REPL_PORT);
 

  

          /* Check if we should use SSL */
 
@@ -3106,10 +3116,11 @@ 
 

      ret = 0;
 

  

  bail:
 

-     slapi_ch_free_string(&bind_cred);
 

+     slapi_ch_free_string(&transport);
 

      slapi_ch_free_string(&filter);
 

      slapi_sdn_free(&range_sdn);
 

      slapi_ch_free_string(&replica_dn);
 

+     slapi_ch_free_string(&bind_cred);
 

      slapi_free_search_results_internal(pb);
 

      slapi_pblock_destroy(pb);
 

  
@@ -3468,21 +3479,24 @@ 
 

                   * for types where the magic value is set.  We do not
 

                   * generate a value for missing types. */
 

                  for (i = 0; config_entry->types && config_entry->types[i]; i++) {
 

-                     value = (char *)slapi_entry_attr_get_ref(e, config_entry->types[i]);
 

+                     value = slapi_entry_attr_get_charptr(e, config_entry->types[i]);
 

                      if (value) {
 

                          if (config_entry->generate == NULL || !slapi_UTF8CASECMP(config_entry->generate, value)) {
 

                              slapi_ch_array_add(&types_to_generate, slapi_ch_strdup(config_entry->types[i]));
 

                          }
 

+                         slapi_ch_free_string(&value);
 

                      }
 

                  }
 

              } else {
 

                  /* For a single type range, we generate the value if
 

                   * the magic value is set or if the type is missing. */
 

-                 value = (char *)slapi_entry_attr_get_ref(e, config_entry->types[0]);
 

+                 value = slapi_entry_attr_get_charptr(e, config_entry->types[0]);
 

+ 

                  if ((config_entry->generate == NULL) || (0 == value) ||
 

                      (value && !slapi_UTF8CASECMP(config_entry->generate, value))) {
 

                      slapi_ch_array_add(&types_to_generate, slapi_ch_strdup(config_entry->types[0]));
 

                  }
 

+                 slapi_ch_free_string(&value);
 

              }
 

  

              if (types_to_generate && types_to_generate[0]) {
 
@@ -4139,24 +4153,28 @@ 
 

                       * for types where the magic value is set.  We do not
 

                       * generate a value for missing types. */
 

                      for (i = 0; config_entry->types && config_entry->types[i]; i++) {
 

-                         value = (char *)slapi_entry_attr_get_ref(e, config_entry->types[i]);
 

+                         value = slapi_entry_attr_get_charptr(e, config_entry->types[i]);
 

+ 

                          if (value && !slapi_UTF8CASECMP(value, DNA_NEEDS_UPDATE)) {
 

                              slapi_ch_array_add(&types_to_generate,
 

                                                 slapi_ch_strdup(config_entry->types[i]));
 

                              /* Need to remove DNA_NEEDS_UPDATE */
 

                              slapi_entry_attr_delete(e, config_entry->types[i]);
 

                          }
 

+                         slapi_ch_free_string(&value);
 

                      }
 

                  } else {
 

                      /* For a single type range, we generate the value if
 

                       * the magic value is set or if the type is missing. */
 

-                     value = (char *)slapi_entry_attr_get_ref(e, config_entry->types[0]);
 

+                     value = slapi_entry_attr_get_charptr(e, config_entry->types[0]);
 

+ 

                      if (0 == value || (value && !slapi_UTF8CASECMP(value, DNA_NEEDS_UPDATE))) {
 

                          slapi_ch_array_add(&types_to_generate,
 

                                             slapi_ch_strdup(config_entry->types[0]));
 

                          /* Need to remove DNA_NEEDS_UPDATE */
 

                          slapi_entry_attr_delete(e, config_entry->types[0]);
 

                      }
 

+                     slapi_ch_free_string(&value);
 

                  }
 

              } else {
 

                  /* check mods for DNA_NEEDS_UPDATE*/

file modified
+2 -2 
@@ -533,7 +533,7 @@ 
 

      }
 

  

      /* Load the origin filter */
 

-     value = (char *)slapi_entry_attr_get_ref(e, MEP_FILTER_TYPE);
 

+     value = (char *)slapi_entry_attr_get_charptr(e, MEP_FILTER_TYPE);
 

      if (value) {
 

          /* Convert to a Slapi_Filter to improve performance. */
 

          if (NULL == (entry->origin_filter = slapi_str2filter(value))) {
 
@@ -544,7 +544,7 @@ 
 

                            MEP_FILTER_TYPE, slapi_sdn_get_dn(entry->sdn), value);
 

              ret = -1;
 

          }
 

- 

+         slapi_ch_free_string(&value);
 

          if (ret != 0) {
 

              goto bail;
 

          }

file modified
+2 -1 
@@ -495,7 +495,7 @@ 
 

      }
 

  

      /* Validate filter by converting to Slapi_Filter */
 

-     pam_filter_str = (char *)slapi_entry_attr_get_ref(e, PAMPT_FILTER_ATTR);
 

+     pam_filter_str = (char *)slapi_entry_attr_get_charptr(e, PAMPT_FILTER_ATTR);
 

      if (pam_filter_str) {
 

          pam_filter = slapi_str2filter(pam_filter_str);
 

          if (pam_filter == NULL) {
 
@@ -524,6 +524,7 @@ 
 

      slapi_ch_array_free(includes);
 

      includes = NULL;
 

      slapi_filter_free(pam_filter, 1);
 

+     slapi_ch_free_string(&pam_filter_str);
 

  

      return rc;
 

  }

file modified
+2 -1 
@@ -3440,7 +3440,7 @@ 
 

          cn = slapi_entry_attr_get_ref(e, "ntuserdomainid");
 

      }
 

  

-     guid = (char *)slapi_entry_attr_get_ref(e, "ntUniqueId");
 

+     guid = slapi_entry_attr_get_charptr(e, "ntUniqueId");

I think there is a risk of double free in the later call 'dash_guid' that can call slapi_ch_free_string 

      if (guid) {
 

          /* the GUID is in a different form in the tombstone DN, so
 

           * we need to transform it from the way we store it. */
 
@@ -3476,6 +3476,7 @@ 
 

                        slapi_entry_get_dn(e));
 

          rc = 1;
 

      }
 

+     slapi_ch_free_string(&guid);
 

  

      return rc;
 

  }

file modified
+5 -2 
@@ -1154,7 +1154,7 @@ 
 

          char *parent = NULL;
 

  

          /* Get the filter and retrieve the filter attribute */
 

-         filter_attr_value = (char *)slapi_entry_attr_get_ref(role_entry, ROLE_FILTER_ATTR_NAME);
 

+         filter_attr_value = (char *)slapi_entry_attr_get_charptr(role_entry, ROLE_FILTER_ATTR_NAME);
 

          if (filter_attr_value == NULL) {
 

              /* Means probably no attribute or no value there */
 

              slapi_ch_free((void **)&this_role);
 
@@ -1197,6 +1197,7 @@ 
 

                                ROLE_FILTER_ATTR_NAME, filter_attr_value,
 

                                ROLE_FILTER_ATTR_NAME);
 

                  slapi_ch_free((void **)&this_role);
 

+                 slapi_ch_free_string(&filter_attr_value);
 

                  return SLAPI_ROLE_ERROR_FILTER_BAD;
 

              }
 

          }
 
@@ -1208,6 +1209,7 @@ 
 

          if (filter == NULL) {
 

              /* An error has occured */
 

              slapi_ch_free((void **)&this_role);
 

+             slapi_ch_free_string(&filter_attr_value);
 

              return SLAPI_ROLE_ERROR_FILTER_BAD;
 

          }
 

          if (roles_check_filter(filter)) {
 
@@ -1218,11 +1220,12 @@ 
 

                            filter_attr_value,
 

                            ROLE_FILTER_ATTR_NAME);
 

              slapi_ch_free((void **)&this_role);
 

+             slapi_ch_free_string(&filter_attr_value);
 

              return SLAPI_ROLE_ERROR_FILTER_BAD;
 

          }
 

          /* Store on the object */
 

          this_role->filter = filter;
 

- 

+         slapi_ch_free_string(&filter_attr_value);
 

          break;
 

      }

file modified
+2 -1 
@@ -573,13 +573,14 @@ 
 

                  }
 

  

                  /* And copy the reason from e */
 

-                 reason = (char *)slapi_entry_attr_get_ref(e, "nsds5ReplConflict");
 

+                 reason = slapi_entry_attr_get_charptr(e, "nsds5ReplConflict");
 

                  if (reason) {
 

                      if (!slapi_entry_attr_hasvalue(addingentry->ep_entry, "nsds5ReplConflict", reason)) {
 

                          slapi_entry_add_string(addingentry->ep_entry, "nsds5ReplConflict", reason);
 

                          slapi_log_err(SLAPI_LOG_REPL, "ldbm_back_add", "Resurrection of %s - Added Conflict reason %s\n",
 

                                        dn, reason);
 

                      }
 

+                     slapi_ch_free_string(&reason);
 

                  }
 

                  /* Clear the Tombstone Flag in the entry */
 

                  slapi_entry_clear_flag(addingentry->ep_entry, SLAPI_ENTRY_FLAG_TOMBSTONE);

file modified
+5 -3 
@@ -49,6 +49,7 @@ 
 

      }
 

  

      if (index_name != NULL) {
 

+         slapi_ch_free_string(index_name);
 

          *index_name = slapi_ch_strdup(attrValue->bv_val);
 

      }
 

  
@@ -113,7 +114,7 @@ 
 

                                          void *arg)
 

  {
 

      ldbm_instance *inst = (ldbm_instance *)arg;
 

-     char *index_name;
 

+     char *index_name = NULL;
 

  

      returntext[0] = '\0';
 

      *returncode = ldbm_index_parse_entry(inst, e, "from DSE add", &index_name);
 
@@ -129,7 +130,7 @@ 
 

              PR_ASSERT(ai != NULL);
 

              ai->ai_indexmask |= INDEX_OFFLINE;
 

          }
 

-         slapi_ch_free((void **)&index_name);
 

+         slapi_ch_free_string(&index_name);
 

          return SLAPI_DSE_CALLBACK_OK;
 

      } else {
 

          return SLAPI_DSE_CALLBACK_ERROR;
 
@@ -358,7 +359,7 @@ 
 

      int rc = LDAP_SUCCESS;
 

      struct attrinfo *ai = NULL;
 

  

-     index_name = (char *)slapi_entry_attr_get_ref(e, "cn");
 

+     index_name = slapi_entry_attr_get_charptr(e, "cn");
 

      if (index_name) {
 

          ainfo_get(inst->inst_be, index_name, &ai);
 

      }
 
@@ -373,6 +374,7 @@ 
 

          PR_ASSERT(ai != NULL);
 

          ai->ai_indexmask &= ~INDEX_OFFLINE;
 

      }
 

+     slapi_ch_free_string(&index_name);
 

      return rc;
 

  }

Description: Some crashes were found in upstream testing. Needed
to revert slapi_entry_attr_get_ref() back to slapi_entry_attr_get_charptr()

relates: https://pagure.io/389-ds-base/issue/50620

cast is not required

cast not required

I think there is a risk of double free in the later call 'dash_guid' that can call slapi_ch_free_string

I think there is a risk of double free in the later call 'dash_guid' that can call slapi_ch_free_string

Well this is how the original code was, and if it is freed the pointer "should" be set to NULL, so the double free "should" not occur.

rebased onto f43477f88686b465b93a478710bd92638110dc79
6 years ago

rebased onto 988eb0159f84d8e49fee6c5c4101b27cdc9c357f
6 years ago

ahoy, upstream testing here :D the latest version of the patch looks good so far.

This pointer will be overwritten if ldbm_index_parse_entry is called.
I think ldbm_index_parse_entry should free it before overwriting it but then it ldbm_instance_index_config_add_callback should init index_name=NULL;

This pointer will be overwritten if ldbm_index_parse_entry is called.
I think ldbm_index_parse_entry should free it before overwriting it but then it ldbm_instance_index_config_add_callback should init index_name=NULL;

This is a revert to the original code, but I see what you saying. I'll fix this as well.

rebased onto c54a4310a617ef752dacdf5e2a3ce45e4c348f72
6 years ago

rebased onto fce5c6c
6 years ago

This pointer will be overwritten if ldbm_index_parse_entry is called.
I think ldbm_index_parse_entry should free it before overwriting it but then it ldbm_instance_index_config_add_callback should init index_name=NULL;

Changes applied, please review...

Thanks Mark. The fix looks good to me. ACK

Pull-Request has been merged by mreynolds
6 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/3676

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
5 years ago
Metadata
None
No Tags
Changes Summary 8
+4 -2
file changed
ldap/servers/plugins/automember/automember.c
+33 -15
file changed
ldap/servers/plugins/dna/dna.c
+2 -2
file changed
ldap/servers/plugins/mep/mep.c
+2 -1
file changed
ldap/servers/plugins/pam_passthru/pam_ptconfig.c
+2 -1
file changed
ldap/servers/plugins/replication/windows_protocol_util.c
+5 -2
file changed
ldap/servers/plugins/roles/roles_cache.c
+2 -1
file changed
ldap/servers/slapd/back-ldbm/ldbm_add.c
+5 -3
file changed
ldap/servers/slapd/back-ldbm/ldbm_index_config.c
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.