#49688 Ticket 49669 - Invalid cachemem size can crash the server during a restore
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket49669  into  master

@@ -6566,6 +6566,16 @@ 

                          goto error_out;

                      }

  

+                     if (inst->inst_parent_dir_name == NULL) {

+                         slapi_log_err(SLAPI_LOG_ERR, "dblayer_restore",

+                                       "Parent directory is not set, aborting restore\n");

+                         if (task) {

+                             slapi_task_log_notice(task, "dblayer_restore - Parent directory is not set, aborting restore\n");

+                         }

+                         PR_CloseDir(dirhandle);

+                         return_value = LDAP_UNWILLING_TO_PERFORM;

+                         goto error_out;

+                     }

                      if (slapd_comp_path(src_dir, inst->inst_parent_dir_name) == 0) {

                          slapi_log_err(SLAPI_LOG_ERR,

                                        "dblayer_restore", "Backup dir %s and target dir %s "

file modified
+13 -3
@@ -1178,7 +1178,7 @@ 

  }

  

  /*

-  * Compare 2 pathes

+  * Compare 2 paths

   * Paths could contain ".", "..", "//" in the path, thus normalize them first.

   * One or two of the paths could be a relative path.

   */
@@ -1186,9 +1186,19 @@ 

  slapd_comp_path(char *p0, char *p1)

  {

      int rval = 0;

-     char *norm_p0 = rel2abspath(p0);

-     char *norm_p1 = rel2abspath(p1);

+     char *norm_p0;

+     char *norm_p1;

  

+     /*

+      * Neither path should be NULL, but it's possible when bad thing happen.

+      * Return 0 which triggers an error in the caller

+      */

+     if (p0 == NULL || p1 == NULL){

+         return 0;

+     }

+ 

+     norm_p0 = rel2abspath(p0);

+     norm_p1 = rel2abspath(p1);

      rval = strcmp(norm_p0, norm_p1);

      slapi_ch_free_string(&norm_p0);

      slapi_ch_free_string(&norm_p1);

Bug Description: If you manually set the dbcachememsize to something larger than
a uint64_t the server can crash from a NULL pointer being
dereferenced.

Fix Description: Catch the NULL pointer before it is dereferenced, and abort the
restore.

https://pagure.io/389-ds-base/issue/49669

Reviewed by: ?

Ack from me, but I think the commit message is misleading? This doesn't seemto be uint64_t related (unless I'm missing something :) )

@mreynolds , the fix looks good to me as well but I do not understand the relation between cache memsize and checking that a path is not NULL. Did I miss something ?

Yeah this is an odd one. What happens is that when the cachememsize value exceeds the int type, it was previously incorrectly thought to be a long not a unint64 (see ldbm_config_set), then the code takes a path where some of the inst elements are not set, and then later get dereferenced.

This PR below fixed the int type checking to be uint64_t for cache sizes

https://pagure.io/389-ds-base/pull-request/49674

So now the issue only happens if you use a value larger than what uint64 can hold. Before this pull request list above valid caches sizes were seen as too large, and we got into this situation where we bailed out of the code path before setting things like parent dir, etc. There was no other good way to fix this because the the way the backend config checking works is mutli phased and "anonymous" in some areas. In ldbm_config_set we don't know what the attribute is, but just know what int type we are expecting. We bail out here before actually calling the "set" function if we exceed the range of the "expected" type, and this "bailing out" leaves the inst struct is an odd state that later leads to NULL dereferences - hence my fix. So this is really a corner case, but requires this fix to solve it. I hope that clears thing up a little (it's a bit confusing I know).

Thanks Mark for these explanations. It make sense, in addition to @firstyear you also got my ACK.

rebased onto 15ff2e3

5 years ago

Pull-Request has been merged by mreynolds

5 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/2747

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