[PATCH] Fix inet6_opt_{append,finish}

[PATCH] Fix inet6_opt_{append,finish}

[Jakub Jelinek]

Hi!

As the attached testcase shows, there have been several bugs:
1) if extbuf == NULL, extlen shouldn't be checked (happened both in
   inet6_opt_{append,finish})
2) if the last option's end was 8 byte aligned already, inet6_opt_finish
   would say it needs 0 bytes to fill it, but then adding
   IP6OPT_PAD1 254 254x'\0'

BTW, the npad calculations are too complicated:
int npad = (align - data_offset % align) & (align - 1);
is I believe the same as:
int npad = -data_offset & (align - 1);
and
int npad = (8 - (offset & 7)) & 7;
is
int npad = -offset & 7;
Apparently at least the latter isn't fully optimized on i?86 (but is on
x86_64), guess ++todo for GCC.

2007-03-15  Jakub Jelinek  <jakub@redhat.com>

	[BZ #4181]
	* inet/inet6_opt.c (add_padding): Only insert padding if npad > 0.
	(inet6_opt_append): Don't check extlen is big enough if extbuf
	is NULL.
	(inet6_opt_finish): Likewise.
	* inet/Makefile (tests): Add test-inet6_opt.
	* inet/test-inet6_opt.c: New test.

--- libc/inet/inet6_opt.c.jj	2006-05-25 06:38:02.000000000 +0200
+++ libc/inet/inet6_opt.c	2007-03-15 12:15:02.000000000 +0100
@@ -1,4 +1,4 @@
-/* Copyright (C) 2006 Free Software Foundation, Inc.
+/* Copyright (C) 2006, 2007 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2006.
 
@@ -51,7 +51,7 @@ add_padding (uint8_t *extbuf, int offset
 {
   if (npad == 1)
     extbuf[offset] = IP6OPT_PAD1;
-  else
+  else if (npad > 0)
     {
       struct ip6_opt *pad_opt = (struct ip6_opt *) (extbuf + offset);
 
@@ -102,21 +102,17 @@ inet6_opt_append (void *extbuf, socklen_
   int data_offset = offset + sizeof (struct ip6_opt);
   int npad = (align - data_offset % align) & (align - 1);
 
-  /* Now we can check whether the buffer is large enough.  */
-  if (data_offset + npad + len > extlen)
-    return -1;
-
-  if (npad != 0)
+  if (extbuf != NULL)
     {
-      if (extbuf != NULL)
-	add_padding (extbuf, offset, npad);
+      /* Now we can check whether the buffer is large enough.  */
+      if (data_offset + npad + len > extlen)
+	return -1;
+
+      add_padding (extbuf, offset, npad);
 
       offset += npad;
-    }
 
-  /* Now prepare the option itself.  */
-  if (extbuf != NULL)
-    {
+      /* Now prepare the option itself.  */
       struct ip6_opt *opt = (struct ip6_opt *) ((uint8_t *) extbuf + offset);
 
       opt->ip6o_type = type;
@@ -124,6 +120,8 @@ inet6_opt_append (void *extbuf, socklen_
 
       *databufp = opt + 1;
     }
+  else
+    offset += npad;
 
   return offset + sizeof (struct ip6_opt) + len;
 }
@@ -145,12 +143,14 @@ inet6_opt_finish (void *extbuf, socklen_
   /* Required padding at the end.  */
   int npad = (8 - (offset & 7)) & 7;
 
-  /* Make sure the buffer is large enough.  */
-  if (offset + npad > extlen)
-    return -1;
-
   if (extbuf != NULL)
-    add_padding (extbuf, offset, npad);
+    {
+      /* Make sure the buffer is large enough.  */
+      if (offset + npad > extlen)
+	return -1;
+
+      add_padding (extbuf, offset, npad);
+    }
 
   return offset + npad;
 }
--- libc/inet/Makefile.jj	2007-01-15 23:25:26.000000000 +0100
+++ libc/inet/Makefile	2007-03-15 12:26:53.000000000 +0100
@@ -52,7 +52,7 @@ routines := htonl htons		\
 aux := check_pf ifreq
 
 tests := htontest test_ifindex tst-ntoa tst-ether_aton tst-network \
-	 tst-gethnm test-ifaddrs bug-if1
+	 tst-gethnm test-ifaddrs bug-if1 test-inet6_opt
 
 include ../Rules
 
--- libc/inet/test-inet6_opt.c.jj	2007-03-15 11:57:02.000000000 +0100
+++ libc/inet/test-inet6_opt.c	2007-03-15 12:29:21.000000000 +0100
@@ -0,0 +1,207 @@
+#include <netinet/in.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define OPT_X	42
+#define OPT_Y	43
+#define OPT_Z	44
+
+static void *
+encode_inet6_opt (socklen_t *elp)
+{
+  void *eb = NULL;
+  socklen_t el;
+  int cl;
+  void *db;
+  int offset;
+  uint8_t val1;
+  uint16_t val2;
+  uint32_t val4;
+  uint64_t val8;
+
+  *elp = 0;
+#define CHECK() \
+  if (cl == -1)						\
+    {							\
+      printf ("cl == -1 on line %d\n", __LINE__);	\
+      free (eb);					\
+      return NULL;					\
+    }
+
+  /* Estimate the length */
+  cl = inet6_opt_init (NULL, 0);
+  CHECK ();
+  cl = inet6_opt_append (NULL, 0, cl, OPT_X, 12, 8, NULL);
+  CHECK ();
+  cl = inet6_opt_append (NULL, 0, cl, OPT_Y, 7, 4, NULL);
+  CHECK ();
+  cl = inet6_opt_append (NULL, 0, cl, OPT_Z, 7, 1, NULL);
+  CHECK ();
+  cl = inet6_opt_finish (NULL, 0, cl);
+  CHECK ();
+  el = cl;
+
+  eb = malloc (el + 8);
+  if (eb == NULL)
+    {
+      puts ("malloc failed");
+      return NULL;
+    }
+  /* Canary.  */
+  memcpy (eb + el, "deadbeef", 8);
+
+  cl = inet6_opt_init (eb, el);
+  CHECK ();
+
+  cl = inet6_opt_append (eb, el, cl, OPT_X, 12, 8, &db);
+  CHECK ();
+  val4 = 0x12345678;
+  offset = inet6_opt_set_val (db, 0, &val4, sizeof  (val4));
+  val8 = 0x0102030405060708LL;
+  inet6_opt_set_val (db, offset, &val8, sizeof  (val8));
+
+  cl = inet6_opt_append (eb, el, cl, OPT_Y, 7, 4, &db);
+  CHECK ();
+  val1 = 0x01;
+  offset = inet6_opt_set_val (db, 0, &val1, sizeof  (val1));
+  val2 = 0x1331;
+  offset = inet6_opt_set_val (db, offset, &val2, sizeof  (val2));
+  val4 = 0x01020304;
+  inet6_opt_set_val (db, offset, &val4, sizeof  (val4));
+
+  cl = inet6_opt_append (eb, el, cl, OPT_Z, 7, 1, &db);
+  CHECK ();
+  inet6_opt_set_val (db, 0, (void *) "abcdefg", 7);
+
+  cl = inet6_opt_finish (eb, el, cl);
+  CHECK ();
+
+  if (memcmp (eb + el, "deadbeef", 8) != 0)
+    {
+      puts ("Canary corrupted");
+      free (eb);
+      return NULL;
+    }
+  *elp = el;
+  return eb;
+}
+
+int
+decode_inet6_opt (void *eb, socklen_t el)
+{
+  int ret = 0;
+  int seq = 0;
+  int cl = 0;
+  int offset;
+  uint8_t type;
+  socklen_t len;
+  uint8_t val1;
+  uint16_t val2;
+  uint32_t val4;
+  uint64_t val8;
+  void *db;
+  char buf[8];
+
+  while ((cl = inet6_opt_next (eb, el, cl, &type, &len, &db)) != -1)
+    switch (type)
+      {
+      case OPT_X:
+	if (seq++ != 0)
+	  {
+	    puts ("OPT_X is not first");
+	    ret = 1;
+	  }
+	if (len != 12)
+	  {
+	    printf ("OPT_X's length %d != 12\n", len);
+	    ret = 1;
+	  }
+	offset = inet6_opt_get_val (db, 0, &val4, sizeof (val4));
+	if (val4 != 0x12345678)
+	  {
+	    printf ("OPT_X's val4 %x != 0x12345678\n", val4);
+	    ret = 1;
+	  }
+	offset = inet6_opt_get_val (db, offset, &val8, sizeof (val8));
+	if (offset != len || val8 != 0x0102030405060708LL)
+	  {
+	    printf ("OPT_X's val8 %llx != 0x0102030405060708\n",
+		    (long long) val8);
+	    ret = 1;
+	  }
+	break;
+      case OPT_Y:
+	if (seq++ != 1)
+	  {
+	    puts ("OPT_Y is not second");
+	    ret = 1;
+	  }
+	if (len != 7)
+	  {
+	    printf ("OPT_Y's length %d != 7\n", len);
+	    ret = 1;
+	  }
+	offset = inet6_opt_get_val (db, 0, &val1, sizeof (val1));
+	if (val1 != 0x01)
+	  {
+	    printf ("OPT_Y's val1 %x != 0x01\n", val1);
+	    ret = 1;
+	  }
+	offset = inet6_opt_get_val (db, offset, &val2, sizeof (val2));
+	if (val2 != 0x1331)
+	  {
+	    printf ("OPT_Y's val2 %x != 0x1331\n", val2);
+	    ret = 1;
+	  }
+	offset = inet6_opt_get_val (db, offset, &val4, sizeof (val4));
+	if (offset != len || val4 != 0x01020304)
+	  {
+	    printf ("OPT_Y's val4 %x != 0x01020304\n", val4);
+	    ret = 1;
+	  }
+	break;
+      case OPT_Z:
+	if (seq++ != 2)
+	  {
+	    puts ("OPT_Z is not third");
+	    ret = 1;
+	  }
+	if (len != 7)
+	  {
+	    printf ("OPT_Z's length %d != 7\n", len);
+	    ret = 1;
+	  }
+	offset = inet6_opt_get_val (db, 0, buf, 7);
+	if (offset != len || memcmp (buf, "abcdefg", 7) != 0)
+	  {
+	    buf[7] = '\0';
+	    printf ("OPT_Z's buf \"%s\" != \"abcdefg\"\n", buf);
+	    ret = 1;
+	  }
+	break;
+      default:
+	printf ("Unknown option %d\n", type);
+	ret = 1;
+	break;
+      }
+  if (seq != 3)
+    {
+      puts ("Didn't see all of OPT_X, OPT_Y and OPT_Z");
+      ret = 1;
+    }
+  return ret;
+}
+
+int
+main (void)
+{
+  void *eb;
+  socklen_t el;
+  eb = encode_inet6_opt (&el);
+  if (eb == NULL)
+    return 1;
+  if (decode_inet6_opt (eb, el))
+    return 1;
+  return 0;
+}

	Jakub