Stop base::OpenFile from leaking fds/handles into child procs.

base/files/file_util_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <fcntl.h>

[gab, 2017/02/09 15:18:09]

#if POSIX

[grt (UTC plus 2), 2017/02/09 15:57:01]

Is this conventional? For some reason I'd thought that only platform-specific
headers were generally put in #if blocks.

[gab, 2017/02/09 17:01:24]

Actually, fcntl.h is already included in the OS_POSIX block below :).

FWIW, I like includes only used by a section of the file to only be included for
it (especially system headers and especially in a file like this which already
has OS specific blocks).

[grt (UTC plus 2), 2017/02/10 09:37:21]

Well, then...
SGTM

 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <fstream>
+#include <initializer_list>

[gab, 2017/02/09 15:18:09]

Is this implicitly required by the for-each below or is it a leftover from
something else you tried?

[grt (UTC plus 2), 2017/02/09 15:57:02]

It is explicitly required for the for loop. :-)

 #include <set>
 #include <utility>
 #include <vector>
 
 #include "base/base_paths.h"
 #include "base/environment.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
@@ skipping 219 matching lines @@
 std::wstring ReadTextFile(const FilePath& filename) {
   wchar_t contents[64];
   std::wifstream file;
   file.open(filename.value().c_str());
   EXPECT_TRUE(file.is_open());
   file.getline(contents, arraysize(contents));
   file.close();
   return std::wstring(contents);
 }
 
+// Sets |is_inheritable| to true if |stream| is set up to be inerhited into

[gab, 2017/02/09 15:18:09]

This reads to me as though it "sets it to true iff it's on and doesn't touch it
otherwise".

Maybe:

// Barring a fatal assertion: sets |is_inheritable| to indicate whether |stream|
is set up to ...

[grt (UTC plus 2), 2017/02/10 09:37:21]

Comment updated.

+// child processes (i.e., HANDLE_FLAG_INHERIT is set on the underlying handle on
+// Windows, or FD_CLOEXEC is not set on the underlying file descriptor on
+// POSIX).
+void GetIsInheritable(FILE* stream, bool* is_inheritable) {
+#if defined(OS_WIN)
+  HANDLE handle = reinterpret_cast<HANDLE>(_get_osfhandle(_fileno(stream)));
+  ASSERT_NE(INVALID_HANDLE_VALUE, handle);
+
+  DWORD info = 0;
+  ASSERT_EQ(TRUE, ::GetHandleInformation(handle, &info));
+  *is_inheritable = ((info & HANDLE_FLAG_INHERIT) != 0);
+#elif defined(OS_POSIX)
+  int fd = fileno(stream);
+  ASSERT_NE(-1, fd);
+  int flags = fcntl(fd, F_GETFD, 0);
+  ASSERT_NE(-1, flags);
+  *is_inheritable = ((flags & FD_CLOEXEC) == 0);
+#endif
+}
+
 TEST_F(FileUtilTest, FileAndDirectorySize) {
   // Create three files of 20, 30 and 3 chars (utf8). ComputeDirectorySize
   // should return 53 bytes.
   FilePath file_01 = temp_dir_.GetPath().Append(FPL("The file 01.txt"));
   CreateTextFile(file_01, L"12345678901234567890");
   int64_t size_f1 = 0;
   ASSERT_TRUE(GetFileSize(file_01, &size_f1));
   EXPECT_EQ(20ll, size_f1);
 
   FilePath subdir_path = temp_dir_.GetPath().Append(FPL("Level2"));
@@ skipping 1443 matching lines @@
   }
 
   // Restore the original $TMP.
   if (original_tmp) {
     ::_tputenv_s(kTmpKey, original_tmp);
     free(original_tmp);
   } else {
     ::_tputenv_s(kTmpKey, _T(""));
   }
 }
+
 #endif  // OS_WIN
 
+// Test that files opened by OpenFile are not set up for inheritance into child
+// procs.
+TEST_F(FileUtilTest, OpenFileNoInheritance) {

[gab, 2017/02/09 15:18:09]

Assuming you verified that this test fails with the old impl?

(I tend to like an explicit TEST= field in CL desc mentioning that for
regression tests)

[grt (UTC plus 2), 2017/02/09 15:57:01]

Yes, indeed.
Added (is that what you meant?)

[gab, 2017/02/09 17:01:24]

Not quite, i.e. wanted to explicitly state that it fails under old impl and
passes now, I edited it already :)

+  FilePath file_path(temp_dir_.GetPath().Append(FPL("a_file")));
+
+  for (const char* mode : {"wb", "r,ccs=UNICODE"}) {

[gab, 2017/02/09 15:18:09]

SCOPED_TRACE(mode);

[grt (UTC plus 2), 2017/02/10 09:37:21]

Done.

+    ASSERT_NO_FATAL_FAILURE(CreateTextFile(file_path, L"Geepers"));
+    FILE* file = OpenFile(file_path, mode);
+    ASSERT_NE(nullptr, file);

[gab, 2017/02/09 15:18:09]

ASSERT_TRUE(file);

[grt (UTC plus 2), 2017/02/09 15:57:01]

I don't get this. |file| is a pointer. Why do a bool conversion when what I care
about is that it isn't null?

[gab, 2017/02/09 17:01:24]

I don't care strongly but the way I see it: if this were code logic we'd write

if (file)

not

if (file != nullptr)

right? (I can't find where but I swear there was recommendation to not add ==
null or != null in code logic)

The ASSERT_NE macro on failure can't give a better printed output than
ASSERT_TRUE (i.e. we already know one of the values so |file| would have to be
== nullptr).

[grt (UTC plus 2), 2017/02/10 09:37:21]

I see where you're going, but looking at this failure:

../../base/files/file_util_unittest.cc(1746): error: Value of: file
  Actual: false
Expected: true

it's telling me a bool had the wrong value. I think the gap between seeing the
failure and understanding it is a bit smaller without involving booleans.

+    bool is_inheritable = false;

[gab, 2017/02/09 15:18:09]

Initialize to |true| to ensure GetIsInheritable() truly flipped it?

[grt (UTC plus 2), 2017/02/10 09:37:21]

Done.

+    ASSERT_NO_FATAL_FAILURE(GetIsInheritable(file, &is_inheritable));
+    EXPECT_FALSE(is_inheritable);
+    ASSERT_TRUE(CloseFile(file));
+    ASSERT_TRUE(DeleteFile(file_path, false));
+  }
+}
+
 TEST_F(FileUtilTest, CreateTemporaryFileTest) {
   FilePath temp_files[3];
   for (int i = 0; i < 3; i++) {
     ASSERT_TRUE(CreateTemporaryFile(&(temp_files[i])));
     EXPECT_TRUE(PathExists(temp_files[i]));
     EXPECT_FALSE(DirectoryExists(temp_files[i]));
   }
   for (int i = 0; i < 3; i++)
     EXPECT_FALSE(temp_files[i] == temp_files[(i+1)%3]);
   for (int i = 0; i < 3; i++)
@@ skipping 844 matching lines @@
   // Trying to close it should crash. This is important for security.
   EXPECT_DEATH(CloseWithScopedFD(fds[1]), "");
 #endif
 }
 
 #endif  // defined(OS_POSIX)
 
 }  // namespace
 
 }  // namespace base