Bug 80047 - fixincludes/fixincl.c: PVS-Studio: Improper Release of Memory Before Removing Last Reference (CWE-401)
Summary: fixincludes/fixincl.c: PVS-Studio: Improper Release of Memory Before Removing...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: bootstrap (show other bugs)
Version: 7.0.1
: P3 normal
Target Milestone: ---
Assignee: Martin Sebor
URL: https://gcc.gnu.org/pipermail/gcc-pat...
Keywords: build, patch
Depends on:
Blocks: cppcheck
  Show dependency treegraph
 
Reported: 2017-03-15 14:38 UTC by Phillip Khandeliants
Modified: 2021-11-15 00:12 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-03-15 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Phillip Khandeliants 2017-03-15 14:38:19 UTC
We have found a weakness (CWE-401) using PVS-Studio tool. PVS-Studio is a static code analyzer for C, C++ and C#: https://www.viva64.com/en/pvs-studio/

Analyzer warning: V575 The null pointer is passed into 'getcwd' function. Inspect the first argument. fixincl.c 1357

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

As an extension to the POSIX.1-2001 standard, glibc's getcwd() allocates the buffer dynamically using malloc if buf is NULL. In this case, the allocated buffer has the length size unless size is zero, when buf is allocated as big as necessary.  The caller should free the returned buffer.
Comment 1 Martin Sebor 2017-03-15 20:21:21 UTC
Confirmed.  Since the Glibc extension may not be provided by other systems I think the portable solution is to avoid relying on it.  Since getcwd() may fail by returning 0, the caller should also avoid assuming it succeeds.  Let me post the following patch:

diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
index 6dba2f6..6e6eb21 100644
--- a/fixincludes/fixincl.c
+++ b/fixincludes/fixincl.c
@@ -1353,8 +1353,10 @@ process (void)
   if (access (pz_curr_file, R_OK) != 0)
     {
       int erno = errno;
+      char cwdbuf[MAXPATHLEN];
+      char *cwd = getcwd (cwdbuf, sizeof cwdbuf);
       fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
-               pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
+               pz_curr_file, cwd ? cwd : "current working directory",
                erno, xstrerror (erno));
       return;
     }
Comment 2 Andrew Pinski 2017-03-15 20:25:32 UTC
>char cwdbuf[MAXPATHLEN];

This is not a GNU style thing.  GNU style mentions against using arbitrary limits.
Comment 3 Martin Sebor 2017-03-15 20:36:35 UTC
Patch posted for review:
https://gcc.gnu.org/ml/gcc-patches/2017-03/msg00853.html

(In reply to Andrew Pinski from comment #2)
> >char cwdbuf[MAXPATHLEN];
> 
> This is not a GNU style thing.  GNU style mentions against using arbitrary
> limits.

There are existing uses of MAXPATHLEN to declare local arrays in this file (create_file, quoted_file_exists) so this one doesn't make things any worse than they already are.
Comment 4 Eric Gallager 2017-07-22 18:55:11 UTC
(In reply to Martin Sebor from comment #3)
> Patch posted for review:
> https://gcc.gnu.org/ml/gcc-patches/2017-03/msg00853.html
> 

Looks like it was approved for gcc8 but never actually committed...
Comment 5 Eric Gallager 2017-07-24 04:46:19 UTC
(In reply to Andrew Pinski from comment #2)
> >char cwdbuf[MAXPATHLEN];
> 
> This is not a GNU style thing.  GNU style mentions against using arbitrary
> limits.

That's bug 21823 btw.
Comment 6 Eric Gallager 2021-11-11 13:05:47 UTC
new patch: https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583820.html
Comment 7 Xi Ruoyao 2021-11-11 16:53:02 UTC
New patch for PR 21823 and this one:

https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584164.html
Comment 8 CVS Commits 2021-11-13 18:34:16 UTC
The master branch has been updated by Xi Ruoyao <xry111@gcc.gnu.org>:

https://gcc.gnu.org/g:04c5a91d068c4ca2f09c2bc206fce00db9d1790b

commit r12-5234-g04c5a91d068c4ca2f09c2bc206fce00db9d1790b
Author: Xi Ruoyao <xry111@mengyan1223.wang>
Date:   Tue Nov 9 21:40:04 2021 +0800

    fixincludes: simplify handling for access() failure [PR21283, PR80047]
    
    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.
Comment 9 Eric Gallager 2021-11-13 21:31:08 UTC
(In reply to CVS Commits from comment #8)
> The master branch has been updated by Xi Ruoyao <xry111@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:04c5a91d068c4ca2f09c2bc206fce00db9d1790b
> 
> commit r12-5234-g04c5a91d068c4ca2f09c2bc206fce00db9d1790b
> Author: Xi Ruoyao <xry111@mengyan1223.wang>
> Date:   Tue Nov 9 21:40:04 2021 +0800
> 
>     fixincludes: simplify handling for access() failure [PR21283, PR80047]
>     
>     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.

So... ok to close as FIXED then? Or leave open for backports?
Comment 10 Xi Ruoyao 2021-11-14 11:58:46 UTC
Fixed in trunk.

I'm not sure if this should be backported.
Comment 11 Eric Gallager 2021-11-15 00:12:33 UTC
(In reply to Xi Ruoyao from comment #10)
> Fixed in trunk.
> 
> I'm not sure if this should be backported.

ok, I'll close this and the other, then.