Message ID | 109aefbeac593ab5660a71df38f1727002c19e39.camel@mengyan1223.wang |
---|---|
State | New |
Headers | show |
Series | fixincludes: don't assume getcwd() can handle NULL argument | expand |
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; }