base: Fix parsing and add dalvik-heap-limit

base/sys_info_android.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 "base/sys_info.h"
 
 #include <sys/system_properties.h>
 
 #include "base/logging.h"
 #include "base/string_piece.h"
@@ skipping 26 matching lines @@
       return;
     }
   }
 
   // For some reason, we couldn't parse the version number string.
   *major_version = kDefaultAndroidMajorVersion;
   *minor_version = kDefaultAndroidMinorVersion;
   *bugfix_version = kDefaultAndroidBugfixVersion;
 }
 
-int ParseHeapSize(const base::StringPiece& str) {
+// Parses a size (specified with unit 'k','m' or 'g').
+// Returns a value in bytes.
+int64 ParseSizeBytes(const base::StringPiece& str) {
   const int64 KB = 1024;
   const int64 MB = 1024 * KB;
   const int64 GB = 1024 * MB;
   CHECK_GT(str.size(), 0u);
-  int64 factor = 1;
+  int64 unit_multiplier = 1;
   size_t length = str.size();
   if (str[length - 1] == 'k') {
-    factor = KB;
+    unit_multiplier = KB;
     length--;
   } else if (str[length - 1] == 'm') {
-    factor = MB;
+    unit_multiplier = MB;
     length--;
   } else if (str[length - 1] == 'g') {
-    factor = GB;
+    unit_multiplier = GB;
     length--;
   } else {
     CHECK('0' <= str[length - 1] && str[length - 1] <= '9');

[jar (doing other things), 2013/02/13 21:54:11]

It is bothersome that the un-tagged number is limited to the range up to 10
billion - 1, but none of the other tagged ranges are checked.  This function
should be consistent, and have simple semantics (always returns a number betwene
32M and 1024^3?), which would be an int.

This really should be written IMO to be consistent. The translation on line 69
should be done first (before line 56).  The multiplier should then be applied
(probably in each if clause) *after* validating that the translated number is in
a range to not overflow when scaled up.

[epenner, 2013/02/14 04:18:36]

Sorry, how do you figure it's limited to 10billion - 1 exactly?  This CHECK only
seems to only validate that right-most digit (left of the tag) is an integer? Or
am I missing something?

[jar (doing other things), 2013/02/15 00:18:23]

You are correct.  I misread the code in a fast (mistaken) glance and mistakenly
read that the "string length was less than 9."  No good excuse for my mis-read.

The code only validates that the last character is a digit. 

If you look at the definition of StringToInt64, if the last character is not a
digit, then the string will (at a minimum) have "trailing characters," and that
will cause the conversion function to return false, so line 70 will fail.

Bottom line: no need to check this, but I don't really mind it either way.  My
pref would be to remove the check on line 66, but I'll leave it to you to
decide.

On 2013/02/14 04:18:36, epenner wrote:

   }
   int64 result = 0;
   bool parsed = base::StringToInt64(str.substr(0, length), &result);
   CHECK(parsed);
-  result = result * factor / MB;
-  // dalvik.vm.heapsize property is writable by user,
-  // truncate it to reasonable value to avoid overflows later.
-  result = std::min<int64>(std::max<int64>(32, result), 1024);
-  return static_cast<int>(result);
+  return result * unit_multiplier;

[jar (doing other things), 2013/02/13 21:54:11]

This newer code is still problematic, as it ignores all overflows as well.

[epenner, 2013/02/14 04:18:36]

Thanks, good point! So, I guess I'll need to add overflow protection depending
on the k/m/g tag, since each unit could overflow differently?  I'll get started
on that.

 }
 
 int GetDalvikHeapSizeMB() {
   char heap_size_str[PROP_VALUE_MAX];
   __system_property_get("dalvik.vm.heapsize", heap_size_str);
-  return ParseHeapSize(heap_size_str);
+  // dalvik.vm.heapsize property is writable by a root user.
+  // Clamp it to reasonable range as a sanity check.
+  const int64 MB = 1024 * 1024;
+  int64 result = ParseSizeBytes(heap_size_str);
+  result = std::min<int64>(std::max<int64>(32 * MB, result), 1024 * MB) / MB;

[jar (doing other things), 2013/02/13 21:54:11]

This bounding was partially (almost) done on line 66 in some, but not all cases
:-/.  Given that it is seemingly a requirement of the parsing, it should all be
done up in that translation code as described.

Given that the parsing function never (should) return a value larger  than
1024^3, it should really just return an int, and you wouldn't need to cast over
here on line 82, even before you scaled it down by dividing.

[epenner, 2013/02/14 04:18:36]

Since we will want to re-use the parsing function in the future, I don't think
we should limit it to 1GB. Assuming I can fix that function to not overflow an
int64, is this code acceptable as is? Each system value might have different
expected ranges, which is why I put the clamp in here.

[jar (doing other things), 2013/02/15 00:18:23]

OK.

On 2013/02/14 04:18:36, epenner wrote:

+  return static_cast<int>(result);
+}
+
+int GetDalvikHeapGrowthLimitMB() {
+  char heap_size_str[PROP_VALUE_MAX];
+  __system_property_get("dalvik.vm.heapgrowthlimit", heap_size_str);
+  // dalvik.vm.heapgrowthlimit property is writable by a root user.
+  // Clamp it to reasonable range as a sanity check.
+  const int64 MB = 1024 * 1024;
+  int64 result = ParseSizeBytes(heap_size_str);
+  result = std::min<int64>(std::max<int64>(16 * MB, result), 1024 * MB) / MB;

[jar (doing other things), 2013/02/14 01:48:41]

Why do you clamp this to a different range than the other property (see line 81,
and note that one uses 16 as a min, while the other uses 32)?

[epenner, 2013/02/14 04:18:36]

The heap growth limit is typically half of the heap size, again, assuming a user
hasn't rooted his/her device and changed it.

[jar (doing other things), 2013/02/15 00:18:23]

nit: Please add comments explaining where these bounding values come from,
especially to justify different limits in different cases (re: line 92 and line
81).

On 2013/02/14 04:18:36, epenner wrote:

+  return static_cast<int>(result);
 }
 
 }  // anonymous namespace
 
 namespace base {
 
 std::string SysInfo::OperatingSystemName() {
   return "Android";
 }
 
@@ skipping 25 matching lines @@
   // Parse out the numbers.
   ParseOSVersionNumbers(os_version_str, major_version, minor_version,
                         bugfix_version);
 }
 
 int SysInfo::DalvikHeapSizeMB() {
   static int heap_size = GetDalvikHeapSizeMB();
   return heap_size;
 }
 
+int SysInfo::DalvikHeapGrowthLimitMB() {
+  static int heap_growth_limit = GetDalvikHeapGrowthLimitMB();
+  return heap_growth_limit;
+}
+
+
 }  // namespace base