| Message ID | 54671A9A.7060908@gmail.com |
|---|---|
| State | New |
| Headers | show |
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).
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.
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;