Fixing a bug related to using a string piece after the inner data is destroyed.

The string piece holds a reference to the internal buffer_ when this class gets destructed the buffer gets destructed but the stringpiece can still be used outside (it is an input parameter). The fix is to first copy into scratch (which is an input parameter) and then put the scratch in the stringpiece (result).
Change: 139236445

2 files changed, 48 insertions, 41 deletions

diff --git a/tensorflow/core/platform/cloud/gcs_file_system.cc b/tensorflow/core/platform/cloud/gcs_file_system.cc
index a3727b109d..3998324047 100644
--- a/tensorflow/core/platform/cloud/gcs_file_system.cc
+++ b/tensorflow/core/platform/cloud/gcs_file_system.cc
@@ -244,8 +244,8 @@ class GcsRandomAccessFile : public RandomAccessFile {
       TF_RETURN_IF_ERROR(LoadBufferFromGCS());
 
       // Set the results.
-      *result = StringPiece(buffer_.data(), std::min(buffer_.size(), n));
-      std::memcpy(scratch, buffer_.data(), result->size());
+      std::memcpy(scratch, buffer_.data(), std::min(buffer_.size(), n));
+      *result = StringPiece(scratch, std::min(buffer_.size(), n));
     }
 
     if (result->size() < n) {
diff --git a/tensorflow/core/platform/cloud/gcs_file_system_test.cc b/tensorflow/core/platform/cloud/gcs_file_system_test.cc
index 964f6c932b..5f5f868a5c 100644
--- a/tensorflow/core/platform/cloud/gcs_file_system_test.cc
+++ b/tensorflow/core/platform/cloud/gcs_file_system_test.cc
@@ -138,50 +138,57 @@ TEST(GcsFileSystemTest, NewRandomAccessFile_WithReadAhead) {
                        new FakeHttpRequestFactory(&requests)),
                    5 /* read ahead bytes */, 5 /* max upload attempts */);
 
-  std::unique_ptr<RandomAccessFile> file;
-  TF_EXPECT_OK(fs.NewRandomAccessFile("gs://bucket/random_access.txt", &file));
-
   char scratch[100];
   StringPiece result;
-
-  // Read the first chunk. The buffer will be updated with 4 + 5 = 9 bytes.
-  scratch[5] = 'x';
-  TF_EXPECT_OK(file->Read(0, 4, &result, scratch));
-  EXPECT_EQ("0123", result);
-  EXPECT_EQ(scratch[5], 'x');  // Make sure we only copied 4 bytes.
-
-  // The second chunk will be fully loaded from the buffer, no requests are
-  // made.
-  TF_EXPECT_OK(file->Read(4, 4, &result, scratch));
-  EXPECT_EQ("4567", result);
-
-  // The chunk is only partially buffered -- the request will be made to
-  // reload the buffer. 9 bytes will be requested (same as initial buffer size).
-  TF_EXPECT_OK(file->Read(6, 5, &result, scratch));
-  EXPECT_EQ("6789a", result);
-
-  // The range can only be partially satisfied. An attempt to fill the buffer
-  // with 10 + 5 = 15 bytes will be made (buffer is resized for this request).
-  EXPECT_EQ(errors::Code::OUT_OF_RANGE,
-            file->Read(6, 10, &result, scratch).code());
-  EXPECT_EQ("6789abcd", result);
-
-  // The range cannot be satisfied, and the requested offset lies within the
-  // buffer, but the end of the range is outside of the buffer.
-  // A new request will be made to read 10 + 5 = 15 bytes.
-  EXPECT_EQ(errors::Code::OUT_OF_RANGE,
-            file->Read(7, 10, &result, scratch).code());
-  EXPECT_EQ("789abcdef", result);
-
-  // The range cannot be satisfied, and the requested offset is greater than the
-  // buffered range. A new request will be made to read 10 + 5 = 15 bytes.
-  EXPECT_EQ(errors::Code::OUT_OF_RANGE,
-            file->Read(20, 10, &result, scratch).code());
-  EXPECT_TRUE(result.empty());
+  {
+    // We are instantiating this in an enclosed scope to make sure after the
+    // unique ptr goes out of scope, we can still access result.
+    std::unique_ptr<RandomAccessFile> file;
+    TF_EXPECT_OK(
+        fs.NewRandomAccessFile("gs://bucket/random_access.txt", &file));
+
+    // Read the first chunk. The buffer will be updated with 4 + 5 = 9 bytes.
+    scratch[5] = 'x';
+    TF_EXPECT_OK(file->Read(0, 4, &result, scratch));
+    EXPECT_EQ("0123", result);
+    EXPECT_EQ(scratch[5], 'x');  // Make sure we only copied 4 bytes.
+
+    // The second chunk will be fully loaded from the buffer, no requests are
+    // made.
+    TF_EXPECT_OK(file->Read(4, 4, &result, scratch));
+    EXPECT_EQ("4567", result);
+
+    // The chunk is only partially buffered -- the request will be made to
+    // reload the buffer. 9 bytes will be requested (same as initial buffer
+    // size).
+    TF_EXPECT_OK(file->Read(6, 5, &result, scratch));
+    EXPECT_EQ("6789a", result);
+
+    // The range can only be partially satisfied. An attempt to fill the buffer
+    // with 10 + 5 = 15 bytes will be made (buffer is resized for this request).
+    EXPECT_EQ(errors::Code::OUT_OF_RANGE,
+              file->Read(6, 10, &result, scratch).code());
+    EXPECT_EQ("6789abcd", result);
+
+    // The range cannot be satisfied, and the requested offset lies within the
+    // buffer, but the end of the range is outside of the buffer.
+    // A new request will be made to read 10 + 5 = 15 bytes.
+    EXPECT_EQ(errors::Code::OUT_OF_RANGE,
+              file->Read(7, 10, &result, scratch).code());
+    EXPECT_EQ("789abcdef", result);
+
+    // The range cannot be satisfied, and the requested offset is greater than
+    // the
+    // buffered range. A new request will be made to read 10 + 5 = 15 bytes.
+    EXPECT_EQ(errors::Code::OUT_OF_RANGE,
+              file->Read(20, 10, &result, scratch).code());
+    EXPECT_TRUE(result.empty());
+
+    TF_EXPECT_OK(file->Read(0, 4, &result, scratch));
+  }
 
   // The beginning of the file is not in the buffer. This call will result
   // in another request. The buffer size is still 15.
-  TF_EXPECT_OK(file->Read(0, 4, &result, scratch));
   EXPECT_EQ("0123", result);
 }