From patchwork Mon Apr 27 14:25:11 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 465043 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 0F6D81402B6 for ; Tue, 28 Apr 2015 00:25:21 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=sourceware.org header.i=@sourceware.org header.b=OtrnFqGU; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type; q=dns; s=default; b= BGMh5MpozTsdg6uQmdjxjUSaLj9n8GqLX2cCO49Ta3SCmkFvowYkxWUKTi8StOEZ EiWw2qa1KL+qaQSL5yJp+Q0txQFbo2tQAfm8ERa9vfjmJkzJnXGXZQ3eIJz7KEbT YpSiBAoKm528vOjtD0IYZ/WDpTcibw7wI6IAlcDlolY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type; s=default; bh=csta FzpLkDYdfklNBeQb6VhJIbc=; b=OtrnFqGU0LoXq4G4gyCQbm8GazIXG+R1SyN1 t/i2H0Ijza1CJix7iio6OFkWBOI0sADNXloPzg+/lDivnEAhHZAlXr3HW/Hsb4bu ovQltekgr9SDU3nn18+fPffftB6MvU+KOfvtwPYxJFH2Ik5lQtDbySY5KL6CTam7 t0Jovj4= Received: (qmail 107952 invoked by alias); 27 Apr 2015 14:25:16 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 107939 invoked by uid 89); 27 Apr 2015 14:25:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=2.2 required=5.0 tests=AWL, BAYES_00, RAR_ATTACHED, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Message-ID: <553E46C7.9000608@redhat.com> Date: Mon, 27 Apr 2015 16:25:11 +0200 From: Florian Weimer User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: "H.J. Lu" CC: GNU C Library Subject: [PATCH COMMITTED] test-skeleton: Support temporary files without memory leaks [BZ#18333] References: <54E24689.4010108@redhat.com> <54E24EEF.6090503@cs.ucla.edu> <54E2516E.8050100@redhat.com> <54E27473.6030407@cs.ucla.edu> <54E46D70.7000601@redhat.com> <553A6311.70407@redhat.com> In-Reply-To: On 04/26/2015 03:50 PM, H.J. Lu wrote: > On Fri, Apr 24, 2015 at 8:36 AM, Florian Weimer 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. From cc8dcf96e71dd643f929e32150904cd6ad69efa8 Mon Sep 17 00:00:00 2001 Message-Id: From: Florian Weimer 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 + + [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 * 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