[dm-devel] [PATCH 4/4] libmultipath: avoid cleanup __attribute__ with cancellation points

[Benjamin Marzinski <bmarzins@redhat.com>]

the cleanup __attribute__ doesn't get run when a thread is cancelled, so
it is only safe in cases where there aren't pthreads or no cancellation
points happen in the code block after the variable needs cleaning up.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c                 |  6 +--
 libmultipath/dmparser.c                  |  2 +-
 libmultipath/generic.c                   |  4 +-
 libmultipath/prioritizers/weightedpath.c |  6 ++-
 multipathd/cli_handlers.c                | 11 ++++--
 multipathd/main.c                        | 49 ++++++++++++++----------
 6 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e5249fc1..25708257 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1075,7 +1075,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
 	int ret = CP_FAIL;
 	int k, i, r;
 	int is_daemon = (cmd == CMD_NONE) ? 1 : 0;
-	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	char *params = NULL;
 	struct multipath * mpp;
 	struct path * pp1 = NULL;
 	struct path * pp2;
@@ -1208,6 +1208,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
 			remove_map(mpp, vecs->pathvec, NULL);
 			continue;
 		}
+		pthread_cleanup_push(cleanup_free_ptr, &params);
 
 		if (cmd == CMD_DRY_RUN)
 			mpp->action = ACT_DRY_RUN;
@@ -1216,8 +1217,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
 				      force_reload == FORCE_RELOAD_YES ? 1 : 0);
 
 		r = domap(mpp, params, is_daemon);
-		free(params);
-		params = NULL;
+		pthread_cleanup_pop(1);
 
 		if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
 			condlog(3, "%s: domap (%u) failure "
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 44650aaa..066a33af 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -94,8 +94,8 @@ int assemble_map(struct multipath *mp, char **params)
 		}
 	}
 
+	condlog(4, "%s: assembled map [%s]", mp->alias, get_strbuf_str(&buff));
 	*params = steal_strbuf_str(&buff);
-	condlog(4, "%s: assembled map [%s]", mp->alias, *params);
 	r = 0;
 out:
 	pthread_cleanup_pop(1);
diff --git a/libmultipath/generic.c b/libmultipath/generic.c
index 3e2268e6..cdee21d9 100644
--- a/libmultipath/generic.c
+++ b/libmultipath/generic.c
@@ -24,11 +24,12 @@ int generic_style(const struct gen_multipath* gm, struct strbuf *buf,
 		  __attribute__((unused)) int verbosity)
 {
 	struct strbuf tmp = STRBUF_INIT;
-	char *alias_buf __attribute__((cleanup(cleanup_charp)));
+	char *alias_buf = NULL;
 	const char *wwid_buf;
 	int ret;
 
 	pthread_cleanup_push_cast(reset_strbuf, &tmp);
+	pthread_cleanup_push(cleanup_free_ptr, &alias_buf);
 	gm->ops->snprint(gm, &tmp, 'n');
 	alias_buf = steal_strbuf_str(&tmp);
 	gm->ops->snprint(gm, &tmp, 'w');
@@ -37,5 +38,6 @@ int generic_style(const struct gen_multipath* gm, struct strbuf *buf,
 	ret = print_strbuf(buf, "%%n %s[%%G]:%%d %%s",
 			   strcmp(alias_buf, wwid_buf) ? "(%w) " : "");
 	pthread_cleanup_pop(1);
+	pthread_cleanup_pop(1);
 	return ret;
 }
diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
index df700bf3..02d40a3d 100644
--- a/libmultipath/prioritizers/weightedpath.c
+++ b/libmultipath/prioritizers/weightedpath.c
@@ -64,7 +64,7 @@ build_wwn_path(struct path *pp, struct strbuf *buf)
 int prio_path_weight(struct path *pp, char *prio_args)
 {
 	struct strbuf path = STRBUF_INIT;
-	char *arg __attribute__((cleanup(cleanup_charp))) = NULL;
+	char *arg = NULL;
 	char *temp, *regex, *prio;
 	char split_char[] = " \t";
 	int priority = DEFAULT_PRIORITY, path_found = 0;
@@ -77,11 +77,12 @@ int prio_path_weight(struct path *pp, char *prio_args)
 	arg = strdup(prio_args);
 	temp = arg;
 
+	pthread_cleanup_push(cleanup_free_ptr, &arg);
 	regex = get_next_string(&temp, split_char);
 
 	/* Return default priority if the argument is not parseable */
 	if (!regex) {
-		return priority;
+		goto out;
 	}
 
 	pthread_cleanup_push_cast(reset_strbuf, &path);
@@ -123,6 +124,7 @@ int prio_path_weight(struct path *pp, char *prio_args)
 		}
 	}
 out:
+	pthread_cleanup_pop(1);
 	pthread_cleanup_pop(1);
 	return priority;
 }
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index ddc807a1..2d1c5cfe 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -812,8 +812,9 @@ cli_reload(void *v, struct strbuf *reply, void *data)
 static int resize_map(struct multipath *mpp, unsigned long long size,
 	       struct vectors * vecs)
 {
-	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	char *params = NULL;
 	unsigned long long orig_size = mpp->size;
+	int r = 1;
 
 	mpp->size = size;
 	update_mpp_paths(mpp, vecs->pathvec);
@@ -823,15 +824,19 @@ static int resize_map(struct multipath *mpp, unsigned long long size,
 		mpp->size = orig_size;
 		return 1;
 	}
+	pthread_cleanup_push(cleanup_free_ptr, &params);
 	mpp->action = ACT_RESIZE;
 	mpp->force_udev_reload = 1;
 	if (domap(mpp, params, 1) == DOMAP_FAIL) {
 		condlog(0, "%s: failed to resize map : %s", mpp->alias,
 			strerror(errno));
 		mpp->size = orig_size;
-		return 1;
+		goto out;
 	}
-	return 0;
+	r = 0;
+out:
+	pthread_cleanup_pop(1);
+	return r;
 }
 
 static int
diff --git a/multipathd/main.c b/multipathd/main.c
index ba52d393..2517b541 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -589,9 +589,9 @@ static int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
 	int retries = 3;
-	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	char *params = NULL;
 	struct path *pp;
-	int i;
+	int i, r;
 
 retry:
 	condlog(4, "%s: updating new map", mpp->alias);
@@ -622,10 +622,11 @@ retry:
 		retries = -1;
 		goto fail;
 	}
-	if (domap(mpp, params, 1) == DOMAP_FAIL && retries-- > 0) {
+	pthread_cleanup_push(cleanup_free_ptr, &params);
+	r = domap(mpp, params, 1);
+	pthread_cleanup_pop(1);
+	if (r == DOMAP_FAIL && retries-- > 0) {
 		condlog(0, "%s: map_udate sleep", mpp->alias);
-		free(params);
-		params = NULL;
 		sleep(1);
 		goto retry;
 	}
@@ -1185,7 +1186,7 @@ int
 ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 {
 	struct multipath * mpp;
-	char *params __attribute((cleanup(cleanup_charp))) = NULL;
+	char *params = NULL;
 	int retries = 3;
 	int start_waiter = 0;
 	int ret;
@@ -1277,6 +1278,7 @@ rescan:
 	/*
 	 * reload the map for the multipath mapped device
 	 */
+	pthread_cleanup_push(cleanup_free_ptr, &params);
 	ret = domap(mpp, params, 1);
 	while (ret == DOMAP_RETRY && retries-- > 0) {
 		condlog(0, "%s: retry domap for addition of new "
@@ -1284,6 +1286,7 @@ rescan:
 		sleep(1);
 		ret = domap(mpp, params, 1);
 	}
+	pthread_cleanup_pop(1);
 	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
 		condlog(0, "%s: failed in domap for addition of new "
 			"path %s", mpp->alias, pp->dev);
@@ -1294,8 +1297,6 @@ rescan:
 			condlog(0, "%s: ev_add_path sleep", mpp->alias);
 			sleep(1);
 			update_mpp_paths(mpp, vecs->pathvec);
-			free(params);
-			params = NULL;
 			goto rescan;
 		}
 		else if (mpp->action == ACT_RELOAD)
@@ -1356,8 +1357,9 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 {
 	struct multipath * mpp;
 	int i, retval = REMOVE_PATH_SUCCESS;
-	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	char *params = NULL;
 
+	pthread_cleanup_push(cleanup_free_ptr, &params);
 	/*
 	 * avoid referring to the map of an orphaned path
 	 */
@@ -1376,7 +1378,8 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		if (update_mpp_paths(mpp, vecs->pathvec)) {
 			condlog(0, "%s: failed to update paths",
 				mpp->alias);
-			goto fail;
+			retval = REMOVE_PATH_MAP_ERROR;
+			goto out;
 		}
 
 		/*
@@ -1398,7 +1401,8 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		if (setup_map(mpp, &params, vecs)) {
 			condlog(0, "%s: failed to setup map for"
 				" removal of path %s", mpp->alias, pp->dev);
-			goto fail;
+			retval = REMOVE_PATH_MAP_ERROR;
+			goto out;
 		}
 
 		if (mpp->wait_for_udev) {
@@ -1431,8 +1435,10 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			/* setup_multipath will free the path
 			 * regardless of whether it succeeds or
 			 * fails */
-			if (setup_multipath(vecs, mpp))
-				return REMOVE_PATH_MAP_ERROR;
+			if (setup_multipath(vecs, mpp)) {
+				retval = REMOVE_PATH_MAP_ERROR;
+				goto fail;
+			}
 			sync_map_state(mpp);
 
 			condlog(2, "%s: path removed from map %s",
@@ -1445,13 +1451,14 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		free_path(pp);
 	}
 out:
-	return retval;
-
+	if (retval == REMOVE_PATH_MAP_ERROR) {
+		condlog(0, "%s: error removing path. removing map %s", pp->dev,
+			mpp->alias);
+		remove_map_and_stop_waiter(mpp, vecs);
+	}
 fail:
-	condlog(0, "%s: error removing path. removing map %s", pp->dev,
-		mpp->alias);
-	remove_map_and_stop_waiter(mpp, vecs);
-	return REMOVE_PATH_MAP_ERROR;
+	pthread_cleanup_pop(1);
+	return retval;
 }
 
 int
@@ -2067,7 +2074,7 @@ int update_prio(struct path *pp, int refresh_all)
 static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
 		      int is_daemon)
 {
-	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	char *params = NULL;
 	struct path *pp;
 	int i, r;
 
@@ -2089,9 +2096,11 @@ static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
 		condlog(0, "%s: failed to setup map", mpp->alias);
 		return 1;
 	}
+	pthread_cleanup_push(cleanup_free_ptr, &params);
 	select_action(mpp, vecs->mpvec, 1);
 
 	r = domap(mpp, params, is_daemon);
+	pthread_cleanup_pop(1);
 	if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
 		condlog(3, "%s: domap (%u) failure "
 			"for reload map", mpp->alias, r);
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel