[PATCH COMMITTED] test-skeleton: Support temporary files without memory leaks [BZ#18333]

[Florian Weimer <fweimer at redhat dot com>]

On 04/26/2015 03:50 PM, H.J. Lu wrote:
> On Fri, Apr 24, 2015 at 8:36 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 02/18/2015 11:46 AM, Florian Weimer wrote:
>>> On 02/16/2015 11:51 PM, Paul Eggert wrote:
>>>> Florian Weimer wrote:
>>>>> So I'm not sure what to do here.  Get rid of the alloca?  That's going
>>>>> to be more difficult to review.
>>>>
>>>> I haven't read the code carefully, but if the only reason for the alloca
>>>> is to have a temporary string that one can munge by storing '\0' bytes
>>>> at strategic locations, then I presume that one could rewrite the code
>>>> to avoid the need to make a temporary copy,
>>>
>>> Indeed.  I introduced __tzstring_len to avoid the need for the copy, and
>>> broke down __tzset_parse_tz into several smaller functions.  Hopefully,
>>> the control flow is more transparent.
>>
>> I've committed this now, with a few whitespace fixes.
>>
> 
> This caused:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=18333

Fixed thusly.  I didn't see the crash in my original testing.

-- 
Florian Weimer / Red Hat Product Security

>From cc8dcf96e71dd643f929e32150904cd6ad69efa8 Mon Sep 17 00:00:00 2001
Message-Id: <cc8dcf96e71dd643f929e32150904cd6ad69efa8.1430144442.git.fweimer@redhat.com>
From: Florian Weimer <fweimer@redhat.com>
Date: Mon, 27 Apr 2015 15:41:03 +0200
Subject: [PATCH] test-skeleton: Support temporary files without memory leaks
 [BZ#18333]
To: libc-alpha@sourceware.org

add_temp_file now makes a copy which is freed by delete_temp_files.
Callers to create_temp_file can now free the returned file name to
avoid the memory leak.  These changes do not affect the leak behavior
of existing code.

Also address a NULL pointer derefence in tzset after a memoru allocation
failure, found during testing.
---
 ChangeLog       | 11 +++++++++++
 NEWS            |  2 +-
 test-skeleton.c | 23 ++++++++++++++++++-----
 time/tzset.c    |  7 ++++++-
 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7d3e69e..373b12c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2015-04-27  Florian Weimer  <fweimer@redhat.com>
+
+	[BZ#18333]
+	* time/tzset.c (parse_tzname): Return error on memory allocation
+	failure.
+	* test-skeleton.c (struct temp_name_list): Change type of name
+	member to non-const.
+	(add_temp_file): Create a copy of the file name.
+	(delete_temp_files): Deallocate memory.
+	(create_temp_file): Add comment.
+
 2015-04-24  Florian Weimer  <fweimer@redhat.com>
 
 	* io/posix_fallocate.c (posix_fallocate): Do not set errno.
diff --git a/NEWS b/NEWS
index 6408bed..72e15f5 100644
--- a/NEWS
+++ b/NEWS
@@ -16,7 +16,7 @@ Version 2.22
   17967, 17969, 17978, 17987, 17991, 17996, 17998, 17999, 18019, 18020,
   18029, 18030, 18032, 18036, 18038, 18039, 18042, 18043, 18046, 18047,
   18068, 18080, 18093, 18100, 18104, 18110, 18111, 18128, 18138, 18185,
-  18197, 18206, 18210, 18211, 18247, 18287.
+  18197, 18206, 18210, 18211, 18247, 18287, 18333.
 
 * Cache information can be queried via sysconf() function on s390 e.g. with
   _SC_LEVEL1_ICACHE_SIZE as argument.
diff --git a/test-skeleton.c b/test-skeleton.c
index 7a8ddfa..43fc236 100644
--- a/test-skeleton.c
+++ b/test-skeleton.c
@@ -73,7 +73,7 @@ static const char *test_dir;
 struct temp_name_list
 {
   struct qelem q;
-  const char *name;
+  char *name;
 } *temp_name_list;
 
 /* Add temporary files in list.  */
@@ -83,14 +83,17 @@ add_temp_file (const char *name)
 {
   struct temp_name_list *newp
     = (struct temp_name_list *) calloc (sizeof (*newp), 1);
-  if (newp != NULL)
+  char *newname = strdup (name);
+  if (newp != NULL && newname != NULL)
     {
-      newp->name = name;
+      newp->name = newname;
       if (temp_name_list == NULL)
 	temp_name_list = (struct temp_name_list *) &newp->q;
       else
 	insque (newp, temp_name_list);
     }
+  else
+    free (newp);
 }
 
 /* Delete all temporary files.  */
@@ -100,11 +103,19 @@ delete_temp_files (void)
   while (temp_name_list != NULL)
     {
       remove (temp_name_list->name);
-      temp_name_list = (struct temp_name_list *) temp_name_list->q.q_forw;
+      free (temp_name_list->name);
+
+      struct temp_name_list *next
+	= (struct temp_name_list *) temp_name_list->q.q_forw;
+      free (temp_name_list);
+      temp_name_list = next;
     }
 }
 
-/* Create a temporary file.  */
+/* Create a temporary file.  Return the opened file descriptor on
+   success, or -1 on failure.  Write the file name to *FILENAME if
+   FILENAME is not NULL.  In this case, the caller is expected to free
+   *FILENAME.  */
 static int
 __attribute__ ((unused))
 create_temp_file (const char *base, char **filename)
@@ -132,6 +143,8 @@ create_temp_file (const char *base, char **filename)
   add_temp_file (fname);
   if (filename != NULL)
     *filename = fname;
+  else
+    free (fname);
 
   return fd;
 }
diff --git a/time/tzset.c b/time/tzset.c
index d115bae..160f5ad 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -201,7 +201,12 @@ parse_tzname (const char **tzp, int whichrule)
       if (*p++ != '>' || len < 3)
 	return false;
     }
-  tz_rules[whichrule].name = __tzstring_len (start, len);
+
+  const char *name = __tzstring_len (start, len);
+  if (name == NULL)
+    return false;
+  tz_rules[whichrule].name = name;
+
   *tzp = p;
   return true;
 }
-- 
2.1.0