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

2014-11-18 Thread 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

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

2014-11-17 Thread 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
jos...@codesourcery.com

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

2014-11-15 Thread 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