[dm-devel] [PATCH v3 3/6] multipathd: fix ev_remove_path return code handling

[Martin Wilck]

On Fri, 2021-05-14 at 15:10 -0500, Benjamin Marzinski wrote:
> When ev_remove_path() returned success, callers assumed that the path
> (and possibly the map) had been removed.  When ev_remove_path()
> returned
> failure, callers assumed that the path had not been removed. However,
> the path could be removed on both success or failure. This could
> cause
> callers to dereference the path after it was removed.
> 
> To deal with this, make ev_remove_path() return a different symbolic
> value for each outcome, and make the callers react appropriately for
> the different values. Found by coverity.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>

Almost perfect - I still have one nit below.

> ---
>  multipathd/cli_handlers.c | 14 +++++++++++--
>  multipathd/main.c         | 41 ++++++++++++++++++++-----------------
> --
>  multipathd/main.h         | 14 +++++++++++++
>  3 files changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 1de6ad8e..1462ea84 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -752,7 +752,8 @@ cli_add_path (void * v, char ** reply, int * len,
> void * data)
>                                 /* Have the checker reinstate this
> path asap */
>                                 pp->tick = 1;
>                                 return 0;
> -                       } else if (!ev_remove_path(pp, vecs, true))
> +                       } else if (ev_remove_path(pp, vecs, true) &
> +                                  REMOVE_PATH_SUCCESS)
>                                 /* Path removed in ev_remove_path()
> */
>                                 pp = NULL;
>                         else {
> @@ -813,6 +814,7 @@ cli_del_path (void * v, char ** reply, int * len,
> void * data)
>         struct vectors * vecs = (struct vectors *)data;
>         char * param = get_keyparam(v, PATH);
>         struct path *pp;
> +       int ret;
>  
>         param = convert_dev(param, 1);
>         condlog(2, "%s: remove path (operator)", param);
> @@ -821,7 +823,15 @@ cli_del_path (void * v, char ** reply, int *
> len, void * data)
>                 condlog(0, "%s: path already removed", param);
>                 return 1;
>         }
> -       return ev_remove_path(pp, vecs, 1);
> +       ret = ev_remove_path(pp, vecs, 1);
> +       if (ret == REMOVE_PATH_DELAY) {
> +               *reply = strdup("delayed\n");
> +               *len = strlen(*reply) + 1;
> +       } else if (ret == REMOVE_PATH_MAP_ERROR) {
> +               *reply = strdup("map reload error. removed\n");
> +               *len = strlen(*reply) + 1;
> +       }

So you went for the new replies. Fine with me, let's see if anyone
complains. But you should check the result of strdup() before calling
strlen():

                 *reply = strdup("delayed\n");
                 if (*reply)
                        *len = strlen(*reply) + 1;                    
                 else {
                        *len = 0;
                        ret = REMOVE_PATH_FAILURE;
                 }

Regards,
Martin