Re: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Jeff Law via Gcc-patches]

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
 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

Re: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Eric Gallager via Gcc-patches]

On Tue, Nov 9, 2021 at 8:50 AM 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]
>
> 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

Re: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Bruce Korb via Gcc-patches]

On 11/10/21 4:22 AM, Xi Ruoyao wrote:

Isn't this warning actually a glibc bug
?

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.

Re: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Xi Ruoyao via Gcc-patches]

On Wed, 2021-11-10 at 00:02 +, 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 
> ?

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.

-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Joseph Myers]

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 
?

-- 
Joseph S. Myers
jos...@codesourcery.com

[PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Xi Ruoyao via Gcc-patches]

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