Message ID | 109aefbeac593ab5660a71df38f1727002c19e39.camel@mengyan1223.wang |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 81E073858424 for <patchwork@sourceware.org>; Tue, 9 Nov 2021 13:50:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 81E073858424 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1636465824; bh=vvXiIRWW+qNN17LTDdjOjaYFXRkQsD1iYwg0dTTGJeA=; h=Subject:To:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=ncMlY/7LG0YEUOt/8dkC2luivqO4a6puf2Szwn/xnZSKWFeEPCl2K320cYROunh5A 2IHuffu2TjW2SpAkiztrPQS51zFt6/xMrv6GVj/DR0CrpRo4F7stMJxiOC3IQyOEvR Tj/ZN0blr0JrxHlO1bFn7zWWksPv/UWXDVGfQHQg= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mengyan1223.wang (mengyan1223.wang [89.208.246.23]) by sourceware.org (Postfix) with ESMTPS id 728903858416 for <gcc-patches@gcc.gnu.org>; Tue, 9 Nov 2021 13:49:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 728903858416 Received: from localhost.localdomain (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature ECDSA (P-384) server-digest SHA384) (Client did not present a certificate) (Authenticated sender: xry111@mengyan1223.wang) by mengyan1223.wang (Postfix) with ESMTPSA id C7D2065B44; Tue, 9 Nov 2021 08:49:41 -0500 (EST) Message-ID: <109aefbeac593ab5660a71df38f1727002c19e39.camel@mengyan1223.wang> Subject: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument To: gcc-patches@gcc.gnu.org Date: Tue, 09 Nov 2021 21:49:38 +0800 Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3037.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, JMQ_SPF_NEUTRAL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Xi Ruoyao via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Xi Ruoyao <xry111@mengyan1223.wang> Cc: Bruce Korb <bkorb@gnu.org> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
fixincludes: don't assume getcwd() can handle NULL argument
|
|
Commit Message
Xi Ruoyao
Nov. 9, 2021, 1:49 p.m. UTC
POSIX says: On some implementations, if buf is a null pointer, getcwd() may obtain size bytes of memory using malloc(). In this case, the pointer returned by getcwd() may be used as the argument in a subsequent call to free(). Invoking getcwd() with buf as a null pointer is not recommended in conforming applications. This produces an error building GCC with --enable-werror-always: ../../../fixincludes/fixincl.c: In function ‘process’: ../../../fixincludes/fixincl.c:1356:7: error: argument 1 is null but the corresponding size argument 2 value is 4096 [-Werror=nonnull] And, at least we've been leaking memory even if getcwd() supports this non-standard extension. fixincludes/ChangeLog: * fixincl.c (process): Allocate and deallocate the buffer for getcwd() explicitly. --- fixincludes/fixincl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Comments
On Tue, 9 Nov 2021, Xi Ruoyao via Gcc-patches wrote: > POSIX says: > > On some implementations, if buf is a null pointer, getcwd() may obtain > size bytes of memory using malloc(). In this case, the pointer returned > by getcwd() may be used as the argument in a subsequent call to free(). > Invoking getcwd() with buf as a null pointer is not recommended in > conforming applications. > > This produces an error building GCC with --enable-werror-always: > > ../../../fixincludes/fixincl.c: In function ‘process’: > ../../../fixincludes/fixincl.c:1356:7: error: argument 1 is null but > the corresponding size argument 2 value is 4096 [-Werror=nonnull] Isn't this warning actually a glibc bug <https://sourceware.org/bugzilla/show_bug.cgi?id=27476>?
On Wed, 2021-11-10 at 00:02 +0000, Joseph Myers wrote: > On Tue, 9 Nov 2021, Xi Ruoyao via Gcc-patches wrote: > > > POSIX says: > > > > On some implementations, if buf is a null pointer, getcwd() may > > obtain > > size bytes of memory using malloc(). In this case, the pointer > > returned > > by getcwd() may be used as the argument in a subsequent call to > > free(). > > Invoking getcwd() with buf as a null pointer is not recommended > > in > > conforming applications. > > > > This produces an error building GCC with --enable-werror-always: > > > > ../../../fixincludes/fixincl.c: In function ‘process’: > > ../../../fixincludes/fixincl.c:1356:7: error: argument 1 is null > > but > > the corresponding size argument 2 value is 4096 [- > > Werror=nonnull] > > Isn't this warning actually a glibc bug > <https://sourceware.org/bugzilla/show_bug.cgi?id=27476>? However we can't assume the libc we are using is Glibc. Even if the libc supports getcwd() with NULL argument, we are still leaking memory.
On 11/10/21 4:22 AM, Xi Ruoyao wrote: >> Isn't this warning actually a glibc bug >> <https://sourceware.org/bugzilla/show_bug.cgi?id=27476>? > However we can't assume the libc we are using is Glibc. Even if the > libc supports getcwd() with NULL argument, we are still leaking memory. You could also free the memory by calling exit(2). Something is pretty wrong if fixincludes cannot access a file it was told to process. So, technically, you're right. Practically, no difference. But it's fine by me to make the change. It does avoid a bug in a certain version of a certain library.
On Tue, Nov 9, 2021 at 8:50 AM Xi Ruoyao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > POSIX says: > > On some implementations, if buf is a null pointer, getcwd() may obtain > size bytes of memory using malloc(). In this case, the pointer returned > by getcwd() may be used as the argument in a subsequent call to free(). > Invoking getcwd() with buf as a null pointer is not recommended in > conforming applications. > > This produces an error building GCC with --enable-werror-always: > > ../../../fixincludes/fixincl.c: In function ‘process’: > ../../../fixincludes/fixincl.c:1356:7: error: argument 1 is null but > the corresponding size argument 2 value is 4096 [-Werror=nonnull] > > And, at least we've been leaking memory even if getcwd() supports this > non-standard extension. > > fixincludes/ChangeLog: > > * fixincl.c (process): Allocate and deallocate the buffer for > getcwd() explicitly. > --- > fixincludes/fixincl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c > index 6dba2f6e830..b4b1e38ede7 100644 > --- a/fixincludes/fixincl.c > +++ b/fixincludes/fixincl.c > @@ -1353,9 +1353,11 @@ process (void) > if (access (pz_curr_file, R_OK) != 0) > { > int erno = errno; > + char *buf = xmalloc (MAXPATHLEN); > fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n", > - pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN), > + pz_curr_file, getcwd (buf, MAXPATHLEN), > erno, xstrerror (erno)); > + free (buf); > return; > } > > -- > 2.33.1 This seems to contradict bug 21823: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21823 It would fix bug 80047, though: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80047
On 11/11/2021 6:04 AM, Eric Gallager via Gcc-patches wrote: > On Tue, Nov 9, 2021 at 8:50 AM Xi Ruoyao via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> POSIX says: >> >> On some implementations, if buf is a null pointer, getcwd() may obtain >> size bytes of memory using malloc(). In this case, the pointer returned >> by getcwd() may be used as the argument in a subsequent call to free(). >> Invoking getcwd() with buf as a null pointer is not recommended in >> conforming applications. >> >> This produces an error building GCC with --enable-werror-always: >> >> ../../../fixincludes/fixincl.c: In function ‘process’: >> ../../../fixincludes/fixincl.c:1356:7: error: argument 1 is null but >> the corresponding size argument 2 value is 4096 [-Werror=nonnull] >> >> And, at least we've been leaking memory even if getcwd() supports this >> non-standard extension. >> >> fixincludes/ChangeLog: >> >> * fixincl.c (process): Allocate and deallocate the buffer for >> getcwd() explicitly. >> --- >> fixincludes/fixincl.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c >> index 6dba2f6e830..b4b1e38ede7 100644 >> --- a/fixincludes/fixincl.c >> +++ b/fixincludes/fixincl.c >> @@ -1353,9 +1353,11 @@ process (void) >> if (access (pz_curr_file, R_OK) != 0) >> { >> int erno = errno; >> + char *buf = xmalloc (MAXPATHLEN); >> fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n", >> - pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN), >> + pz_curr_file, getcwd (buf, MAXPATHLEN), >> erno, xstrerror (erno)); >> + free (buf); >> return; >> } >> >> -- >> 2.33.1 > This seems to contradict bug 21823: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21823 I think the suggestion in that BZ is fundamentally broken in that it depends on behavior extensions that can not be relied upon. Providing a backup value of MAXPATHLEN for systems that don't provide it is a better choice. I'm less concerned about the leak and much more concerned about depending on the posix extension. Jeff
diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c index 6dba2f6e830..b4b1e38ede7 100644 --- a/fixincludes/fixincl.c +++ b/fixincludes/fixincl.c @@ -1353,9 +1353,11 @@ process (void) if (access (pz_curr_file, R_OK) != 0) { int erno = errno; + char *buf = xmalloc (MAXPATHLEN); fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n", - pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN), + pz_curr_file, getcwd (buf, MAXPATHLEN), erno, xstrerror (erno)); + free (buf); return; }