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

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

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