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

Introduce a SetRegValueWorkItem overload that accepts a callback instead
of a fixed value.

The callback is used to determine the desired value after inspecting the
existing value.

Also modified the existing test's paradigm to bring it up to modern days
(from initial.commit!) and added some more tests to cover this new use case.

Bypassing the wstring usage presubmit warnings because we will likely want
to merge this, will consider fixing the API on trunk after.

BUG=488247, 502363
NOPRESUBMIT=true

Committed: https://crrev.com/69821f190e784c132d598e07433165c934ee3087
Cr-Commit-Position: refs/heads/master@{#337164}

[gab, 2015-06-26 19:12:08]

Patchset #1 (id:1) has been deleted

[gab, 2015-06-26 19:18:25]

Patchset #1 (id:20001) has been deleted

[gab, 2015-06-26 19:19:48]

gab@chromium.org changed reviewers:
+ grt@chromium.org

[gab, 2015-06-26 19:19:48]

Greg PTAL (I stand before you honored to finally get back to some good ol'
installer code reviews :-)).

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

looks good. some small comments.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:14: void
StringToBinaryData(const std::wstring& str_value,
nit: add a comment

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:154: DCHECK(type_ == REG_SZ);
DCHECK_EQ(REG_SZ, type_);

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:158: ?
reinterpret_cast<wchar_t*>(&previous_value_[0])
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.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item_unittest.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item_unittest.cc:289:
base::Bind(&VerifyPreviousValueAndReplace, kDataStr1, kDataStr2)));
could you make the test ASSERT that the callback is run? one way to do so would
be to pass a ptr to a bool to the function that the fn sets to true (i.e.
"was_run") and then ASSERT that it was after the call to Do. you could also
reset the bool and ASSERT that it isn't run during rollback.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item_unittest.cc:291:
EXPECT_TRUE(work_item->Do());
all of these should be ASSERT_TRUE rather than EXPECT_TRUE since there's no
point in evaluating anything else if the work items fails.

[gab, 2015-06-29 20:10:02]

Patchset #2 (id:60001) has been deleted

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

Thanks, PTAL.

Cheers,
Gab

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:14: void
StringToBinaryData(const std::wstring& str_value,
On 2015/06/29 15:37:48, grt wrote:
> nit: add a comment

Done.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:154: DCHECK(type_ == REG_SZ);
On 2015/06/29 15:37:48, grt wrote:
> DCHECK_EQ(REG_SZ, type_);

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
        ]


:-(

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:158: ?
reinterpret_cast<wchar_t*>(&previous_value_[0])
On 2015/06/29 15:37:48, grt wrote:
> 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. :-) 


Indeed, great great point sir!

> for bonus points, remove the last char from the resulting string if it's a
string terminator.

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..?

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item_unittest.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item_unittest.cc:289:
base::Bind(&VerifyPreviousValueAndReplace, kDataStr1, kDataStr2)));
On 2015/06/29 15:37:48, grt wrote:
> could you make the test ASSERT that the callback is run? one way to do so
would
> be to pass a ptr to a bool to the function that the fn sets to true (i.e.
> "was_run") and then ASSERT that it was after the call to Do. you could also
> reset the bool and ASSERT that it isn't run during rollback.

I feel this is tangling into how the implementation works a bit too much. The
API of this call is "I will set what this callback tells me", not "I will call
this callback" (which is of course related, but not what we're testing).

The only place kDataStr2 is mentioned before the EXPECT_EQ on 295 is in this
Bind, so I don't see how it could have arrived there without the callback having
ran and I think this does verify that it did.

As for testing it's not called in Rollback(), we are also testing that the
result of the rollback is not kDataStr2, so I guess the only point of checking
the callback was *not* called is performance which I don't think is very
relevant here (i.e. in the limit, hooking everything and ensuring only the
strict minimum was called by the impl in every unittest is probably overkill).

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item_unittest.cc:291:
EXPECT_TRUE(work_item->Do());
On 2015/06/29 15:37:48, grt wrote:
> all of these should be ASSERT_TRUE rather than EXPECT_TRUE since there's no
> point in evaluating anything else if the work items fails.

Good point, replaced for every test in this file (since I'm already touching
them a bunch).

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:43: if
((*str_value)[str_len_maybe_with_null - 1] == L'\0')
Think this is safe or should I check the resulting string's length?

[grt (UTC plus 2), 2015-06-30 20:26:43]

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:154: DCHECK(type_ == REG_SZ);
On 2015/06/29 20:11:01, gab wrote:
> On 2015/06/29 15:37:48, grt wrote:
> > DCHECK_EQ(REG_SZ, type_);
> 
> 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
>         ]
> 
> 
> :-(

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

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:158: ?
reinterpret_cast<wchar_t*>(&previous_value_[0])
On 2015/06/29 20:11:01, gab wrote:
> On 2015/06/29 15:37:48, grt wrote:
> > 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. :-) 
> 
> 
> Indeed, great great point sir!
> 
> > for bonus points, remove the last char from the resulting string if it's a
> string terminator.
> 
> 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..?

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.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item_unittest.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item_unittest.cc:289:
base::Bind(&VerifyPreviousValueAndReplace, kDataStr1, kDataStr2)));
On 2015/06/29 20:11:01, gab wrote:
> On 2015/06/29 15:37:48, grt wrote:
> > could you make the test ASSERT that the callback is run? one way to do so
> would
> > be to pass a ptr to a bool to the function that the fn sets to true (i.e.
> > "was_run") and then ASSERT that it was after the call to Do. you could also
> > reset the bool and ASSERT that it isn't run during rollback.
> 
> I feel this is tangling into how the implementation works a bit too much. The
> API of this call is "I will set what this callback tells me", not "I will call
> this callback"

My reason for testing that the callback is called: if the test asserted that the
callback was called and this assertion failed, you would know exactly where to
look to diagnose/fix the error. On the other hand, if only the expectation of
the data value fails, then you have more places where you need to poke around to
see why it failed. Since the callback is part of the API contract, I don't think
testing that it was called falls in the category of white-box testing (where the
test is too coupled to the implementation).

> (which is of course related, but not what we're testing).
> 
> The only place kDataStr2 is mentioned before the EXPECT_EQ on 295 is in this
> Bind, so I don't see how it could have arrived there without the callback
having
> ran

Now you're justifying that the test is properly written and that it can't fail.
This is a weaker argument that having a test that will simply fail if the right
thing doesn't happen.

> and I think this does verify that it did.
> 
> As for testing it's not called in Rollback(), we are also testing that the
> result of the rollback is not kDataStr2, so I guess the only point of checking
> the callback was *not* called is performance

I disagree. Rolling back should restore the previous value, which is not a
function of the callback. A bug in the code that invoked the callback at the
wrong time is a bug.

> which I don't think is very
> relevant here (i.e. in the limit, hooking everything and ensuring only the
> strict minimum was called by the impl in every unittest is probably overkill).

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:42: // it (C++ strings are not
NULL terminated).
this comment is a bit misleading: while the terminator is not counted in
std::basic_string<>::size(), std::basic_string<>.c_str() is null-terminated
(StringToBinaryData relies on this). how about just "  // Trim off a trailing
string terminator, if present."

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:43: if
((*str_value)[str_len_maybe_with_null - 1] == L'\0')
On 2015/06/29 20:11:01, gab wrote:
> Think this is safe or should I check the resulting string's length?

I think it's safe. str_len_maybe_with_null >= 1, so at the extreme it erases all
of str_value, right?

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

Thanks, PTAL

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:154: DCHECK(type_ == REG_SZ);
On 2015/06/30 20:26:42, grt wrote:
> On 2015/06/29 20:11:01, gab wrote:
> > On 2015/06/29 15:37:48, grt wrote:
> > > DCHECK_EQ(REG_SZ, type_);
> > 
> > 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
> >         ]
> > 
> > 
> > :-(
> 
> I think the cast is worth it so that failures contain the unexpected value:
>     DCHECK_EQ(static_cast<DWORD>(REG_SZ), type_);

Done.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:158: ?
reinterpret_cast<wchar_t*>(&previous_value_[0])
On 2015/06/30 20:26:42, grt wrote:
> On 2015/06/29 20:11:01, gab wrote:
> > On 2015/06/29 15:37:48, grt wrote:
> > > 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. :-) 
> > 
> > 
> > Indeed, great great point sir!
> > 
> > > for bonus points, remove the last char from the resulting string if it's a
> > string terminator.
> > 
> > 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..?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item_unittest.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item_unittest.cc:289:
base::Bind(&VerifyPreviousValueAndReplace, kDataStr1, kDataStr2)));
On 2015/06/30 20:26:42, grt wrote:
> On 2015/06/29 20:11:01, gab wrote:
> > On 2015/06/29 15:37:48, grt wrote:
> > > could you make the test ASSERT that the callback is run? one way to do so
> > would
> > > be to pass a ptr to a bool to the function that the fn sets to true (i.e.
> > > "was_run") and then ASSERT that it was after the call to Do. you could
also
> > > reset the bool and ASSERT that it isn't run during rollback.
> > 
> > I feel this is tangling into how the implementation works a bit too much.
The
> > API of this call is "I will set what this callback tells me", not "I will
call
> > this callback"
> 
> My reason for testing that the callback is called: if the test asserted that
the
> callback was called and this assertion failed, you would know exactly where to
> look to diagnose/fix the error. On the other hand, if only the expectation of
> the data value fails, then you have more places where you need to poke around
to
> see why it failed. Since the callback is part of the API contract, I don't
think
> testing that it was called falls in the category of white-box testing (where
the
> test is too coupled to the implementation).
> 
> > (which is of course related, but not what we're testing).
> > 
> > The only place kDataStr2 is mentioned before the EXPECT_EQ on 295 is in this
> > Bind, so I don't see how it could have arrived there without the callback
> having
> > ran
> 
> Now you're justifying that the test is properly written and that it can't
fail.
> This is a weaker argument that having a test that will simply fail if the
right
> thing doesn't happen.
> 
> > and I think this does verify that it did.
> > 
> > As for testing it's not called in Rollback(), we are also testing that the
> > result of the rollback is not kDataStr2, so I guess the only point of
checking
> > the callback was *not* called is performance
> 
> I disagree. Rolling back should restore the previous value, which is not a
> function of the callback. A bug in the code that invoked the callback at the
> wrong time is a bug.
> 
> > which I don't think is very
> > relevant here (i.e. in the limit, hooking everything and ensuring only the
> > strict minimum was called by the impl in every unittest is probably
overkill).
> 

Done because I don't care so much, but I feel this clutters up the test for no
added value (other than mild bonus if the test starts failing at which point
whoever is debugging it could add a few logs to see whether the issue is that
the callback isn't being called or the callee is misbehaving).

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:42: // it (C++ strings are not
NULL terminated).
On 2015/06/30 20:26:42, grt wrote:
> this comment is a bit misleading: while the terminator is not counted in
> std::basic_string<>::size(), std::basic_string<>.c_str() is null-terminated
> (StringToBinaryData relies on this). how about just "  // Trim off a trailing
> string terminator, if present."

sgtm, done.

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:43: if
((*str_value)[str_len_maybe_with_null - 1] == L'\0')
On 2015/06/30 20:26:42, grt wrote:
> On 2015/06/29 20:11:01, gab wrote:
> > Think this is safe or should I check the resulting string's length?
> 
> I think it's safe. str_len_maybe_with_null >= 1, so at the extreme it erases
all
> of str_value, right?

Well I was thinking if somehow this ends up constructing a shorter string than
str_len_maybe_with_null (e.g., somehow this is bigger than allowed max string
size?!) than this can result in a buffer overflow?

[grt (UTC plus 2), 2015-07-01 15:37:17]

lgtm

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:43: if
((*str_value)[str_len_maybe_with_null - 1] == L'\0')
On 2015/07/01 02:55:19, gab (OOO) wrote:
> On 2015/06/30 20:26:42, grt wrote:
> > On 2015/06/29 20:11:01, gab wrote:
> > > Think this is safe or should I check the resulting string's length?
> > 
> > I think it's safe. str_len_maybe_with_null >= 1, so at the extreme it erases
> all
> > of str_value, right?
> 
> Well I was thinking if somehow this ends up constructing a shorter string than
> str_len_maybe_with_null (e.g., somehow this is bigger than allowed max string
> size?!) than this can result in a buffer overflow?

Ah, I see. You can do away with str_len_maybe_with_null with something like
this:
  if (!str_value->empty()) {
    auto iter = str_value->end();
    if (*--iter == L'\0')
      str_value->erase(iter);
  }

[gab, 2015-07-02 01:59:06]

Thanks, adopted suggestion :-).

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/80001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:43: if
((*str_value)[str_len_maybe_with_null - 1] == L'\0')
On 2015/07/01 15:37:16, grt wrote:
> On 2015/07/01 02:55:19, gab (OOO) wrote:
> > On 2015/06/30 20:26:42, grt wrote:
> > > On 2015/06/29 20:11:01, gab wrote:
> > > > Think this is safe or should I check the resulting string's length?
> > > 
> > > I think it's safe. str_len_maybe_with_null >= 1, so at the extreme it
erases
> > all
> > > of str_value, right?
> > 
> > Well I was thinking if somehow this ends up constructing a shorter string
than
> > str_len_maybe_with_null (e.g., somehow this is bigger than allowed max
string
> > size?!) than this can result in a buffer overflow?
> 
> Ah, I see. You can do away with str_len_maybe_with_null with something like
> this:
>   if (!str_value->empty()) {
>     auto iter = str_value->end();
>     if (*--iter == L'\0')
>       str_value->erase(iter);
>   }

I like that, done.

[gab, 2015-07-02 01:59:11]

The CQ bit was checked by gab@chromium.org

[gab, 2015-07-02 01:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1220473002/#ps120001
(title: "adopt grt's suggestion")

[commit-bot: I haz the power, 2015-07-02 01:59:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220473002/120001

[commit-bot: I haz the power, 2015-07-02 02:11:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-02 02:11:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gab, 2015-07-02 02:15:10]

On 2015/07/02 02:11:36, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Skipping wstring presubmit.

[gab, 2015-07-02 02:15:18]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-07-02 02:15:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220473002/120001

[commit-bot: I haz the power, 2015-07-02 02:59:50]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-07-02 03:00:56]

Patchset 4 (id:??) landed as
https://crrev.com/69821f190e784c132d598e07433165c934ee3087
Cr-Commit-Position: refs/heads/master@{#337164}

[gab, 2015-07-06 20:56:03]

Post-commit note below.

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:158: ?
reinterpret_cast<wchar_t*>(&previous_value_[0])
On 2015/06/29 15:37:48, grt wrote:
> there is no guarantee that previous_value_ is null terminated

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

> , 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.

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

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
File chrome/installer/util/set_reg_value_work_item.cc (right):

https://codereview.chromium.org/1220473002/diff/40001/chrome/installer/util/s...
chrome/installer/util/set_reg_value_work_item.cc:158: ?
reinterpret_cast<wchar_t*>(&previous_value_[0])
On 2015/07/06 20:56:02, gab wrote:
> On 2015/06/29 15:37:48, grt wrote:
> > there is no guarantee that previous_value_ is null terminated
> 
> Actually registry.cc's ReadValue() [1] does assume it's null terminated.

That's a bug. The one in mini_installer.cc does something close to the right
thing.

> It
> appears [2] that REG_SZ is indeed the type of a null-terminated string

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".

>, shall we
> go back to this simpler version?

No, we should fix base::win::RegKey!