commit 2c31f86d436bcd434c710da5ad8aee7c439dcb14
author Simon Que <sque@chromium.org> Thu Sep 22 13:02:11 2016 -0700
committer Simon Que <sque@chromium.org> Thu Oct 06 18:13:31 2016 +0000
tree 4ebe958dc0b4b5cd98e36e6b240a41ac3f2b6ac6
parent 5cb7844ef66ad2af828b573a34002fb866e07f0b

quipper: Don't call fclose() on null FILE pointer

The behavior is undefined. Check for it in FileReader.

Also replaced some fopen/fclose file reads with FileReader.

BUG=chromium:649242
TEST=build successfully

Change-Id: I111cb3263abdfbc6f232517f47f296646e3f60c3
Previous-Reviewed-on: https://chromium-review.googlesource.com/394231
(cherry picked from commit f9583eeb3849b1616d9e6af28222608ff0d665a6)
Reviewed-on: https://chromium-review.googlesource.com/394231
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>
Tested-by: Simon Que <sque@chromium.org>

chromiumos-wide-profiling/file_reader.cc

diff --git a/chromiumos-wide-profiling/file_reader.cc b/chromiumos-wide-profiling/file_reader.cc
index c2cb3e5..0fbb154 100644
--- a/chromiumos-wide-profiling/file_reader.cc
+++ b/chromiumos-wide-profiling/file_reader.cc
@@ -25,7 +25,9 @@
 }
 
 FileReader::~FileReader() {
-  fclose(infile_);
+  if (IsOpen()) {
+    fclose(infile_);
+  }
 }
 
 bool FileReader::ReadData(const size_t size, void* dest) {

chromiumos-wide-profiling/test_utils.cc

diff --git a/chromiumos-wide-profiling/test_utils.cc b/chromiumos-wide-profiling/test_utils.cc
index 2a7a327..290e16a 100644
--- a/chromiumos-wide-profiling/test_utils.cc
+++ b/chromiumos-wide-profiling/test_utils.cc
@@ -7,13 +7,13 @@
 #include <string.h>
 
 #include <algorithm>
-#include <cstdio>
 #include <cstdlib>
 #include <sstream>
 
 #include "base/logging.h"
 
 #include "chromiumos-wide-profiling/compat/proto.h"
+#include "chromiumos-wide-profiling/file_reader.h"
 #include "chromiumos-wide-profiling/perf_protobuf_io.h"
 #include "chromiumos-wide-profiling/run_command.h"
 #include "chromiumos-wide-profiling/utils.h"
@@ -126,12 +126,10 @@
 #endif  // !QUIPPER_EXTERNAL_TEST_PATHS
 
 int64_t GetFileSize(const string& filename) {
-  FILE* fp = fopen(filename.c_str(), "rb");
-  if (!fp)
+  FileReader reader(filename);
+  if (!reader.IsOpen())
     return -1;
-  int64_t file_size = GetFileSizeFromHandle(fp);
-  fclose(fp);
-  return file_size;
+  return reader.size();
 }
 
 bool CompareFileContents(const string& filename1, const string& filename2) {

chromiumos-wide-profiling/utils.cc

diff --git a/chromiumos-wide-profiling/utils.cc b/chromiumos-wide-profiling/utils.cc
index 6260575..1d0ca99 100644
--- a/chromiumos-wide-profiling/utils.cc
+++ b/chromiumos-wide-profiling/utils.cc
@@ -17,6 +17,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "chromiumos-wide-profiling/compat/proto.h"
+#include "chromiumos-wide-profiling/file_reader.h"
 
 namespace {
 
@@ -27,15 +28,6 @@
 
 namespace quipper {
 
-int64_t GetFileSizeFromHandle(FILE* fp) {
-  int64_t position = ftell(fp);
-  fseek(fp, 0, SEEK_END);
-  int64_t file_size = ftell(fp);
-  // Restore the original file handle position.
-  fseek(fp, position, SEEK_SET);
-  return file_size;
-}
-
 static uint64_t Md5Prefix(
     const unsigned char* data,
     unsigned long length) { // NOLINT
@@ -65,15 +57,18 @@
 }
 
 bool FileToBuffer(const string& filename, std::vector<char>* contents) {
-  FILE* fp = fopen(filename.c_str(), "rb");
-  if (!fp)
+  FileReader reader(filename);
+  if (!reader.IsOpen())
     return false;
-  int64_t file_size = quipper::GetFileSizeFromHandle(fp);
+  size_t file_size = reader.size();
   contents->resize(file_size);
   // Do not read anything if the file exists but is empty.
-  if (file_size > 0)
-    CHECK_GT(fread(contents->data(), file_size, 1, fp), 0U);
-  fclose(fp);
+  if (file_size > 0 &&
+      !reader.ReadData(file_size, contents->data())) {
+    LOG(ERROR) << "Failed to read " << file_size << " bytes from file "
+               << filename << ", only read " << reader.Tell();
+    return false;
+  }
   return true;
 }