[PATCH] libmultipath: make vector_foreach_slot_backwards work as expected

[PATCH] libmultipath: make vector_foreach_slot_backwards work as expected

[Benjamin Marzinski]

All of the code that uses vector_foreach_slot_backwards() treats "i" as
the index of the entry "p", but the way it was coded, that wasn't the
case. "i" was the number of the entry counting from 1, not 0.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/vector.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/vector.h b/libmultipath/vector.h
index 41d2b896..344dffd5 100644
--- a/libmultipath/vector.h
+++ b/libmultipath/vector.h
@@ -40,7 +40,7 @@ typedef struct _vector *vector;
 #define vector_foreach_slot_after(v,p,i) \
 	for (; (v) && i < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); i++)
 #define vector_foreach_slot_backwards(v,p,i) \
-	for (i = VECTOR_SIZE(v); i > 0 && ((p) = (v)->slot[i-1]); i--)
+	for (i = VECTOR_SIZE(v) - 1; (int)i >= 0 && ((p) = (v)->slot[i]); i--)
 
 #define identity(x) (x)
 /*
-- 
2.17.2

Re: [PATCH] libmultipath: make vector_foreach_slot_backwards work as expected

[Martin Wilck]

On Fri, 2019-05-24 at 17:41 -0500, Benjamin Marzinski wrote:
> All of the code that uses vector_foreach_slot_backwards() treats "i"
> as
> the index of the entry "p", but the way it was coded, that wasn't the
> case. "i" was the number of the entry counting from 1, not 0.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/vector.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Martin Wilck <mwilck@suse.com>

Let's add that there was only one caller that actually used
"i"("multipath -W"). So the harm done by this bug was not as bad as one
might think.

Re: [PATCH] libmultipath: make vector_foreach_slot_backwards work as expected

[Benjamin Marzinski]

On Mon, May 27, 2019 at 12:03:59PM +0200, Martin Wilck wrote:
> On Fri, 2019-05-24 at 17:41 -0500, Benjamin Marzinski wrote:
> > All of the code that uses vector_foreach_slot_backwards() treats "i"
> > as
> > the index of the entry "p", but the way it was coded, that wasn't the
> > case. "i" was the number of the entry counting from 1, not 0.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/vector.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> 
> Let's add that there was only one caller that actually used
> "i"("multipath -W"). So the harm done by this bug was not as bad as one
> might think.
>  

Actually, it caused _find_controllers() to delete the wrong slot from
map->pgvec, which caused a crash on a future _find_controllers() call
because we don't check for a NULL path between

path = nvme_pg_to_path(pg);

and

path->seen = false;

Since there should never be a NULL path, I don't think we need to add
that check, but this happens in more than multipath -W. We use "i" in
_cleanup_foreign(), cleanup_nvme_map(), _delete_all(), and
_find_controllers(). 

-Ben