[dm-devel] [PATCH 2/5] libmultipath fix NULL dereference in select_action

[lixiaokeng]

I got a multipath segfault while running iscsi login/logout and following scripts in parallel:

#!/bin/bash
interval=1
while true
do
              multipath -F &> /dev/null
              multipath -r &> /dev/null
              multipath -v2 &> /dev/null
              multipath -ll &> /dev/null
              sleep $interval
done

This is the debuginfo:
Core was generated by `multipath -v2'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007fc5c5c8dedf in select_action (mpp=0x55a94f1f8980, curmp=<optimized out>,
    force_reload=<optimized out>) at configure.c:709
709		if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) { {

(gdb) bt
#0  0x00007fc5c5c8dedf in select_action (mpp=0x55a94f1f8980, curmp=<optimized out>,
    force_reload=<optimized out>) at configure.c:709
#1  0x00007fc5c5c8fd0f in coalesce_paths (vecs=0x7ffff1c1c220, newmp=0x0, refwwid=0x0,
    force_reload=0, cmd=CMD_CREATE) at configure.c:1090
#2  0x000055a94e9f524d in configure (devpath=<optimized out>, dev_type=DEV_NONE,
    cmd=CMD_CREATE, conf=0x55a94f1b92d0) at main.c:757
#3  main (argc=<optimized out>, argv=<optimized out>) at main.c:1100
(gdb) p *cmpp
$1 = {
  wwid = "3600140566411a6d4873434fba988066f\000\070\060\066\066f", '\000' <repeats 88 times>,
  ...
  paths = 0x0, pg = 0x0, dmi = 0x55a94f22c3f0, alias = 0x55a94f205fb0 "mpathc",
  alias_prefix = 0x0, selector = 0x0, features = 0x0, hwhandler = 0x0, mpe = 0x0,
  hwe = 0x0, waiter = 0, stat_switchgroup = 0, stat_path_failures = 0, stat_map_loads = 0,
  ...
  generic_mp = {ops = 0x7fc5c5cada40 <dm_gen_multipath_ops>}}

There are many NULL pointers in cmpp such as selector, features, hwhandler and so on.
Here these on mpp is not NULL  but we can not be sure that won't be in mpp, so we
add checking pointers of both mpp and cmpp before using them.

The changes like "mpp->features != cmpp->features" make sure that mpp->action is not
set to ACT_RELOAD when they both equal NULL. Other changes check pointers don't equal
NULL. When only one is NULL, we think there is some difference between mpp and cmpp,
so mpp->action should be set to ACT_RELOAD.

Signed-off-by: Lixiaokeng <lixiaokeng at huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
---
 libmultipath/configure.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 96c79610..e02e7ef4 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -726,16 +726,20 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}

 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
+	    mpp->features != cmpp->features &&
+	    (!mpp->features || !cmpp->features ||
 	    !!strstr(mpp->features, "queue_if_no_path") !=
-	    !!strstr(cmpp->features, "queue_if_no_path")) {
+	    !!strstr(cmpp->features, "queue_if_no_path"))) {
 		mpp->action =  ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (no_path_retry change)",
 			mpp->alias);
 		return;
 	}
-	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
+	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON || !cmpp->hwhandler ||
 	     strcmp(cmpp->hwhandler, "0") == 0) &&
-	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
+	     mpp->hwhandler != cmpp->hwhandler &&
+	     (!mpp->hwhandler || !cmpp->hwhandler||
+	     strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
 	     strncmp(cmpp->hwhandler, mpp->hwhandler,
 		    strlen(mpp->hwhandler)))) {
 		mpp->action = ACT_RELOAD;
@@ -745,8 +749,10 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}

 	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
+	    mpp->features != cmpp->features &&
+	    (!mpp->features || !cmpp->features ||
 	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
-	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
+	    !!strstr(cmpp->features, "retain_attached_hw_handler")) &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
@@ -754,8 +760,16 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 		return;
 	}

-	cmpp_feat = STRDUP(cmpp->features);
-	mpp_feat = STRDUP(mpp->features);
+	if (!cmpp->features ) {
+		cmpp_feat = NULL;
+	} else {
+		cmpp_feat = STRDUP(cmpp->features);
+	}
+	if (!mpp->features ) {
+		mpp_feat = NULL;
+	} else {
+		mpp_feat = STRDUP(mpp->features);
+	}
 	if (cmpp_feat && mpp_feat) {
 		remove_feature(&mpp_feat, "queue_if_no_path");
 		remove_feature(&mpp_feat, "retain_attached_hw_handler");
@@ -770,8 +784,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	FREE(cmpp_feat);
 	FREE(mpp_feat);

-	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
-		    strlen(mpp->selector))) {
+	if (mpp->selector != cmpp->selector && (!cmpp->selector || !mpp->selector ||
+		    strncmp(cmpp->selector, mpp->selector,strlen(mpp->selector)))) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (selector change)",
 			mpp->alias);
--