Fix out of bounds write in ReadRemoteData

Fix out of bounds write in ReadRemoteData

https://codereview.chromium.org/2685213002 made us pass data instead
of &data to ReadProcessMemory() but it didn't update the sizeof()
arg.

ReadRemoteData() is called with LONG and DWORDs which have size 4,
but sizeof(data) on 64-bit is the size of a pointer, i.e. 8 -- so
ReadProcessMemory() now reads 8 bytes into a 4-byte buffer.
Fix this.

BUG=692561
Review-Url: https://codereview.chromium.org/2691373003
Cr-Commit-Position: refs/heads/master@{#450751}
Committed: https://chromium.googlesource.com/chromium/src/+/2c8e9d56d99a51c887ec4d15cbde5309533a96fb

[Nico, 2017-02-15 16:06:33]

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

[Nico, 2017-02-15 16:06:55]

thakis@chromium.org changed reviewers:
+ chrisha@chromium.org, pmonette@chromium.org

[Nico, 2017-02-15 16:06:56]

first to stamp wins!

[commit-bot: I haz the power, 2017-02-15 16:07:03]

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

[Patrick Monette, 2017-02-15 16:12:14]

Thanks for fixing it. LGTM

[Nico, 2017-02-15 16:14:44]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2017-02-15 16:14:49]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-02-15 16:15:17]

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-02-15 17:42:26]

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

[commit-bot: I haz the power, 2017-02-15 17:42:27]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nico, 2017-02-15 17:43:45]

That's a failure of this test on the 64 rel bot:

ModuleEventSinkImplTest.CallsForwardedAsExpected (run #1):
[ RUN      ] ModuleEventSinkImplTest.CallsForwardedAsExpected
e: \c \win\src\chrome
rowser\conflicts\module_event_sink_impl_win_unittest.cc(84): error: Value
of: modules().size()
  Actual: 0
Expected: 1u
Which is: 1
[  FAILED  ] ModuleEventSinkImplTest.CallsForwardedAsExpected (0 ms)

I'm pretty sure this patch here is correct -- how confident are we that the
test is implemented correctly?

On Wed, Feb 15, 2017 at 12:42 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Try jobs failed on following builders:
> win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> http://build.chromium.org/p/tryserver.chromium.win/
> builders/win_chromium_x64_rel_ng/builds/367003)
>
> https://codereview.chromium.org/2691373003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2017-02-15 17:45:41]

https://codereview.chromium.org/2691373003/diff/1/chrome/browser/conflicts/mo...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2691373003/diff/1/chrome/browser/conflicts/mo...
chrome/browser/conflicts/module_event_sink_impl_win.cc:71: if (bytes_read !=
sizeof(data))
ah, i need to change this too

[Nico, 2017-02-15 17:46:38]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-02-15 17:46:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
pmonette@chromium.org
Link to the patchset: https://codereview.chromium.org/2691373003/#ps20001
(title: "better")

[commit-bot: I haz the power, 2017-02-15 17:47: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-02-15 18:59:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487180798168940,
"parent_rev": "73b9848f38e00bf1b9f48cfa2da7f8cda7c41599", "commit_rev":
"2c8e9d56d99a51c887ec4d15cbde5309533a96fb"}

[commit-bot: I haz the power, 2017-02-15 18:59:59]

Description was changed from

==========
Fix out of bounds write in ReadRemoteData

https://codereview.chromium.org/2685213002 made us pass data instead
of &data to ReadProcessMemory() but it didn't update the sizeof()
arg.

ReadRemoteData() is called with LONG and DWORDs which have size 4,
but sizeof(data) on 64-bit is the size of a pointer, i.e. 8 -- so
ReadProcessMemory() now reads 8 bytes into a 4-byte buffer.
Fix this.

BUG=692561
==========

to

==========
Fix out of bounds write in ReadRemoteData

https://codereview.chromium.org/2685213002 made us pass data instead
of &data to ReadProcessMemory() but it didn't update the sizeof()
arg.

ReadRemoteData() is called with LONG and DWORDs which have size 4,
but sizeof(data) on 64-bit is the size of a pointer, i.e. 8 -- so
ReadProcessMemory() now reads 8 bytes into a 4-byte buffer.
Fix this.

BUG=692561

Review-Url: https://codereview.chromium.org/2691373003
Cr-Commit-Position: refs/heads/master@{#450751}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c8e9d56d99a51c887ec4d15cbde...
==========

[commit-bot: I haz the power, 2017-02-15 19:00:00]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2c8e9d56d99a51c887ec4d15cbde...

[chrisha, 2017-02-15 21:23:43]

https://codereview.chromium.org/2691373003/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2691373003/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_event_sink_impl_win.cc:67: if
(!::ReadProcessMemory(process, typed_address, data, sizeof(*data),
Pointers are hard /facepalm

Thanks for catching this. Surprised this didn't catch on a sanitizer bot?

[Nico, 2017-02-15 21:47:58]

https://codereview.chromium.org/2691373003/diff/20001/chrome/browser/conflict...
File chrome/browser/conflicts/module_event_sink_impl_win.cc (right):

https://codereview.chromium.org/2691373003/diff/20001/chrome/browser/conflict...
chrome/browser/conflicts/module_event_sink_impl_win.cc:67: if
(!::ReadProcessMemory(process, typed_address, data, sizeof(*data),
On 2017/02/15 21:23:42, chrisha wrote:
> Pointers are hard /facepalm
> 
> Thanks for catching this. Surprised this didn't catch on a sanitizer bot?

Do we have 64-bit sanitizer bots? The clang/asan runtime at least isn't quite
ready on 64-bit yet.

Also, at least clang/asan works by emitting instrumented code and
ReadProcessMemory is a system function and hence not instrumented.

Having said that, I found this 'cause it did break our clang/win 64-bit dbg bot,
so it was caught by some bot. A 64-bit debug msvc bot would've probably caught
this as well, but as far as I know we don't have one.

[chrisha, 2017-02-16 15:20:52]

> Do we have 64-bit sanitizer bots? The clang/asan runtime at least isn't quite
> ready on 64-bit yet.

Yeah, forgot about that. According to etienneb@ Win64 ASAN needs 1 last CL in
LLVM, then some DEPS rolls and we should be good to go. All tests instrumenting
and running on his dev box!

[Nico, 2017-02-16 16:21:31]

Nice :-)

On Feb 16, 2017 10:20 AM, <chrisha@chromium.org> wrote:

> > Do we have 64-bit sanitizer bots? The clang/asan runtime at least isn't
> quite
> > ready on 64-bit yet.
>
> Yeah, forgot about that. According to etienneb@ Win64 ASAN needs 1 last
> CL in
> LLVM, then some DEPS rolls and we should be good to go. All tests
> instrumenting
> and running on his dev box!
>
> https://codereview.chromium.org/2691373003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.