Patchwork Fix libcpp memory leak (PR middle-end/56461)

login
register
mail settings
Submitter Jakub Jelinek
Date Feb. 28, 2013, 8:12 p.m.
Message ID <20130228201235.GY12913@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/224179/
State New
Headers show

Comments

Jakub Jelinek - Feb. 28, 2013, 8:12 p.m.
Hi!

On #include <signal.h> on Linux we leak memory in the preprocessor.
The problem is that signal.h doesn't have standard multiple inclusion
guards, the multiple inclusion guard is defined only conditionally
and the whole header isn't protected by that guard, just big part of it.
Furthermore, signal.h includes sys/context.h, which later includes signal.h
again.  That means we call read_file on the signal.h header while it is
stacked (so file->buffer != NULL && !file->buffer_valid) and just overwrite
file->buffer with a newly allocated memory.  Later on when doing
_cpp_pop_file_buffer on the inner signal.h we free the second buffer
and clear file->buffer{,_start,_valid} fields, then later on when the outer
signal.h (same _cpp_file structure) is being unstacked, those fields are
already NULL and we don't free anything, thus we leak the memory.

Fixed by making struct cpp_buffer be the owner of the buffer, responsible
for freeing it, rather than struct _cpp_file.  Bootstrapped/regtested on
x86_64-linux and i686-linux, ok for trunk?

2013-02-28  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/56461
	* internal.h (struct cpp_buffer): Add to_free field.
	(_cpp_pop_file_buffer): Add third argument.
	* files.c (_cpp_stack_file): Set buffer->to_free.
	(_cpp_pop_file_buffer): Add to_free argument.  Free to_free
	if non-NULL, and if equal to file->buffer_start, also clear
	file->buffer{,_start,_valid}.
	* directives.c (_cpp_pop_buffer): Pass buffer->to_free
	to _cpp_pop_file_buffer.


	Jakub
Tom Tromey - March 6, 2013, 4:12 p.m.
>>>>> "Jakub" == Jakub Jelinek <jakub@redhat.com> writes:

Jakub> 2013-02-28  Jakub Jelinek  <jakub@redhat.com>
Jakub> 	PR middle-end/56461
Jakub> 	* internal.h (struct cpp_buffer): Add to_free field.
Jakub> 	(_cpp_pop_file_buffer): Add third argument.
Jakub> 	* files.c (_cpp_stack_file): Set buffer->to_free.
Jakub> 	(_cpp_pop_file_buffer): Add to_free argument.  Free to_free
Jakub> 	if non-NULL, and if equal to file->buffer_start, also clear
Jakub>  file-> buffer{,_start,_valid}.
Jakub> 	* directives.c (_cpp_pop_buffer): Pass buffer->to_free
Jakub> 	to _cpp_pop_file_buffer.

This is ok.  Thanks, Jakub.

Tom

Patch

--- libcpp/internal.h.jj	2013-02-27 14:05:39.000000000 +0100
+++ libcpp/internal.h	2013-02-28 17:09:41.263168009 +0100
@@ -301,6 +301,8 @@  struct cpp_buffer
 
   const unsigned char *buf;        /* Entire character buffer.  */
   const unsigned char *rlimit;     /* Writable byte at end of file.  */
+  const unsigned char *to_free;	   /* Pointer that should be freed when
+				      popping the buffer.  */
 
   _cpp_line_note *notes;           /* Array of notes.  */
   unsigned int cur_note;           /* Next note to process.  */
@@ -635,7 +637,8 @@  extern int _cpp_compare_file_date (cpp_r
 extern void _cpp_report_missing_guards (cpp_reader *);
 extern void _cpp_init_files (cpp_reader *);
 extern void _cpp_cleanup_files (cpp_reader *);
-extern void _cpp_pop_file_buffer (cpp_reader *, struct _cpp_file *);
+extern void _cpp_pop_file_buffer (cpp_reader *, struct _cpp_file *,
+				  const unsigned char *);
 extern bool _cpp_save_file_entries (cpp_reader *pfile, FILE *f);
 extern bool _cpp_read_file_entries (cpp_reader *, FILE *);
 extern const char *_cpp_get_file_name (_cpp_file *);
--- libcpp/files.c.jj	2013-02-28 10:57:50.000000000 +0100
+++ libcpp/files.c	2013-02-28 17:10:06.023024353 +0100
@@ -877,6 +877,7 @@  _cpp_stack_file (cpp_reader *pfile, _cpp
 			    && !CPP_OPTION (pfile, directives_only));
   buffer->file = file;
   buffer->sysp = sysp;
+  buffer->to_free = file->buffer_start;
 
   /* Initialize controlling macro state.  */
   pfile->mi_valid = true;
@@ -1418,7 +1419,8 @@  cpp_push_default_include (cpp_reader *pf
 /* Do appropriate cleanup when a file INC's buffer is popped off the
    input stack.  */
 void
-_cpp_pop_file_buffer (cpp_reader *pfile, _cpp_file *file)
+_cpp_pop_file_buffer (cpp_reader *pfile, _cpp_file *file,
+		      const unsigned char *to_free)
 {
   /* Record the inclusion-preventing macro, which could be NULL
      meaning no controlling macro.  */
@@ -1428,12 +1430,15 @@  _cpp_pop_file_buffer (cpp_reader *pfile,
   /* Invalidate control macros in the #including file.  */
   pfile->mi_valid = false;
 
-  if (file->buffer_start)
+  if (to_free)
     {
-      free ((void *) file->buffer_start);
-      file->buffer_start = NULL;
-      file->buffer = NULL;
-      file->buffer_valid = false;
+      if (to_free == file->buffer_start)
+	{
+	  file->buffer_start = NULL;
+	  file->buffer = NULL;
+	  file->buffer_valid = false;
+	}
+      free ((void *) to_free);
     }
 }
 
--- libcpp/directives.c.jj	2013-01-15 09:04:55.000000000 +0100
+++ libcpp/directives.c	2013-02-28 17:09:53.239097488 +0100
@@ -2558,6 +2558,7 @@  _cpp_pop_buffer (cpp_reader *pfile)
   cpp_buffer *buffer = pfile->buffer;
   struct _cpp_file *inc = buffer->file;
   struct if_stack *ifs;
+  const unsigned char *to_free;
 
   /* Walk back up the conditional stack till we reach its level at
      entry to this file, issuing error messages.  */
@@ -2571,6 +2572,7 @@  _cpp_pop_buffer (cpp_reader *pfile)
   /* _cpp_do_file_change expects pfile->buffer to be the new one.  */
   pfile->buffer = buffer->prev;
 
+  to_free = buffer->to_free;
   free (buffer->notes);
 
   /* Free the buffer object now; we may want to push a new buffer
@@ -2579,7 +2581,7 @@  _cpp_pop_buffer (cpp_reader *pfile)
 
   if (inc)
     {
-      _cpp_pop_file_buffer (pfile, inc);
+      _cpp_pop_file_buffer (pfile, inc, to_free);
 
       _cpp_do_file_change (pfile, LC_LEAVE, 0, 0, 0);
     }