[PATCH] Check 'fd' neither -1 nor 0, before close it

[PATCH] Check 'fd' neither -1 nor 0, before close it

[Chen Gang]

'fd' may be 0 which does not need 'open' operation, so neither need
'close' operation on it when it is 0.

Also in c_common_read_pch(), when failure occurs, also need be sure the
'fd' is not '-1' for the next close operation.

It passes testsuite under Fedora x86_64-unknown-linux-gnu.


gcc/

	* c-family/c-pch.c (c_common_read_pch): Check 'fd' neither -1
	nor 0, before close it,

libcpp/

	* files.c (_cpp_compare_file_date, read_file, validate_pch
	open_file, _cpp_save_file_entries): Check 'fd' neither -1 nor 0,
	before close it.
---
 gcc/c-family/c-pch.c | 10 ++++++----
 libcpp/files.c       | 20 +++++++++++---------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/gcc/c-family/c-pch.c b/gcc/c-family/c-pch.c
index 93609b6..d001965 100644
--- a/gcc/c-family/c-pch.c
+++ b/gcc/c-family/c-pch.c
@@ -355,7 +355,8 @@ c_common_read_pch (cpp_reader *pfile, const char *name,
   if (f == NULL)
     {
       cpp_errno (pfile, CPP_DL_ERROR, "calling fdopen");
-      close (fd);
+      if (fd && fd != -1)
+	close (fd);
       goto end;
     }
 
@@ -376,14 +377,15 @@ c_common_read_pch (cpp_reader *pfile, const char *name,
   timevar_push (TV_PCH_CPP_RESTORE);
   if (cpp_read_state (pfile, name, f, smd) != 0)
     {
-      fclose (f);
+      if (fd)
+	fclose (f);
       timevar_pop (TV_PCH_CPP_RESTORE);
       goto end;
     }
   timevar_pop (TV_PCH_CPP_RESTORE);
 
-
-  fclose (f);
+  if (fd)
+    fclose (f);
 
   line_table->trace_includes = saved_trace_includes;
   linemap_add (line_table, LC_ENTER, 0, saved_loc.file, saved_loc.line);
diff --git a/libcpp/files.c b/libcpp/files.c
index 3984821..5c845da 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -243,7 +243,8 @@ open_file (_cpp_file *file)
 	  errno = ENOENT;
 	}
 
-      close (file->fd);
+      if (file->fd)
+        close (file->fd);
       file->fd = -1;
     }
 #if defined(_WIN32) && !defined(__CYGWIN__)
@@ -753,7 +754,8 @@ read_file (cpp_reader *pfile, _cpp_file *file)
     }
 
   file->dont_read = !read_file_guts (pfile, file);
-  close (file->fd);
+  if (file->fd)
+    close (file->fd);
   file->fd = -1;
 
   return !file->dont_read;
@@ -1435,11 +1437,9 @@ _cpp_compare_file_date (cpp_reader *pfile, const char *fname,
   if (file->err_no)
     return -1;
 
-  if (file->fd != -1)
-    {
-      close (file->fd);
-      file->fd = -1;
-    }
+  if (file->fd && file->fd != -1)
+    close (file->fd);
+  file->fd = -1;
 
   return file->st.st_mtime > pfile->buffer->file->st.st_mtime;
 }
@@ -1694,7 +1694,8 @@ validate_pch (cpp_reader *pfile, _cpp_file *file, const char *pchname)
 
       if (!valid)
 	{
-	  close (file->fd);
+	  if (file->fd)
+	    close (file->fd);
 	  file->fd = -1;
 	}
 
@@ -1849,7 +1850,8 @@ _cpp_save_file_entries (cpp_reader *pfile, FILE *fp)
 	    }
 	  ff = fdopen (f->fd, "rb");
 	  md5_stream (ff, result->entries[count].sum);
-	  fclose (ff);
+	  if (f->fd)
+	    fclose (ff);
 	  f->fd = oldfd;
 	}
       result->entries[count].size = f->st.st_size;
-- 
1.9.3

Re: [PATCH] Check 'fd' neither -1 nor 0, before close it

[Joseph Myers]

On Sat, 15 Nov 2014, Chen Gang wrote:

> Also in c_common_read_pch(), when failure occurs, also need be sure the
> 'fd' is not '-1' for the next close operation.

Please clarify how c_common_read_pch gets called with fd == -1.  
Certainly checking after an error is too late; we shouldn't call fdopen in 
that case at all, and I think something further up the call chain should 
have avoided calling c_common_read_pch with fd == -1 at all (the question 
is just exactly what point on the call chain is missing such a check and 
needs it added).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Check 'fd' neither -1 nor 0, before close it

[Chen Gang]

On 11/18/14 0:38, Joseph Myers wrote:
> On Sat, 15 Nov 2014, Chen Gang wrote:
> 
>> Also in c_common_read_pch(), when failure occurs, also need be sure the
>> 'fd' is not '-1' for the next close operation.
> 
> Please clarify how c_common_read_pch gets called with fd == -1.  
> Certainly checking after an error is too late; we shouldn't call fdopen in 
> that case at all, and I think something further up the call chain should 
> have avoided calling c_common_read_pch with fd == -1 at all (the question 
> is just exactly what point on the call chain is missing such a check and 
> needs it added).
> 

According to current source code, the author wants 'fd' should never be
'-1' in c_common_read_pch(). "grep -rn read_pch *" in root directory,
then "grep -rn _cpp_stack_file *", can know it is used in 3 areas:

 - c_common_pch_pragma() in "gcc/c-family/c-pch.c", it has already
   checked 'fd' before call c_common_read_pch()

 - _cpp_stack_include() in "libcpp/files.c", before _cpp_stack_file(),
   has called and checked _cpp_find_file().

 - cpp_read_main_file() in "libcpp/init.c", before _cpp_stack_file(),
   has called and checked _cpp_find_file().

In c_common_read_pch(), even if 'fd' is '-1', we should check it firstly
before call fdopen(), instead of check '-1' in failure code block.

But for me, it contents another 2 related issues:

 - _cpp_find_file() always return a valid pointer, so related check code
   "file == NULL" is no use for _cpp_find_file() in _cpp_stack_include().

 - According to its comments, _cpp_find_file() can not be sure of
   'file->fd' must not be '-1', even when "file->err_no == 0", we need
   reopen it if necessary.

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed