Introduce a SetRegValueWorkItem overload that accepts a callback instead (...)

chrome/installer/util/set_reg_value_work_item.cc

 // Copyright (c) 2011 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 "chrome/installer/util/set_reg_value_work_item.h"
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/logging_installer.h"
 
+namespace {
+
+void StringToBinaryData(const std::wstring& str_value,

[grt (UTC plus 2), 2015/06/29 15:37:48]

nit: add a comment

[gab, 2015/06/29 20:11:01]

Done.

+                        std::vector<uint8>* binary_data) {
+  const uint8* data = reinterpret_cast<const uint8*>(str_value.c_str());
+  binary_data->assign(data, data + (str_value.length() + 1) * sizeof(wchar_t));
+}
+
+}  // namespace
+
 SetRegValueWorkItem::~SetRegValueWorkItem() {
 }
 
 SetRegValueWorkItem::SetRegValueWorkItem(HKEY predefined_root,
                                          const std::wstring& key_path,
                                          REGSAM wow64_access,
                                          const std::wstring& value_name,
                                          const std::wstring& value_data,
                                          bool overwrite)
     : predefined_root_(predefined_root),
       key_path_(key_path),
       value_name_(value_name),
       overwrite_(overwrite),
       wow64_access_(wow64_access),
       status_(SET_VALUE),
       type_(REG_SZ),
       previous_type_(0) {
   DCHECK(wow64_access == 0 ||
          wow64_access == KEY_WOW64_32KEY ||
          wow64_access == KEY_WOW64_64KEY);
-  const uint8* data = reinterpret_cast<const uint8*>(value_data.c_str());
-  value_.assign(data, data + (value_data.length() + 1) * sizeof(wchar_t));
+  StringToBinaryData(value_data, &value_);
 }
 
 SetRegValueWorkItem::SetRegValueWorkItem(HKEY predefined_root,
                                          const std::wstring& key_path,
                                          REGSAM wow64_access,
                                          const std::wstring& value_name,
                                          DWORD value_data,
                                          bool overwrite)
     : predefined_root_(predefined_root),
       key_path_(key_path),
@@ skipping 24 matching lines @@
       status_(SET_VALUE),
       type_(REG_QWORD),
       previous_type_(0) {
   DCHECK(wow64_access == 0 ||
          wow64_access == KEY_WOW64_32KEY ||
          wow64_access == KEY_WOW64_64KEY);
   const uint8* data = reinterpret_cast<const uint8*>(&value_data);
   value_.assign(data, data + sizeof(value_data));
 }
 
+SetRegValueWorkItem::SetRegValueWorkItem(
+    HKEY predefined_root,
+    const std::wstring& key_path,
+    REGSAM wow64_access,
+    const std::wstring& value_name,
+    const GetValueFromExistingCallback& get_value_callback)
+    : predefined_root_(predefined_root),
+      key_path_(key_path),
+      value_name_(value_name),
+      get_value_callback_(get_value_callback),
+      overwrite_(true),
+      wow64_access_(wow64_access),
+      status_(SET_VALUE),
+      type_(REG_SZ),
+      previous_type_(0) {
+  DCHECK(wow64_access == 0 ||
+         wow64_access == KEY_WOW64_32KEY ||
+         wow64_access == KEY_WOW64_64KEY);
+  // Nothing to do, |get_value_callback| will fill |value_| later.
+}
+
 bool SetRegValueWorkItem::Do() {
   LONG result = ERROR_SUCCESS;
   base::win::RegKey key;
   if (status_ != SET_VALUE) {
     // we already did something.
     VLOG(1) << "multiple calls to Do()";
     result = ERROR_CANTWRITE;
     return ignore_failure_;
   }
 
@@ skipping 23 matching lines @@
       previous_value_.resize(size);
       result = key.ReadValue(value_name_.c_str(), &previous_value_[0], &size,
                              &previous_type_);
       if (result != ERROR_SUCCESS) {
         previous_value_.clear();
         VLOG(1) << "Failed to save original value. Error: " << result;
       }
     }
   }
 
+  if (!get_value_callback_.is_null()) {
+    // Although this could be made more generic, for now this assumes the
+    // |type_| of |value_| is REG_SZ.
+    DCHECK(type_ == REG_SZ);

[grt (UTC plus 2), 2015/06/29 15:37:48]

DCHECK_EQ(REG_SZ, type_);

[gab, 2015/06/29 20:11:01]

I tried that put it would require a cast or:

d:\src\chrome\src\base\logging.h(540) : error C4389: '==' : signed/unsigned
mismatch
        d:\src\chrome\src\chrome\installer\util\set_reg_value_work_item.cc(156)
: see reference to function template instantiation 'std::string
*logging::CheckEQImpl<int,DWORD>(const t1 &,const t2 &,const char *)' being
compiled
        with
        [
            t1=int
,            t2=DWORD
        ]


:-(

[grt (UTC plus 2), 2015/06/30 20:26:42]

I think the cast is worth it so that failures contain the unexpected value:
    DCHECK_EQ(static_cast<DWORD>(REG_SZ), type_);

[gab, 2015/07/01 02:55:19]

Done.

+
+    const std::wstring previous_value_str(
+        (previous_type_ == REG_SZ && previous_value_.size() >= sizeof(wchar_t))
+            ? reinterpret_cast<wchar_t*>(&previous_value_[0])

[grt (UTC plus 2), 2015/06/29 15:37:48]

there is no guarantee that previous_value_ is null terminated, so you'll need to
do something like:
  std::wstring prevoius_value_str;
  if (previous_type_ == REG_SZ && previous_value_.size() >= sizeof(wchar_t))
    previous_type_.assign(reinterpret_cast<wchar_t*>(&previous_value_[0]),
previous_value_.size() / sizeof(wchar_t));
or something along those lines that would actually compile. :-) for bonus
points, remove the last char from the resulting string if it's a string
terminator.

[gab, 2015/06/29 20:11:01]

Indeed, great great point sir!
Good point, in fact one of my new tests fails if not doing this :-):

[ RUN      ] SetRegValueWorkItemTest.ModifyExistingWithCallback
d:\src\chrome\src\chrome\installer\util\set_reg_value_work_item_unittest.cc(274):
error: Value of: actual_previous_value
  Actual: L"data_111\0"
Expected: expected_previous_value
Which is: L"data_111"
[  FAILED  ] SetRegValueWorkItemTest.ModifyExistingWithCallback (0 ms)�

I implemented a fix and it should work fine most of the time since accross
Chromium we save C-strings (here and in registry.cc's WriteValue), but wouldn't
it be a problem if this is reading an OS value which is meant to contain null
characters as separators and is doubly-null-terminated at the end (we would eat
the last null).

Not sure how to go about this, maybe the current impl is fine since we should be
reading back things we wrote 99.9% of the time..?

[grt (UTC plus 2), 2015/06/30 20:26:42]

If the callback is meant to take the true registry value (as opposed to some
approximation assuming that the value was a char16 string), then the callback
should take a std::vector<uint8_t> containing previous_value_ and the DWORD type
as inputs. Then it's up to the implementer of the callback to decide what to do
with that data. It's more work for the callback implementer.

In this case, the callback is only used when writing REG_SZ values. We
unconditionally interpret REG_SZ values as optionally-null-terminated char16
strings, so I think it's just fine to handle the previous value in the same way
as we handle REG_SZ everywhere else. So this is fine.

[gab, 2015/07/01 02:55:19]

Acknowledged.

[gab, 2015/07/06 20:56:02]

Actually registry.cc's ReadValue() [1] does assume it's null terminated. It
appears [2] that REG_SZ is indeed the type of a null-terminated string, shall we
go back to this simpler version?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/registry....

[2]
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724884(v=vs.85).aspx

[grt (UTC plus 2), 2015/07/10 19:10:52]

That's a bug. The one in mini_installer.cc does something close to the right
thing.
It is the "type" of a null-terminated string, but there's nothing in Windows
that makes sure it is null-terminated. You're welcome to write a ten-byte REG_SZ
containing UTF16 "hello" (no terminator). Windows is perfectly happy with this,
but you will fall over when you read it if you expect to read twelve bytes, the
last two of which are "\0\0".
No, we should fix base::win::RegKey!

+            : L"");
+    StringToBinaryData(get_value_callback_.Run(previous_value_str), &value_);
+  }
+
   result = key.WriteValue(value_name_.c_str(), &value_[0],
                           static_cast<DWORD>(value_.size()), type_);
   if (result != ERROR_SUCCESS) {
     VLOG(1) << "Failed to write value " << key_path_ << " error: " << result;
     return ignore_failure_;
   }
 
   status_ = previous_type_ ? VALUE_OVERWRITTEN : NEW_VALUE_CREATED;
   return true;
 }
@@ skipping 28 matching lines @@
     result = key.WriteValue(value_name_.c_str(), previous_value,
                             static_cast<DWORD>(previous_value_.size()),
                             previous_type_);
     VLOG(1) << "rollback: restoring " << value_name_ << " error: " << result;
   } else {
     NOTREACHED();
   }
 
   status_ = VALUE_ROLL_BACK;
 }