[tracing] Smaps provider uses fscanf instead of istream

base/trace_event/process_memory_maps_dump_provider.cc

 // Copyright 2015 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 "base/trace_event/process_memory_maps_dump_provider.h"
 
-#include <cctype>
-#include <fstream>
+#include <fcntl.h>
+#include <inttypes.h>

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

inttypes is not well supported on windows.
use base/format_macros.h

[ssid, 2015/09/21 11:25:43]

Done.

 
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/process/process_metrics.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/process_memory_maps.h"
 
 namespace base {
 namespace trace_event {
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 // static
-std::istream* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
+ScopedFD* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
 
 namespace {
 
 const uint32 kMaxLineSize = 4096;
+const uint32 kMaxCounterNameSize = 20;
 
-bool ParseSmapsHeader(std::istream* smaps,
-                      ProcessMemoryMaps::VMRegion* region) {
+bool ParseSmapsHeader(FILE* smaps_file, ProcessMemoryMaps::VMRegion* region) {
   // e.g., "00400000-00421000 r-xp 00000000 fc:01 1234  /foo.so\n"
   bool res = true;  // Whether this region should be appended or skipped.
-  uint64 end_addr;
-  std::string protection_flags;
-  std::string ignored;
-  *smaps >> std::hex >> region->start_address;
-  smaps->ignore(1);
-  *smaps >> std::hex >> end_addr;
+  uint64 end_addr = 0;
+  if (fscanf(smaps_file, "%" SCNx64 "-%" SCNx64, &region->start_address,
+             &end_addr) != 2)
+    return false;
+
   if (end_addr > region->start_address) {
     region->size_in_bytes = end_addr - region->start_address;
   } else {
     // This is not just paranoia, it can actually happen (See crbug.com/461237).
     region->size_in_bytes = 0;
     res = false;
   }
 
+  char protection_flags[4];

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

char protection_flags[5] = {0}; so you get also a NUL terminator, which is
definitely safer

[ssid, 2015/09/21 11:25:43]

Done.

+  CHECK_EQ(1, fscanf(smaps_file, "%4s", protection_flags));

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

use %4c, is safer

[ssid, 2015/09/21 11:25:43]

Actually %4c does not add a '\0' at the end as far as i tested. But still it is
not required for our case. So made it 4c

+
   region->protection_flags = 0;
-  *smaps >> protection_flags;
-  CHECK_EQ(4UL, protection_flags.size());
   if (protection_flags[0] == 'r') {
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsRead;
   }
   if (protection_flags[1] == 'w') {
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsWrite;
   }
   if (protection_flags[2] == 'x') {
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsExec;
   }
-  *smaps >> ignored;  // Ignore mapped file offset.
-  *smaps >> ignored;  // Ignore device maj-min (fc:01 in the example above).
-  *smaps >> ignored;  // Ignore inode number (1234 in the example above).
 
-  while (smaps->peek() == ' ')
-    smaps->ignore(1);
+  // Ignore mapped file offset, device maj-min (fc:01 in the example above) and
+  // inode number (1234 in the example above).
+  const int kMaxAddressLength = 20;
+  char ignore[kMaxAddressLength];
+  if (fscanf(smaps_file, "%s %s %s", ignore, ignore, ignore) != 3)

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

you don't need the ignore vars, nor kMaxAddressLength.
Just use %*s to skip the assignment, i.e.:

fscanf(smaps_file, "%*s %*s %*s").

If you fail here the next line parsing will be borked.

[ssid, 2015/09/21 11:25:43]

Moved everything to single line fscanf.

+    return false;
+
   char mapped_file[kMaxLineSize];
-  smaps->getline(mapped_file, sizeof(mapped_file));
-  region->mapped_file = mapped_file;
+  char next;

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

you don't need all this as scanf handles multiple spaces fine.
Actually I think you can solve everything with one scanf in one go, without any
fgetc/fputc

[ssid, 2015/09/21 11:25:43]

Hm, it was tricky case if file name does not exist. From discussion, i am using
strip white spaces on both sides of the file.

+  do {
+    next = fgetc(smaps_file);
+  } while (next == ' ');
+  ungetc(next, smaps_file);
+  if (fgets(mapped_file, sizeof(mapped_file), smaps_file)) {
+    mapped_file[strlen(mapped_file) - 1] = '\0';
+    region->mapped_file = mapped_file;
+  }
 
   return res;
 }
 
-uint64 ReadCounterBytes(std::istream* smaps) {
+uint64 ReadCounterBytes(FILE* smaps_file) {
   uint64 counter_value = 0;
-  *smaps >> std::dec >> counter_value;
+  CHECK_EQ(1, fscanf(smaps_file, "%" SCNu64, &counter_value));
   return counter_value * 1024;
 }
 
-uint32 ParseSmapsCounter(std::istream* smaps,
+uint32 ParseSmapsCounter(FILE* smaps_file,
                          ProcessMemoryMaps::VMRegion* region) {
   // A smaps counter lines looks as follows: "RSS:  0 Kb\n"
   uint32 res = 1;
-  std::string counter_name;
-  *smaps >> counter_name;
+  char counter_name[kMaxCounterNameSize];
+  if (fscanf(smaps_file, "%s", counter_name) != 1)
+    return 0;
 
   // TODO(primiano): "Swap" should also be accounted as resident. Check
   // whether Rss isn't already counting swapped and fix below if that is
   // the case.
-  if (counter_name == "Pss:") {
-    region->byte_stats_proportional_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Private_Dirty:") {
-    region->byte_stats_private_dirty_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Private_Clean:") {
-    region->byte_stats_private_clean_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Shared_Dirty:") {
-    region->byte_stats_shared_dirty_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Shared_Clean:") {
-    region->byte_stats_shared_clean_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Swap:") {
-    region->byte_stats_swapped = ReadCounterBytes(smaps);
+  if (strcmp(counter_name, "Pss:") == 0) {
+    region->byte_stats_proportional_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Private_Dirty:") == 0) {
+    region->byte_stats_private_dirty_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Private_Clean:") == 0) {
+    region->byte_stats_private_clean_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Shared_Dirty:") == 0) {
+    region->byte_stats_shared_dirty_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Shared_Clean:") == 0) {
+    region->byte_stats_shared_clean_resident = ReadCounterBytes(smaps_file);
+  } else if (strcmp(counter_name, "Swap:") == 0) {
+    region->byte_stats_swapped = ReadCounterBytes(smaps_file);
   } else {
     res = 0;
   }
 
 #ifndef NDEBUG
   // Paranoid check against changes of the Kernel /proc interface.
   if (res) {
-    std::string unit;
-    *smaps >> unit;
+    std::string unit(2, 0);

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

I think you can just do a single scanf like
scanf("Counter name: %s kB"), and simplify all this code.

[ssid, 2015/09/21 11:25:43]

Done.

+    fscanf(smaps_file, "%s", &unit[0]);
     DCHECK_EQ("kB", unit);
   }
 #endif
 
-  smaps->ignore(kMaxLineSize, '\n');
+  // Ignore the rest of the line.
+  char ignore[kMaxLineSize];
+  fgets(ignore, sizeof(ignore), smaps_file);
 
   return res;
 }
 
-uint32 ReadLinuxProcSmapsFile(std::istream* smaps, ProcessMemoryMaps* pmm) {
-  if (!smaps->good())
+uint32 ReadLinuxProcSmapsFile(ScopedFD& smaps_fd, ProcessMemoryMaps* pmm) {
+  if (!smaps_fd.is_valid())
     return 0;
 
+  FILE* smaps_file = fdopen(smaps_fd.get(), "r");

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

Hmm I think you should cache also the FILE* struct, together with the scopedfd.
Rationale: you can't close the FILE without closing the underlying file, but
then you lose the fd, and you don't want that.
If you don't close you'll leak FILE* structs, which is similarly bad.

[ssid, 2015/09/21 11:25:43]

Yes, Made necessary changes.

+  fseek(smaps_file, 0, SEEK_SET);
+
   const uint32 kNumExpectedCountersPerRegion = 6;
   uint32 counters_parsed_for_current_region = 0;
   uint32 num_valid_regions = 0;
   ProcessMemoryMaps::VMRegion region;
   bool should_add_current_region = false;
   for (;;) {
-    int next = smaps->peek();
-    if (next == std::ifstream::traits_type::eof() || next == '\n')
+    char next = 0;
+    // Peek into the next integer and move pointer back.
+    next = fgetc(smaps_file);

[Primiano Tucci (use gerrit), 2015/09/10 16:52:36]

not sure what all this is for? aren't you trying to do a feof(f) here?

[ssid, 2015/09/21 11:25:43]

hm, the next is also used to check if it is a header or segment. Though I could
shorten the seek.

+    if (next == EOF)
       break;
+    fseek(smaps_file, -1, SEEK_CUR);
     if (isxdigit(next) && !isupper(next)) {
       region = ProcessMemoryMaps::VMRegion();
       counters_parsed_for_current_region = 0;
-      should_add_current_region = ParseSmapsHeader(smaps, &region);
+      should_add_current_region = ParseSmapsHeader(smaps_file, &region);
     } else {
-      counters_parsed_for_current_region += ParseSmapsCounter(smaps, &region);
+      counters_parsed_for_current_region +=
+          ParseSmapsCounter(smaps_file, &region);
       DCHECK_LE(counters_parsed_for_current_region,
                 kNumExpectedCountersPerRegion);
       if (counters_parsed_for_current_region == kNumExpectedCountersPerRegion) {
         if (should_add_current_region) {
           pmm->AddVMRegion(region);
           ++num_valid_regions;
           should_add_current_region = false;
         }
       }
     }
@@ skipping 21 matching lines @@
 bool ProcessMemoryMapsDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
                                                  ProcessMemoryDump* pmd) {
   // Snapshot of memory maps is not taken for light dump requests.
   if (args.level_of_detail == MemoryDumpArgs::LevelOfDetail::LOW)
     return true;
 
   uint32 res = 0;
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   if (UNLIKELY(proc_smaps_for_testing)) {
-    res = ReadLinuxProcSmapsFile(proc_smaps_for_testing, pmd->process_mmaps());
+    res = ReadLinuxProcSmapsFile(*proc_smaps_for_testing, pmd->process_mmaps());
   } else {
-    std::ifstream proc_self_smaps("/proc/self/smaps");
-    res = ReadLinuxProcSmapsFile(&proc_self_smaps, pmd->process_mmaps());
+    ScopedFD smaps_fd(HANDLE_EINTR(open("/proc/self/smaps", O_RDONLY)));
+    res = ReadLinuxProcSmapsFile(smaps_fd, pmd->process_mmaps());
   }
 #else
   LOG(ERROR) << "ProcessMemoryMaps dump provider is supported only on Linux";
 #endif
 
   if (res > 0) {
     pmd->set_has_process_mmaps();
     return true;
   }
 
   return false;
 }
 
 }  // namespace trace_event
 }  // namespace base