In no particular order, changed/fixed all of the following in

ares_parse_txt_reply() current version:

- Fixed a couple of potential double free's.

- Fixed memory leaks upon out of memory condition.

- Fixed pointer arithmetic.

- Setting ntxtreply to zero upon entry for all failure cases.

- Changed data type to size_t for variables substr_len, str_len and
  the length member of ares_txt_reply struct.

- Avoided a couple of memcpy() calls.

- Changed i data type to unsigned int to prevent compiler warnings.

- Adjusted a comment.

- Use ARES_SUCCESS literal for successfull completion.

- Added CVS Id tag.

diff --git a/ares/Makefile.inc b/ares/Makefile.inc
index 1fe1129..e383ebb 100644 (file)
--- a/ares/Makefile.inc
+++ b/ares/Makefile.inc
@@ -24,7 +24,6 @@ CSOURCES = ares__close_sockets.c      \
   ares_parse_ptr_reply.c               \
   ares_parse_srv_reply.c               \
   ares_parse_txt_reply.c               \
-  ares_free_txt_reply.c                 \
   ares_process.c                       \
   ares_query.c                         \
   ares_search.c                                \
@@ -83,6 +82,7 @@ MANPAGES = ares_cancel.3              \
   ares_parse_ns_reply.3                        \
   ares_parse_ptr_reply.3               \
   ares_parse_srv_reply.3               \
+  ares_parse_txt_reply.3               \
   ares_process.3                       \
   ares_query.3                         \
   ares_save_options.3                  \
@@ -117,6 +117,7 @@ HTMLPAGES = ares_cancel.html                \
   ares_parse_ns_reply.html             \
   ares_parse_ptr_reply.html            \
   ares_parse_srv_reply.html            \
+  ares_parse_txt_reply.html            \
   ares_process.html                    \
   ares_query.html                      \
   ares_save_options.html               \
@@ -151,6 +152,7 @@ PDFPAGES = ares_cancel.pdf          \
   ares_parse_ns_reply.pdf              \
   ares_parse_ptr_reply.pdf             \
   ares_parse_srv_reply.pdf             \
+  ares_parse_txt_reply.pdf             \
   ares_process.pdf                     \
   ares_query.pdf                       \
   ares_save_options.pdf                        \

diff --git a/ares/ares.h b/ares/ares.h
index 35038d2..593a145 100644 (file)
--- a/ares/ares.h
+++ b/ares/ares.h
@@ -437,7 +437,7 @@ struct ares_srv_reply {
 };
 
 struct ares_txt_reply {
-  unsigned int  length;
+  size_t         length;  /* length excludes null termination */
   unsigned char *txt;
 };
 

diff --git a/ares/ares_parse_txt_reply.c b/ares/ares_parse_txt_reply.c
index c99ee72..6950912 100644 (file)
--- a/ares/ares_parse_txt_reply.c
+++ b/ares/ares_parse_txt_reply.c
@@ -1,4 +1,7 @@
+/* $Id$ */
+
 /* Copyright (C) 2009 Jakub Hrozek <jhrozek@redhat.com>
+*  Copyright (C) 2009 Yang Tse <yangsita@gmail.com>
 *
 * Permission to use, copy, modify, and distribute this
 * software and its documentation for any purpose and without
@@ -38,18 +41,21 @@ int
 ares_parse_txt_reply (const unsigned char *abuf, int alen,
                       struct ares_txt_reply **txt_out, int *ntxtreply)
 {
-  unsigned char substr_len = 0;
-  unsigned char str_len = 0;
-  unsigned int qdcount, ancount;
+  size_t substr_len, str_len;
+  unsigned int qdcount, ancount, i;
   const unsigned char *aptr;
   const unsigned char *strptr;
-  int status, i, rr_type, rr_class, rr_len;
+  int status, rr_type, rr_class, rr_len;
   long len;
   char *hostname = NULL, *rr_name = NULL;
   struct ares_txt_reply *txt = NULL;
 
+  /* Set *txt_out to NULL for all failure cases. */
   *txt_out = NULL;
 
+  /* Same with *nsrvreply. */
+  *ntxtreply = 0;
+
   /* Give up if abuf doesn't have room for a header. */
   if (alen < HFIXEDSZ)
     return ARES_EBADRESP;
@@ -82,11 +88,16 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
       free (hostname);
       return ARES_ENOMEM;
     }
-  /* Zero out so we can safely free txt.txt even if NULL */
-  memset(txt, 0, ancount * sizeof (struct ares_txt_reply));
+
+  /* Initialize ares_txt_reply array */
+  for (i = 0; i < ancount; i++)
+    {
+      txt[i].txt = NULL;
+      txt[i].length = 0;
+    }
 
   /* Examine each answer resource record (RR) in turn. */
-  for (i = 0; i < (int) ancount; i++)
+  for (i = 0; i < ancount; i++)
     {
       /* Decode the RR up to the data field. */
       status = ares_expand_name (aptr, abuf, alen, &rr_name, &len);
@@ -116,18 +127,17 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
            * substrings contained therein.
            */
 
-          /* Realloc would be expensive, compute the total length */
-          txt[i].length = 0;
+          /* Compute total length to allow a single memory allocation */
           strptr = aptr;
           while (strptr < (aptr + rr_len))
             {
-              memcpy ((void *) &substr_len, strptr, sizeof (unsigned char));
+              substr_len = (unsigned char)*strptr;
               txt[i].length += substr_len;
               strptr += substr_len + 1;
             }
 
           /* Including null byte */
-          txt[i].txt = malloc (sizeof (unsigned char) * (txt[i].length + 1));
+          txt[i].txt = malloc (txt[i].length + 1);
           if (txt[i].txt == NULL)
             {
               status = ARES_ENOMEM;
@@ -135,15 +145,13 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
             }
 
           /* Step through the list of substrings, concatenating them */
-          substr_len = 0;
           str_len = 0;
           strptr = aptr;
           while (strptr < (aptr + rr_len))
             {
-              memcpy ((void *) &substr_len, strptr, sizeof (unsigned char));
+              substr_len = (unsigned char)*strptr;
               strptr++;
-              memcpy ((void *) txt[i].txt + str_len, strptr,
-                      sizeof (unsigned char) * substr_len);
+              memcpy ((char *) txt[i].txt + str_len, strptr, substr_len);
               str_len += substr_len;
               strptr += substr_len;
             }
@@ -159,18 +167,25 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
       rr_name = NULL;
     }
 
-  free (hostname);
-  free (rr_name);
+  if (hostname);
+    free (hostname);
+  if (rr_name);
+    free (rr_name);
 
   /* clean up on error */
   if (status != ARES_SUCCESS)
     {
-      ares_free_txt_reply(txt, ancount);
+    for (i = 0; i < ancount; i++)
+      {
+        if (txt[i].txt)
+          free (txt[i].txt);
+      }
       return status;
     }
 
   /* everything looks fine, return the data */
   *txt_out = txt;
   *ntxtreply = ancount;
-  return 0;
+
+  return ARES_SUCCESS;
 }