diff mbox series

[2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800]

Message ID 20211105211718.2261686-3-rep.dot.nop@gmail.com
State New
Headers show
Series incpath, Fortran: Fix memory leak in gfc_add_include_path | expand

Commit Message

Bernhard Reutner-Fischer Nov. 5, 2021, 9:17 p.m. UTC
From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org>

gcc/fortran/ChangeLog:

	PR fortran/68800
	* cpp.h (gfc_cpp_free_cpp_dirs): New declaration.
	* cpp.c (gfc_cpp_free_cpp_dirs): New definition.
	(gfc_cpp_add_include_path, gfc_cpp_add_include_path_after): Add
	comment.
	* f95-lang.c (gfc_finish): Call gfc_cpp_free_cpp_dirs.
	* scanner.c (add_path_to_list): Allocate unzeroed memory.
	(gfc_add_include_path): Add comment and fix formatting.

---
Bootstrapped and regtested without regressions on x86_64-unknown-linux.
Ok for trunk?

Note: in add_path_to_list i changed dir->path = XCNEWVEC to XNEWVEC
since strcpy and strcat add a terminating null byte for us anyway.
Fixes:

-== 68 bytes in 1 blocks are still reachable in loss record 228 of 371
-==    at : malloc (vg_replace_malloc.c:380)
-==    by : xmalloc (xmalloc.c:149)
-==    by : xstrdup (xstrdup.c:34)
-==    by : gfc_add_include_path(char const*, bool, bool, bool, bool) (scanner.c
:428)
-==    by : gfc_handle_option(unsigned long, char const*, long, int, unsigned in
t, cl_option_handlers const*) (options.c:702)
-==    by : handle_option(gcc_options*, gcc_options*, cl_decoded_option const*,
unsigned int, int, unsigned int, cl_option_handlers const*, bool, diagnostic_con
text*) (opts-common.c:1181)
-==    by : read_cmdline_option(gcc_options*, gcc_options*, cl_decoded_option*,
unsigned int, unsigned int, cl_option_handlers const*, diagnostic_context*) (opt
s-common.c:1431)
-==    by : read_cmdline_options (opts-global.c:237)
-==    by : decode_options(gcc_options*, gcc_options*, cl_decoded_option*, unsig
ned int, unsigned int, diagnostic_context*, void (*)()) (opts-global.c:319)
-==    by : toplev::main(int, char**) (toplev.c:2273)
-==    by : main (main.c:39)
---
 gcc/fortran/cpp.c      | 13 +++++++++++--
 gcc/fortran/cpp.h      |  1 +
 gcc/fortran/f95-lang.c |  2 +-
 gcc/fortran/scanner.c  |  7 ++++---
 4 files changed, 17 insertions(+), 6 deletions(-)

Comments

Harald Anlauf Nov. 6, 2021, 7:22 p.m. UTC | #1
Hi Bernhard,

I cannot comment on the gcc/ parts, but

Am 05.11.21 um 22:17 schrieb Bernhard Reutner-Fischer via Gcc-patches:
> From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org>
> 
> gcc/fortran/ChangeLog:
> 
> 	PR fortran/68800
> 	* cpp.h (gfc_cpp_free_cpp_dirs): New declaration.
> 	* cpp.c (gfc_cpp_free_cpp_dirs): New definition.
> 	(gfc_cpp_add_include_path, gfc_cpp_add_include_path_after): Add
> 	comment.
> 	* f95-lang.c (gfc_finish): Call gfc_cpp_free_cpp_dirs.
> 	* scanner.c (add_path_to_list): Allocate unzeroed memory.
> 	(gfc_add_include_path): Add comment and fix formatting.
> 
> ---
> Bootstrapped and regtested without regressions on x86_64-unknown-linux.
> Ok for trunk?
> 
> Note: in add_path_to_list i changed dir->path = XCNEWVEC to XNEWVEC
> since strcpy and strcat add a terminating null byte for us anyway.
> Fixes:
> 
> -== 68 bytes in 1 blocks are still reachable in loss record 228 of 371
> -==    at : malloc (vg_replace_malloc.c:380)
> -==    by : xmalloc (xmalloc.c:149)
> -==    by : xstrdup (xstrdup.c:34)
> -==    by : gfc_add_include_path(char const*, bool, bool, bool, bool) (scanner.c
> :428)
> -==    by : gfc_handle_option(unsigned long, char const*, long, int, unsigned in
> t, cl_option_handlers const*) (options.c:702)
> -==    by : handle_option(gcc_options*, gcc_options*, cl_decoded_option const*,
> unsigned int, int, unsigned int, cl_option_handlers const*, bool, diagnostic_con
> text*) (opts-common.c:1181)
> -==    by : read_cmdline_option(gcc_options*, gcc_options*, cl_decoded_option*,
> unsigned int, unsigned int, cl_option_handlers const*, diagnostic_context*) (opt
> s-common.c:1431)
> -==    by : read_cmdline_options (opts-global.c:237)
> -==    by : decode_options(gcc_options*, gcc_options*, cl_decoded_option*, unsig
> ned int, unsigned int, diagnostic_context*, void (*)()) (opts-global.c:319)
> -==    by : toplev::main(int, char**) (toplev.c:2273)
> -==    by : main (main.c:39)
> ---
>   gcc/fortran/cpp.c      | 13 +++++++++++--
>   gcc/fortran/cpp.h      |  1 +
>   gcc/fortran/f95-lang.c |  2 +-
>   gcc/fortran/scanner.c  |  7 ++++---
>   4 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c
> index e86386c8b17..04fe8fe460b 100644
> --- a/gcc/fortran/cpp.c
> +++ b/gcc/fortran/cpp.c
> @@ -728,12 +728,20 @@ gfc_cpp_done (void)
>     cpp_clear_file_cache (cpp_in);
>   }

why do you introduce a wrapper for something outside of fortran
that is used only once,

> -/* PATH must be malloc-ed and NULL-terminated.  */
> +/* Free all cpp include dirs.  */
> +void
> +gfc_cpp_free_cpp_dirs (void)
> +{
> +  free_cpp_dirs ();
> +}
> +
> +/* PATH must be NULL-terminated.  */
>   void
>   gfc_cpp_add_include_path (char *path, bool user_supplied)
>   {
>     /* CHAIN sets cpp_dir->sysp which differs from 0 if PATH is a system
> -     include path. Fortran does not define any system include paths.  */
> +     include path. Fortran does not define any system include paths.
> +     incpath.c manages the incoming, xstrdup()ed path.  */
>     int cxx_aware = 0;
>   
>     add_path (path, INC_BRACKET, cxx_aware, user_supplied);
> @@ -742,6 +750,7 @@ gfc_cpp_add_include_path (char *path, bool user_supplied)
>   void
>   gfc_cpp_add_include_path_after (char *path, bool user_supplied)
>   {
> +  /* incpath.c manages the incoming, xstrdup()ed path.  */
>     int cxx_aware = 0;
>     add_path (path, INC_AFTER, cxx_aware, user_supplied);
>   }
> diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h
> index 44644a2a333..963b9a9c89e 100644
> --- a/gcc/fortran/cpp.h
> +++ b/gcc/fortran/cpp.h
> @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool);
>   bool gfc_cpp_preprocess (const char *source_file);
>   
>   void gfc_cpp_done (void);
> +void gfc_cpp_free_cpp_dirs (void);
>   
>   void gfc_cpp_add_include_path (char *path, bool user_supplied);
>   void gfc_cpp_add_include_path_after (char *path, bool user_supplied);
> diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
> index 58dcaf01d75..ec4c2cf01d9 100644
> --- a/gcc/fortran/f95-lang.c
> +++ b/gcc/fortran/f95-lang.c
> @@ -275,7 +275,7 @@ gfc_finish (void)
>     gfc_cpp_done ();
>     gfc_done_1 ();
>     gfc_release_include_path ();
> -  return;

namely here?

> +  gfc_cpp_free_cpp_dirs ();
>   }

Why not call free_cpp_dirs () here directly, omit all unnecessary
stuff, and maybe only add a brief comment here?

>   /* These functions and variables deal with binding contours.  We only
> diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c
> index 69b81ab97f8..268d1e3e7fb 100644
> --- a/gcc/fortran/scanner.c
> +++ b/gcc/fortran/scanner.c
> @@ -370,7 +370,7 @@ add_path_to_list (gfc_directorylist **list, const char *path,
>     char *q;
>     size_t len;
>     int i;
> -
> +
>     p = path;
>     while (*p == ' ' || *p == '\t')  /* someone might do "-I include" */
>       if (*p++ == '\0')
> @@ -409,7 +409,7 @@ add_path_to_list (gfc_directorylist **list, const char *path,
>       *list = dir;
>     dir->use_for_modules = use_for_modules;
>     dir->warn = warn; > -  dir->path = XCNEWVEC (char, strlen (p) + 2);
> +  dir->path = XNEWVEC (char, strlen (p) + 2);
>     strcpy (dir->path, p);
>     strcat (dir->path, "/");	/* make '/' last character */
>   }
> @@ -424,8 +424,9 @@ gfc_add_include_path (const char *path, bool use_for_modules, bool file_dir,
>   		    defer_warn);
>   
>     /* For '#include "..."' these directories are automatically searched.  */
> +  /* CPP manages 'path' on it's own via incpath.c so give it it's own copy.  */
>     if (!file_dir)
> -    gfc_cpp_add_include_path (xstrdup(path), true);
> +    gfc_cpp_add_include_path (xstrdup (path), true);
>   }
>   
>   
> 

Harald
Bernhard Reutner-Fischer Nov. 7, 2021, 1:38 a.m. UTC | #2
On Sat, 6 Nov 2021 20:22:53 +0100
Harald Anlauf <anlauf@gmx.de> wrote:

> Hi Bernhard,
> 
> I cannot comment on the gcc/ parts, but
> 

> > diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c
> > index e86386c8b17..04fe8fe460b 100644
> > --- a/gcc/fortran/cpp.c
> > +++ b/gcc/fortran/cpp.c
> > @@ -728,12 +728,20 @@ gfc_cpp_done (void)
> >     cpp_clear_file_cache (cpp_in);
> >   }  
> 
> why do you introduce a wrapper for something outside of fortran
> that is used only once,
> 
> > -/* PATH must be malloc-ed and NULL-terminated.  */
> > +/* Free all cpp include dirs.  */
> > +void
> > +gfc_cpp_free_cpp_dirs (void)
> > +{
> > +  free_cpp_dirs ();
> > +}

> > diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h
> > index 44644a2a333..963b9a9c89e 100644
> > --- a/gcc/fortran/cpp.h
> > +++ b/gcc/fortran/cpp.h
> > @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool);
> >   bool gfc_cpp_preprocess (const char *source_file);
> >
> >   void gfc_cpp_done (void);
> > +void gfc_cpp_free_cpp_dirs (void);
> >
> >   void gfc_cpp_add_include_path (char *path, bool user_supplied);
> >   void gfc_cpp_add_include_path_after (char *path, bool user_supplied);
> > diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
> > index 58dcaf01d75..ec4c2cf01d9 100644
> > --- a/gcc/fortran/f95-lang.c
> > +++ b/gcc/fortran/f95-lang.c
> > @@ -275,7 +275,7 @@ gfc_finish (void)
> >     gfc_cpp_done ();
> >     gfc_done_1 ();
> >     gfc_release_include_path ();
> > -  return;  
> 
> namely here?
> 
> > +  gfc_cpp_free_cpp_dirs ();
> >   }  
> 
> Why not call free_cpp_dirs () here directly, omit all unnecessary
> stuff, and maybe only add a brief comment here?

cpp.c includes incpath.h, f95-lang.c does not and should not.
So the cleanest thing is to keep the cpp handling in cpp.[ch] and have
the language frontend call into it's cpp bits.

It would be rather rogue to
extern void free_cpp_dirs (void);
in f95-lang.c and directly call it in gfc_finish, i'd say?

thanks,
Bernhard Reutner-Fischer April 2, 2023, 7:27 p.m. UTC | #3
ping?

libcpp maintainers, is the helper in incpath.* ok?

fortraners,
Do you prefer a rogue, local forward declaration, or is the
introduction of that trivial wrapper ok? I don't think pulling in cpp.h
from f95-lang.cc is desirable, but i can do that if you all think
that's preferred.

cover-letter:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583522.html

gcc/incpath.* bits (i guess that'd be for the libcpp maintainers):
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583520.html

fortran bits:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583521.html

thanks,

On Sun, 7 Nov 2021 02:38:21 +0100
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:

> On Sat, 6 Nov 2021 20:22:53 +0100
> Harald Anlauf <anlauf@gmx.de> wrote:
> 
> > Hi Bernhard,
> > 
> > I cannot comment on the gcc/ parts, but
> >   
> 
> > > diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c
> > > index e86386c8b17..04fe8fe460b 100644
> > > --- a/gcc/fortran/cpp.c
> > > +++ b/gcc/fortran/cpp.c
> > > @@ -728,12 +728,20 @@ gfc_cpp_done (void)
> > >     cpp_clear_file_cache (cpp_in);
> > >   }    
> > 
> > why do you introduce a wrapper for something outside of fortran
> > that is used only once,
> >   
> > > -/* PATH must be malloc-ed and NULL-terminated.  */
> > > +/* Free all cpp include dirs.  */
> > > +void
> > > +gfc_cpp_free_cpp_dirs (void)
> > > +{
> > > +  free_cpp_dirs ();
> > > +}  
> 
> > > diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h
> > > index 44644a2a333..963b9a9c89e 100644
> > > --- a/gcc/fortran/cpp.h
> > > +++ b/gcc/fortran/cpp.h
> > > @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool);
> > >   bool gfc_cpp_preprocess (const char *source_file);
> > >
> > >   void gfc_cpp_done (void);
> > > +void gfc_cpp_free_cpp_dirs (void);
> > >
> > >   void gfc_cpp_add_include_path (char *path, bool user_supplied);
> > >   void gfc_cpp_add_include_path_after (char *path, bool user_supplied);
> > > diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
> > > index 58dcaf01d75..ec4c2cf01d9 100644
> > > --- a/gcc/fortran/f95-lang.c
> > > +++ b/gcc/fortran/f95-lang.c
> > > @@ -275,7 +275,7 @@ gfc_finish (void)
> > >     gfc_cpp_done ();
> > >     gfc_done_1 ();
> > >     gfc_release_include_path ();
> > > -  return;    
> > 
> > namely here?
> >   
> > > +  gfc_cpp_free_cpp_dirs ();
> > >   }    
> > 
> > Why not call free_cpp_dirs () here directly, omit all unnecessary
> > stuff, and maybe only add a brief comment here?  
> 
> cpp.c includes incpath.h, f95-lang.c does not and should not.
> So the cleanest thing is to keep the cpp handling in cpp.[ch] and have
> the language frontend call into it's cpp bits.
> 
> It would be rather rogue to
> extern void free_cpp_dirs (void);
> in f95-lang.c and directly call it in gfc_finish, i'd say?
> 
> thanks,
diff mbox series

Patch

diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c
index e86386c8b17..04fe8fe460b 100644
--- a/gcc/fortran/cpp.c
+++ b/gcc/fortran/cpp.c
@@ -728,12 +728,20 @@  gfc_cpp_done (void)
   cpp_clear_file_cache (cpp_in);
 }
 
-/* PATH must be malloc-ed and NULL-terminated.  */
+/* Free all cpp include dirs.  */
+void
+gfc_cpp_free_cpp_dirs (void)
+{
+  free_cpp_dirs ();
+}
+
+/* PATH must be NULL-terminated.  */
 void
 gfc_cpp_add_include_path (char *path, bool user_supplied)
 {
   /* CHAIN sets cpp_dir->sysp which differs from 0 if PATH is a system
-     include path. Fortran does not define any system include paths.  */
+     include path. Fortran does not define any system include paths.
+     incpath.c manages the incoming, xstrdup()ed path.  */
   int cxx_aware = 0;
 
   add_path (path, INC_BRACKET, cxx_aware, user_supplied);
@@ -742,6 +750,7 @@  gfc_cpp_add_include_path (char *path, bool user_supplied)
 void
 gfc_cpp_add_include_path_after (char *path, bool user_supplied)
 {
+  /* incpath.c manages the incoming, xstrdup()ed path.  */
   int cxx_aware = 0;
   add_path (path, INC_AFTER, cxx_aware, user_supplied);
 }
diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h
index 44644a2a333..963b9a9c89e 100644
--- a/gcc/fortran/cpp.h
+++ b/gcc/fortran/cpp.h
@@ -46,6 +46,7 @@  void gfc_cpp_post_options (bool);
 bool gfc_cpp_preprocess (const char *source_file);
 
 void gfc_cpp_done (void);
+void gfc_cpp_free_cpp_dirs (void);
 
 void gfc_cpp_add_include_path (char *path, bool user_supplied);
 void gfc_cpp_add_include_path_after (char *path, bool user_supplied);
diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
index 58dcaf01d75..ec4c2cf01d9 100644
--- a/gcc/fortran/f95-lang.c
+++ b/gcc/fortran/f95-lang.c
@@ -275,7 +275,7 @@  gfc_finish (void)
   gfc_cpp_done ();
   gfc_done_1 ();
   gfc_release_include_path ();
-  return;
+  gfc_cpp_free_cpp_dirs ();
 }
 
 /* These functions and variables deal with binding contours.  We only
diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c
index 69b81ab97f8..268d1e3e7fb 100644
--- a/gcc/fortran/scanner.c
+++ b/gcc/fortran/scanner.c
@@ -370,7 +370,7 @@  add_path_to_list (gfc_directorylist **list, const char *path,
   char *q;
   size_t len;
   int i;
-  
+
   p = path;
   while (*p == ' ' || *p == '\t')  /* someone might do "-I include" */
     if (*p++ == '\0')
@@ -409,7 +409,7 @@  add_path_to_list (gfc_directorylist **list, const char *path,
     *list = dir;
   dir->use_for_modules = use_for_modules;
   dir->warn = warn;
-  dir->path = XCNEWVEC (char, strlen (p) + 2);
+  dir->path = XNEWVEC (char, strlen (p) + 2);
   strcpy (dir->path, p);
   strcat (dir->path, "/");	/* make '/' last character */
 }
@@ -424,8 +424,9 @@  gfc_add_include_path (const char *path, bool use_for_modules, bool file_dir,
 		    defer_warn);
 
   /* For '#include "..."' these directories are automatically searched.  */
+  /* CPP manages 'path' on it's own via incpath.c so give it it's own copy.  */
   if (!file_dir)
-    gfc_cpp_add_include_path (xstrdup(path), true);
+    gfc_cpp_add_include_path (xstrdup (path), true);
 }