[Sync] Address use-after-free in Directory::InsertEntry

[Sync] Address use-after-free in Directory::InsertEntry

Directory::InsertEntry takes pointer to EntryKernel and inserts owning pointer
into methandles_mapo with WrapUnique. The object is still owned by unique_ptr in
ModelNeutralMutableEntry ctor. If one if the steps inside InsertEntry fails
ModelNeutralMutableEntry will not release unique_ptr which will cause object to
be freed while metahandles map still has entry pointing to it.

I changed InsertEntry to pass ownint pointer to EntryKernel. Caller is free to
stash non-owning pointer, but has to reset it to nullptr if InsertEntry fails.

I refactored couple of functions in Directory to be more strict with pointers to
EntryKernel. Particularly DeleteEntry should use entry found in metahandles_map,
not the entry passed as an argument to remove entry from different indices. It
shouldn't matter in terms of correctness, but makes it easier to reason about
the logic.

BUG=705704
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2844333003
Cr-Commit-Position: refs/heads/master@{#468102}
Committed: https://chromium.googlesource.com/chromium/src/+/34f7b0fb34457f5442bd7798a754872ee9a4e499

[skym, 2017-04-28 16:01:57]

lgtm

[pavely, 2017-04-28 18:26:55]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-04-28 18:27:36]

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-04-28 19:58:34]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493404015487800, "parent_rev":
"13d934bcc82c0e7ac86afe39fdede0a8d3b46d2a", "commit_rev":
"34f7b0fb34457f5442bd7798a754872ee9a4e499"}

[commit-bot: I haz the power, 2017-04-28 19:58:45]

Description was changed from

==========
[Sync] Address use-after-free in Directory::InsertEntry

Directory::InsertEntry takes pointer to EntryKernel and inserts owning pointer
into methandles_mapo with WrapUnique. The object is still owned by unique_ptr in
ModelNeutralMutableEntry ctor. If one if the steps inside InsertEntry fails
ModelNeutralMutableEntry will not release unique_ptr which will cause object to
be freed while metahandles map still has entry pointing to it.

I changed InsertEntry to pass ownint pointer to EntryKernel. Caller is free to
stash non-owning pointer, but has to reset it to nullptr if InsertEntry fails.

I refactored couple of functions in Directory to be more strict with pointers to
EntryKernel. Particularly DeleteEntry should use entry found in metahandles_map,
not the entry passed as an argument to remove entry from different indices. It
shouldn't matter in terms of correctness, but makes it easier to reason about
the logic.

BUG=705704
R=skym@chromium.org
==========

to

==========
[Sync] Address use-after-free in Directory::InsertEntry

Directory::InsertEntry takes pointer to EntryKernel and inserts owning pointer
into methandles_mapo with WrapUnique. The object is still owned by unique_ptr in
ModelNeutralMutableEntry ctor. If one if the steps inside InsertEntry fails
ModelNeutralMutableEntry will not release unique_ptr which will cause object to
be freed while metahandles map still has entry pointing to it.

I changed InsertEntry to pass ownint pointer to EntryKernel. Caller is free to
stash non-owning pointer, but has to reset it to nullptr if InsertEntry fails.

I refactored couple of functions in Directory to be more strict with pointers to
EntryKernel. Particularly DeleteEntry should use entry found in metahandles_map,
not the entry passed as an argument to remove entry from different indices. It
shouldn't matter in terms of correctness, but makes it easier to reason about
the logic.

BUG=705704
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2844333003
Cr-Commit-Position: refs/heads/master@{#468102}
Committed:
https://chromium.googlesource.com/chromium/src/+/34f7b0fb34457f5442bd7798a754...
==========

[commit-bot: I haz the power, 2017-04-28 19:58:46]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/34f7b0fb34457f5442bd7798a754...