[quagga-dev 5271] Re: mes_lookup / LOOKUP robustness

Paul Jakma Paul.Jakma at Sun.COM
Tue Feb 26 23:29:47 GMT 2008


On Tue, 26 Feb 2008, Andrew J. Schorr wrote:

> 1. Are there any places in the code that actually test for a NULL
> return value from mes_lookup and handle that case appropriately?

>From my own quick inspection, I'd say none ;)

> For example, in ospfd/ospf_dump.c:ospf_lsa_header_dump, the return
> code is actually checked:

Hmm, is it?

> void
> ospf_lsa_header_dump (struct lsa_header *lsah)
> {
>  const char *lsah_type = LOOKUP (ospf_lsa_type_msg, lsah->type);
>
>  zlog_debug ("    LS type %d (%s)", lsah->type,
>             (lsah->type ? lsah_type : "unknown type"));

It's test lsah->type, not lsah_type - it's assuming therefore that 
ospf_lsa_type_msg has strings for every possible value. However it 
doesn't, so lsah_type could still be NULL.

> Based on a quick scan  of the source, that may be the only such case,
> but I'm not certain.

It doesn't seem to be a shining example :).

> It seems that there is often no reason to initialize str, so why 
> bother to do so in the normal case?

Hmm, I guess. Or just get rid of it altogether. How about this?:

Index: lib/log.c
===================================================================
RCS file: /var/cvsroot/quagga/lib/log.c,v
retrieving revision 1.33
diff -u -p -r1.33 log.c
--- lib/log.c	6 Aug 2007 15:21:45 -0000	1.33
+++ lib/log.c	26 Feb 2008 23:28:46 -0000
@@ -756,10 +756,17 @@ lookup (struct message *mes, int key)
  const char *
  mes_lookup (struct message *meslist, int max, int index)
  {
+  int shift;
+ 
+  /* key number ranges could start anywhere, e.g. 1 is typical for protocol
+     constants */
+  shift = meslist[0].key;
+
    /* first check for best case: index is in range and matches the key
       value in that slot */
-  if ((index >= 0) && (index < max) && (meslist[index].key == index))
-    return meslist[index].str;
+  if ((index >= shift) && ((index - shift) < max)
+      && (meslist[index - shift].key == index))
+    return meslist[index - shift].str;

    /* fall back to linear search */
    {
@@ -769,14 +776,16 @@ mes_lookup (struct message *meslist, int
        {
  	if (meslist->key == index)
  	  {
+	    const char *str = (meslist->str ? meslist->str : "(nil)");
+
  	    zlog_debug ("message index %d [%s] found in position %d (max is %d)",
-		      index, meslist->str, i, max);
-	    return meslist->str;
+		      index, str, i, max);
+	    return str;
  	  }
        }
    }
    zlog_err("message index %d not found (max is %d)", index, max);
-  return NULL;
+  return "(nil)";
  }

  /* Wrapper around strerror to handle case where it returns NULL. */

regards,
-- 
Paul Jakma,
Solaris Networking                       Sun Microsystems, Scotland
http://opensolaris.org/os/project/quagga tel: EMEA x73150 / +44 15066 73150



More information about the Quagga-dev mailing list