Bug 21823 - MAXPATHLEN usage in [gcc]/fixincludes
Summary: MAXPATHLEN usage in [gcc]/fixincludes
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: other (show other bugs)
Version: 4.0.0
: P2 trivial
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL: https://gcc.gnu.org/pipermail/gcc-pat...
Keywords: build, easyhack, patch
Depends on:
Blocks: 21824
  Show dependency treegraph
 
Reported: 2005-05-30 15:09 UTC by Alfred M. Szmidt
Modified: 2021-11-15 00:13 UTC (History)
5 users (show)

See Also:
Host: i686-pc-gnu0.3
Target: i686-pc-gnu0.3
Build: i686-pc-gnu0.3
Known to work:
Known to fail:
Last reconfirmed: 2005-06-19 14:01:30


Attachments
Don't use arbitrary limits. (703 bytes, patch)
2005-10-01 16:37 UTC, Alfred M. Szmidt
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alfred M. Szmidt 2005-05-30 15:09:03 UTC
The way MAXPATHLEN is used in fixincludes (server.c and fixincl.c) is wrong,
instead of defining a bogus value on platforms that do not have MAXPATHLEN
defined (i.e. GNU) one should try and use getcwd as follows:

  char *dir = getcwd (NULL, 0);

instead of passing a buffer and a size.

This will only work on systems that use the GNU C Library.
Comment 1 Andrew Pinski 2005-06-19 14:01:30 UTC
Confirmed.
Comment 2 Alfred M. Szmidt 2005-10-01 16:37:56 UTC
Created attachment 9857 [details]
Don't use arbitrary limits.

The following fixes fixincludes.

fixincludes/ChangeLog
2005-09-16  Alfred M. Szmidt  <ams@gnu.org>

	* fixincl.c (quoted_file_exists): Use xmalloc to allocate memory
	for FNAME.
	(create_file): Use xmalloc to allocate memory for FNAME.

	* server.c (server_setup): Use dynamic allocation for BUFF.
Comment 3 Eric Gallager 2017-10-24 09:58:01 UTC
(In reply to Alfred M. Szmidt from comment #2)
> Created attachment 9857 [details]
> Don't use arbitrary limits.
> 
> The following fixes fixincludes.
> 
> fixincludes/ChangeLog
> 2005-09-16  Alfred M. Szmidt  <ams@gnu.org>
> 
> 	* fixincl.c (quoted_file_exists): Use xmalloc to allocate memory
> 	for FNAME.
> 	(create_file): Use xmalloc to allocate memory for FNAME.
> 
> 	* server.c (server_setup): Use dynamic allocation for BUFF.

Please send this patch to the gcc-patches mailing list for review, if it still applies
Comment 4 Alfred M. Szmidt 2017-10-30 18:16:49 UTC
   > Created attachment 9857 [details]
   > Don't use arbitrary limits.
   > 
   > The following fixes fixincludes.
   > 
   > fixincludes/ChangeLog
   > 2005-09-16  Alfred M. Szmidt  <ams@gnu.org>
   > 
   > 	* fixincl.c (quoted_file_exists): Use xmalloc to allocate memory
   > 	for FNAME.
   > 	(create_file): Use xmalloc to allocate memory for FNAME.
   > 
   > 	* server.c (server_setup): Use dynamic allocation for BUFF.

   Please send this patch to the gcc-patches mailing list for review, if it still
   applies

MAXPATHLEN is still used in fixincludes.  Seeing that this patch is
over 10 years, I am not sure it even applies and thus a good idea to
forward it to gcc-patches for review.  The fix is simple enough in
fixincludes (simply use xmalloc).
Comment 5 Eric Gallager 2018-09-21 02:01:37 UTC
(In reply to Alfred M. Szmidt from comment #4)
>    > Created attachment 9857 [details]
>    > Don't use arbitrary limits.
>    > 
>    > The following fixes fixincludes.
>    > 
>    > fixincludes/ChangeLog
>    > 2005-09-16  Alfred M. Szmidt  <ams@gnu.org>
>    > 
>    > 	* fixincl.c (quoted_file_exists): Use xmalloc to allocate memory
>    > 	for FNAME.
>    > 	(create_file): Use xmalloc to allocate memory for FNAME.
>    > 
>    > 	* server.c (server_setup): Use dynamic allocation for BUFF.
> 
>    Please send this patch to the gcc-patches mailing list for review, if it
> still
>    applies
> 
> MAXPATHLEN is still used in fixincludes.  Seeing that this patch is
> over 10 years, I am not sure it even applies and thus a good idea to
> forward it to gcc-patches for review.  The fix is simple enough in
> fixincludes (simply use xmalloc).

ok, adding "easyhack" keyword then
Comment 6 Eric Gallager 2021-11-11 13:07:48 UTC
There's a patch for related bug 80047 that would make this worse: https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583820.html
Comment 7 Xi Ruoyao 2021-11-11 16:51:55 UTC
New patch, for both PR 80047 and this one.
Comment 8 Xi Ruoyao 2021-11-11 16:52:12 UTC
(In reply to Xi Ruoyao from comment #7)
> New patch, for both PR 80047 and this one.

https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584164.html
Comment 9 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 10 Eric Gallager 2021-11-13 21:31:45 UTC
(In reply to CVS Commits from comment #9)
> 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.

Is it ok to close this as FIXED now? Or should we leave it open for backports?
Comment 11 Eric Gallager 2021-11-15 00:13:30 UTC
(In reply to Eric Gallager from comment #10)
> Is it ok to close this as FIXED now? Or should we leave it open for
> backports?

Closing per comments in bug 80047