[PATCH] potentially bug in __catfind(), catalog.cpp

[Farid Zaripov <Fa...@epam.com>]

  I'm not sure to name this as bug. Let's it be a potentially bug.

  Below is the __catfind() function from catalog,cpp file:

  CatVector::size_type __catfind(nl_catd id)
  {
    CatVector::size_type i = 0;
    while (i < __rw_catlist.size() && __rw_catlist[i] &&
__rw_catlist[i]->id() != id)
      i++;
    if (!__rw_catlist[i])
      return __rw_catlist.size();
    return i;
  }  

  If this function will be invoked with invalid id when __rw_catlist
vector
is filled up, then after while loop i == __rw_catlist.size() and we get
undefined behavior in "if (!__rw_catlist[i])" line.

  I propose to rewrite this function this way:

  CatVector::size_type __catfind(nl_catd id)
  {
    for (CatVector::size_type i = 0; i < __rw_catlist.size() &&
__rw_catlist[i]; ++i)
      if (__rw_catlist[i]->id() == id)
          return i;
    
    return __rw_catlist.size();
  }  

  I can't write the regression test for this situation because the
__catfind() is invoked
in library always with valid id. And also I can't write the test which
invokes __catfind()
explicitly because of that function is inaccessible from user code.

  Here the patch:

  ChangeLog:
  * catalog.cpp (__catfind): Fixed potentially undefined behavior when
  __rw_catlist vector is full and id is not valid.


Index: catalog.cpp
===================================================================
--- catalog.cpp	(revision 575597)
+++ catalog.cpp	(working copy)
@@ -71,12 +71,11 @@
 
   CatVector::size_type __catfind(nl_catd id)
   {
-    CatVector::size_type i = 0;
-    while (i < __rw_catlist.size() && __rw_catlist[i] &&
__rw_catlist[i]->id() != id)
-      i++;
-    if (!__rw_catlist[i])
-      return __rw_catlist.size();
-    return i;
+    for (CatVector::size_type i = 0; i < __rw_catlist.size() &&
__rw_catlist[i]; ++i)
+      if (__rw_catlist[i]->id() == id)
+          return i;
+    
+    return __rw_catlist.size();
   }  
 
 

Farid.

Re: [PATCH] potentially bug in __catfind(), catalog.cpp

[Martin Sebor <se...@roguewave.com>]

Farid Zaripov wrote:
>   I'm not sure to name this as bug. Let's it be a potentially bug.
> 
>   Below is the __catfind() function from catalog,cpp file:
> 
>   CatVector::size_type __catfind(nl_catd id)
>   {
>     CatVector::size_type i = 0;
>     while (i < __rw_catlist.size() && __rw_catlist[i] &&
> __rw_catlist[i]->id() != id)
>       i++;
>     if (!__rw_catlist[i])
>       return __rw_catlist.size();
>     return i;
>   }  
> 
>   If this function will be invoked with invalid id when __rw_catlist
> vector
> is filled up, then after while loop i == __rw_catlist.size() and we get
> undefined behavior in "if (!__rw_catlist[i])" line.

Sure looks that way. Good catch!

> 
>   I propose to rewrite this function this way:
> 
>   CatVector::size_type __catfind(nl_catd id)
>   {
>     for (CatVector::size_type i = 0; i < __rw_catlist.size() &&
> __rw_catlist[i]; ++i)
>       if (__rw_catlist[i]->id() == id)
>           return i;
>     
>     return __rw_catlist.size();
>   }  
> 
>   I can't write the regression test for this situation because the
> __catfind() is invoked
> in library always with valid id. And also I can't write the test which
> invokes __catfind()
> explicitly because of that function is inaccessible from user code.
> 
>   Here the patch:

Makes sense to me (remember the 78 character/line limit below).

Martin

> 
>   ChangeLog:
>   * catalog.cpp (__catfind): Fixed potentially undefined behavior when
>   __rw_catlist vector is full and id is not valid.
> 
> 
> Index: catalog.cpp
> ===================================================================
> --- catalog.cpp	(revision 575597)
> +++ catalog.cpp	(working copy)
> @@ -71,12 +71,11 @@
>  
>    CatVector::size_type __catfind(nl_catd id)
>    {
> -    CatVector::size_type i = 0;
> -    while (i < __rw_catlist.size() && __rw_catlist[i] &&
> __rw_catlist[i]->id() != id)
> -      i++;
> -    if (!__rw_catlist[i])
> -      return __rw_catlist.size();
> -    return i;
> +    for (CatVector::size_type i = 0; i < __rw_catlist.size() &&
> __rw_catlist[i]; ++i)
> +      if (__rw_catlist[i]->id() == id)
> +          return i;
> +    
> +    return __rw_catlist.size();
>    }  
>  
>  
> 
> Farid.
>