Check Swaps and MergeDictionary for base::Value

Check Swaps and MergeDictionary for base::Value

The following is a dangerous practice:

1: DictionaryValue d;
2: Value *p = &d;
3: *p = Value(1.234);
4: DictionaryValue().Swap(&d);

After line 3, |d| is no longer a DictionaryValue.

Before r451296, it caused no bad memory access, because the storage
for the double and for the dictionary did not overlap. After that CL, this code would crash. In any case, it is dangerous.

Similar issue is there for DictionaryValue::MergeDictionary and ListValue::Swap.

This issue is only temporary, because DictionaryValue will eventually become
just Value. But until then, the code needs to be sure to avoid such scenarios.

Therefore this CL introduces CHECKS for the correct value types.

The CL also introduces two more CHECKS in Internal*AssignFrom to prevent
executing a desctructor on an arbitrary address.

All the above are CHECKS, as opposed to DCHECKS, because if the checked
conditions do not hold, execution of arbitrary parts of memory might be
possible.

BUG=697817
Review-Url: https://codereview.chromium.org/2728773005
Cr-Commit-Position: refs/heads/master@{#455288}
Committed: https://chromium.googlesource.com/chromium/src/+/182756aeb9ef4240cace58875e71d42a7402711c

[vabr (Chromium), 2017-03-02 23:02:05]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 23:02:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-03-02 23:25:07]

vabr@chromium.org changed reviewers:
+ brettw@chromium.org

[vabr (Chromium), 2017-03-02 23:25:07]

Hi Brett,

While I'm not sure how much this is related to the crash in
https://crbug.com/697817, the CHECKS proposed here are stopping potential
execution on random addresses.

Could you please review?

Cheers,
Vaclav

[vabr (Chromium), 2017-03-02 23:26:20]

Description was changed from

==========
Check Swaps and MergeDictionary for base::Value

The following is a dangerous practice:

1: DictionaryValue d;
2: Value *p = &d;
3: *p = Value(1.234);
4: DictionaryValue().Swap(&d);

After line 3, |d| is no longer a DictionaryValue.

However, before r451296, it caused no bad memory access, because the storage
for the double and for the dictionary did not overlap.

Similar issue is there for DictionaryValue::MergeDictionary and ListValue::Swap.

This issue is only temporary, because DictionaryValue will eventually become
just Value. But until then, the code needs to be sure to avoid such scenarios.

Therefore this CL introduces CHECKS for the correct value types.

The CL also introduces two more CHECKS in Internal*AssignFrom to prevent
executing a desctructor on an arbitrary address.

All the above are CHECKS, as opposed to DCHECKS, because if the checked
conditions do not hold, execution of arbitrary parts of memory might be
possible.

BUG=697817
==========

to

==========
Check Swaps and MergeDictionary for base::Value

The following is a dangerous practice:

1: DictionaryValue d;
2: Value *p = &d;
3: *p = Value(1.234);
4: DictionaryValue().Swap(&d);

After line 3, |d| is no longer a DictionaryValue.

Before r451296, it caused no bad memory access, because the storage
for the double and for the dictionary did not overlap. After that CL, this code
would crash. In any case, it is dangerous.

Similar issue is there for DictionaryValue::MergeDictionary and ListValue::Swap.

This issue is only temporary, because DictionaryValue will eventually become
just Value. But until then, the code needs to be sure to avoid such scenarios.

Therefore this CL introduces CHECKS for the correct value types.

The CL also introduces two more CHECKS in Internal*AssignFrom to prevent
executing a desctructor on an arbitrary address.

All the above are CHECKS, as opposed to DCHECKS, because if the checked
conditions do not hold, execution of arbitrary parts of memory might be
possible.

BUG=697817
==========

[commit-bot: I haz the power, 2017-03-03 00:09:55]

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

[commit-bot: I haz the power, 2017-03-03 00:09:55]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-03-06 12:34:43]

jdoerrie@chromium.org changed reviewers:
+ jdoerrie@chromium.org

[jdoerrie, 2017-03-06 12:34:43]

LGTM, as you said, this should only be temporary.

https://codereview.chromium.org/2728773005/diff/20001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2728773005/diff/20001/base/values.cc#newcode538
base/values.cc:538: CHECK_EQ(type_, that.type_);
You could add a comment here that this does not prevent copy assigning between
Values of different types (we do a CleanUp followed by a CopyConstructFrom in
that case, see line 180). This confused me the first time I skimmed through this
CL. Another option would be to rename this method to
|InternalCopyAssignFromSameType|, WDYT?

https://codereview.chromium.org/2728773005/diff/20001/base/values.cc#newcode568
base/values.cc:568: CHECK_EQ(type_, that.type_);
See above (s/copy/move).

[vabr (Chromium), 2017-03-06 15:13:02]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 15:13:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 17:05:36]

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

[commit-bot: I haz the power, 2017-03-06 17:05:37]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-03-06 17:09:32]

Thanks, Jan!

Brett -- a gentle ping for a review. :)

Cheers,
Vaclav

https://codereview.chromium.org/2728773005/diff/20001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2728773005/diff/20001/base/values.cc#newcode538
base/values.cc:538: CHECK_EQ(type_, that.type_);
On 2017/03/06 12:34:43, jdoerrie wrote:
> You could add a comment here that this does not prevent copy assigning between
> Values of different types (we do a CleanUp followed by a CopyConstructFrom in
> that case, see line 180). This confused me the first time I skimmed through
this
> CL. Another option would be to rename this method to
> |InternalCopyAssignFromSameType|, WDYT?

Yes, it is confusing. In fact, as I studied the crashes and found this method, I
was enthusiastic about having found the cause, until I realised there is the
CleanUp+construct path as you described.
I think changing the name is a good idea.
Done.

https://codereview.chromium.org/2728773005/diff/20001/base/values.cc#newcode568
base/values.cc:568: CHECK_EQ(type_, that.type_);
On 2017/03/06 12:34:43, jdoerrie wrote:
> See above (s/copy/move).

Done.

[brettw, 2017-03-07 21:40:17]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-03-07 21:40:17]

lgtm

[brettw, 2017-03-07 21:40:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdoerrie@chromium.org
Link to the patchset: https://codereview.chromium.org/2728773005/#ps40001
(title: "Add ...SameType")

[commit-bot: I haz the power, 2017-03-07 21:41:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 23:34:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488922817382460,
"parent_rev": "1c36e4ed00bdc18fa8c3892f40e1a801f32a4d54", "commit_rev":
"182756aeb9ef4240cace58875e71d42a7402711c"}

[commit-bot: I haz the power, 2017-03-07 23:35:13]

Description was changed from

==========
Check Swaps and MergeDictionary for base::Value

The following is a dangerous practice:

1: DictionaryValue d;
2: Value *p = &d;
3: *p = Value(1.234);
4: DictionaryValue().Swap(&d);

After line 3, |d| is no longer a DictionaryValue.

Before r451296, it caused no bad memory access, because the storage
for the double and for the dictionary did not overlap. After that CL, this code
would crash. In any case, it is dangerous.

Similar issue is there for DictionaryValue::MergeDictionary and ListValue::Swap.

This issue is only temporary, because DictionaryValue will eventually become
just Value. But until then, the code needs to be sure to avoid such scenarios.

Therefore this CL introduces CHECKS for the correct value types.

The CL also introduces two more CHECKS in Internal*AssignFrom to prevent
executing a desctructor on an arbitrary address.

All the above are CHECKS, as opposed to DCHECKS, because if the checked
conditions do not hold, execution of arbitrary parts of memory might be
possible.

BUG=697817
==========

to

==========
Check Swaps and MergeDictionary for base::Value

The following is a dangerous practice:

1: DictionaryValue d;
2: Value *p = &d;
3: *p = Value(1.234);
4: DictionaryValue().Swap(&d);

After line 3, |d| is no longer a DictionaryValue.

Before r451296, it caused no bad memory access, because the storage
for the double and for the dictionary did not overlap. After that CL, this code
would crash. In any case, it is dangerous.

Similar issue is there for DictionaryValue::MergeDictionary and ListValue::Swap.

This issue is only temporary, because DictionaryValue will eventually become
just Value. But until then, the code needs to be sure to avoid such scenarios.

Therefore this CL introduces CHECKS for the correct value types.

The CL also introduces two more CHECKS in Internal*AssignFrom to prevent
executing a desctructor on an arbitrary address.

All the above are CHECKS, as opposed to DCHECKS, because if the checked
conditions do not hold, execution of arbitrary parts of memory might be
possible.

BUG=697817

Review-Url: https://codereview.chromium.org/2728773005
Cr-Commit-Position: refs/heads/master@{#455288}
Committed:
https://chromium.googlesource.com/chromium/src/+/182756aeb9ef4240cace58875e71...
==========

[commit-bot: I haz the power, 2017-03-07 23:35:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/182756aeb9ef4240cace58875e71...