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

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

[Xi Ruoyao]

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

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 
<https://sourceware.org/bugzilla/show_bug.cgi?id=27476>?

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Xi Ruoyao]

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.

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

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

[Bruce Korb]

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.

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

[Eric Gallager]

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

[PATCH] fixincludes: fix portability issues about getcwd() [PR21283, PR80047]

[Xi Ruoyao]

[Revised to handle PR 21283.]

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.

And, MAXPATHLEN may be not unavailable on certain platform.  PATH_MAX is
POSIX, but getcwd() may produce a path with length larger than it.  So it's
suggested by POSIX [1] to call getcwd() with progressively larger buffers
until it does not give an [ERANGE] error.

[1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html

fixincludes/ChangeLog:

	PR other/21823
	PR bootstrap/80047
	* fixincl.c (process): Allocate and deallocate the buffer for
	  getcwd() progressively.
---
 fixincludes/fixincl.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
index 6dba2f6e830..1580c67efec 100644
--- a/fixincludes/fixincl.c
+++ b/fixincludes/fixincl.c
@@ -1353,9 +1353,18 @@ process (void)
   if (access (pz_curr_file, R_OK) != 0)
     {
       int erno = errno;
+      char *buf = NULL;
+      const char *cwd = NULL;
+      for (size_t size = 256; !cwd; size += size)
+	{
+	  buf = xrealloc (buf, size);
+	  cwd = getcwd (buf, size);
+	  if (!cwd && errno != ERANGE)
+	    cwd = "the working directory";
+	}
       fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
-               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
-               erno, xstrerror (erno));
+	       pz_curr_file, cwd, erno, xstrerror (erno));
+      free (buf);
       return;
     }
 
-- 
2.33.1

Re: [PATCH] fixincludes: fix portability issues about getcwd() [PR21283, PR80047]

[Bruce Korb]

If you are going to be excruciatingly, painfully correct, free() is 
going to be unhappy about freeing a static string in the event getcwd() 
fails for some inexplicable reason. I'd replace the free() + return with 
a call to exit. Maybe even:

    if (VERY_UNLIKELY (access (pz_curr_file, R_OK) != 0)) abort()

On 11/11/21 8:33 AM, Xi Ruoyao wrote:
> ---
>   fixincludes/fixincl.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
> index 6dba2f6e830..1580c67efec 100644
> --- a/fixincludes/fixincl.c
> +++ b/fixincludes/fixincl.c
> @@ -1353,9 +1353,18 @@ process (void)
>     if (access (pz_curr_file, R_OK) != 0)
>       {
>         int erno = errno;
> +      char *buf = NULL;
> +      const char *cwd = NULL;
> +      for (size_t size = 256; !cwd; size += size)
> +	{
> +	  buf = xrealloc (buf, size);
> +	  cwd = getcwd (buf, size);
> +	  if (!cwd && errno != ERANGE)
> +	    cwd = "the working directory";
> +	}
>         fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
> -               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
> -               erno, xstrerror (erno));
> +	       pz_curr_file, cwd, erno, xstrerror (erno));
> + free (buf); return;
>       }
>   

Re: [PATCH] fixincludes: fix portability issues about getcwd() [PR21283, PR80047]

[Xi Ruoyao]

On Fri, 2021-11-12 at 12:59 -0800, Bruce Korb wrote:
> If you are going to be excruciatingly, painfully correct, free() is
> going to be unhappy about freeing a static string in the event
> getcwd() fails for some inexplicable reason. I'd replace the free() +
> return with a call to exit. Maybe even:

It's free (buf), not free (cwd).  buf won't point to a static string.

buf may be NULL though, but free (NULL) is legal (no-op).


> > if (VERY_UNLIKELY (access (pz_curr_file, R_OK) != 0)) abort()

Perhaps just 

if (access (pz_curr_file, R_OK) != 0))
  {
    /* Some really inexplicable error happens. */
    fprintf (stderr, "Cannot access %s: %s",
             pz_curr_file, xstrerror (errno));
    abort();
  }

It will show which file can't be accessed so it's possible to diagnose.
And the working directory will be outputed by "make" when the fixincl
command fails anyway, so we don't need to really care it.

> On 11/11/21 8:33 AM, Xi Ruoyao wrote:
>  
> > ---
> >  fixincludes/fixincl.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
> > index 6dba2f6e830..1580c67efec 100644
> > --- a/fixincludes/fixincl.c
> > +++ b/fixincludes/fixincl.c
> > @@ -1353,9 +1353,18 @@ process (void)
> >    if (access (pz_curr_file, R_OK) != 0)
> >      {
> >        int erno = errno;
> > +      char *buf = NULL;
> > +      const char *cwd = NULL;
> > +      for (size_t size = 256; !cwd; size += size)
> > +	{
> > +	  buf = xrealloc (buf, size);
> > +	  cwd = getcwd (buf, size);
> > +	  if (!cwd && errno != ERANGE)
> > +	    cwd = "the working directory";
> > +	}
> >        fprintf (stderr, "Cannot access %s from %s\n\terror %d
> > (%s)\n",
> > -               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
> > -               erno, xstrerror (erno));
> > +	       pz_curr_file, cwd, erno, xstrerror (erno));
> > +      free (buf);
> >        return;
> >      }
> >  
>  

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

[PATCH] fixincludes: simplify handling for access() failure [PR21283, PR80047]

[Xi Ruoyao]

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]

It's suggested by POSIX to call getcwd() with progressively larger
buffers until it does not give an [ERANGE] error. However, it's highly
unlikely that this error-handling route is ever used.

So we can simplify it instead of writting too much code.  We give up to
use getcwd(), because `make` will output a `Leaving directory ...` message
containing the path to cwd when we call abort().

fixincludes/ChangeLog:

	PR other/21823
	PR bootstrap/80047
	* fixincl.c (process): Simplify the handling for highly
	  unlikely access() failure, to avoid using non-standard
	  extensions.
---
 fixincludes/fixincl.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
index 6dba2f6e830..ee57fbf61b4 100644
--- a/fixincludes/fixincl.c
+++ b/fixincludes/fixincl.c
@@ -1352,11 +1352,10 @@ process (void)
 
   if (access (pz_curr_file, R_OK) != 0)
     {
-      int erno = errno;
-      fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
-               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
-               erno, xstrerror (erno));
-      return;
+      /* Some really strange error happened. */
+      fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file,
+	       xstrerror (errno));
+      abort();
     }
 
   pz_curr_data = load_file (pz_curr_file);
-- 
2.33.1

> On Fri, 2021-11-12 at 12:59 -0800, Bruce Korb wrote:
> > If you are going to be excruciatingly, painfully correct, free() is
> > going to be unhappy about freeing a static string in the event
> > getcwd() fails for some inexplicable reason. I'd replace the free()
> +
> > return with a call to exit. Maybe even:
> 
> It's free (buf), not free (cwd).  buf won't point to a static string.
> 
> buf may be NULL though, but free (NULL) is legal (no-op).
> 
> 
> > > if (VERY_UNLIKELY (access (pz_curr_file, R_OK) != 0)) abort()
> 
> Perhaps just 
> 
> if (access (pz_curr_file, R_OK) != 0))
>   {
>     /* Some really inexplicable error happens. */
>     fprintf (stderr, "Cannot access %s: %s",
>              pz_curr_file, xstrerror (errno));
>     abort();
>   }
> 
> It will show which file can't be accessed so it's possible to
> diagnose.
> And the working directory will be outputed by "make" when the fixincl
> command fails anyway, so we don't need to really care it.

Re: [PATCH] fixincludes: simplify handling for access() failure [PR21283, PR80047]

[Bruce Korb]

Perfect.

On 11/12/21 1:58 PM, Xi Ruoyao wrote:
> diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
> index 6dba2f6e830..ee57fbf61b4 100644
> --- a/fixincludes/fixincl.c
> +++ b/fixincludes/fixincl.c
> @@ -1352,11 +1352,10 @@ process (void)
>   
>     if (access (pz_curr_file, R_OK) != 0)
>       {
> -      int erno = errno;
> -      fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
> -               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
> -               erno, xstrerror (erno));
> -      return;
> +      /* Some really strange error happened. */
> +      fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file,
> +	       xstrerror (errno));
> +      abort();
>       }
>   
>     pz_curr_data = load_file (pz_curr_file);

committed: [PATCH] fixincludes: simplify handling for access() failure [PR21283, PR80047]

[Xi Ruoyao]

On Sat, 2021-11-13 at 08:13 -0800, Bruce Korb wrote:
> Perfect.

Committed at r12-5234 with minor format fix.

> On 11/12/21 1:58 PM, Xi Ruoyao wrote:
> > diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
> > index 6dba2f6e830..ee57fbf61b4 100644
> > --- a/fixincludes/fixincl.c
> > +++ b/fixincludes/fixincl.c
> > @@ -1352,11 +1352,10 @@ process (void)
> >   
> >     if (access (pz_curr_file, R_OK) != 0)
> >       {
> > -      int erno = errno;
> > -      fprintf (stderr, "Cannot access %s from %s\n\terror %d
> > (%s)\n",
> > -               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
> > -               erno, xstrerror (erno));
> > -      return;
> > +      /* Some really strange error happened. */
> > +      fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file,
> > +              xstrerror (errno));
> > +      abort();
> >       }
> >   
> >     pz_curr_data = load_file (pz_curr_file);

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

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

[Jeff Law]

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