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

Message ID 14343662168642b2d975044fccf5e235695bedc7.camel@mengyan1223.wang
State New
Headers
Series fixincludes: fix portability issues about getcwd() [PR21283, PR80047] |

Commit Message

Xi Ruoyao Nov. 11, 2021, 4:33 p.m. UTC
  [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(-)
  

Comments

Bruce Korb Nov. 12, 2021, 8:59 p.m. UTC | #1
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;
>       }
>
  
Xi Ruoyao Nov. 12, 2021, 9:08 p.m. UTC | #2
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;
> >      }
> >  
>
  

Patch

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