Mojo: Fix possibly-invalid vector subscripting in RawChannelPosix.

Mojo: Fix possibly-invalid vector subscripting in RawChannelPosix.

In RawChannelPosix::OnFileCanReadWithoutBlocking()'s call to memmove(),
read_buffer_start may point one past the end of the buffer. This isn't a
"real" problem, since in that case read_buffer_num_valid_bytes_ will be
zero, but it's illegal to subscript a vector with an invalid index and
an assertion fails in Debug builds (an alternate fix would be to replace
&read_buffer_[read_buffer_start] with &read_buffer_[0] +
read_buffer_start).

The bug was exhibited by the flakily-failing
MultiprocessMessagePipeTest.QueueMessages (in Debug builds), so to test
run:

out/Debug/mojo_system_unittests \
    --gtest_filter=MultiprocessMessagePipeTest.QueueMessages \
    --gtest_repeat=-1 --single-process-tests

R=darin@chromium.org
BUG=329622
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244194

[darin (slow to review), 2014-01-09 21:34:55]

LGTM
On Jan 9, 2014 10:46 AM, <viettrungluu@chromium.org> wrote:

> Reviewers: darin,
>
> Description:
> Mojo: Fix possibly-invalid vector subscripting in RawChannelPosix.
>
> In RawChannelPosix::OnFileCanReadWithoutBlocking()'s call to memmove(),
> read_buffer_start may point one past the end of the buffer. This isn't a
> "real" problem, since in that case read_buffer_num_valid_bytes_ will be
> zero, but it's illegal to subscript a vector with an invalid index and
> an assertion fails in Debug builds (an alternate fix would be to replace
> &read_buffer_[read_buffer_start] with &read_buffer_[0] +
> read_buffer_start).
>
> The bug was exhibited by the flakily-failing
> MultiprocessMessagePipeTest.QueueMessages (in Debug builds), so to test
> run:
>
> out/Debug/mojo_system_unittests \
>     --gtest_filter=MultiprocessMessagePipeTest.QueueMessages \
>     --gtest_repeat=-1 --single-process-tests
>
> R=darin@chromium.org
> BUG=329622
>
> Please review this at https://codereview.chromium.org/132173003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+4, -2 lines):
>   M mojo/system/raw_channel_posix.cc
>
>
> Index: mojo/system/raw_channel_posix.cc
> diff --git a/mojo/system/raw_channel_posix.cc b/mojo/system/raw_channel_
> posix.cc
> index 0038afbd2ba8ab08e9363a0e9b28d0557e31abc3..
> fa9d5f29239dddb1bc99bc9ba4522916fefb35d7 100644
> --- a/mojo/system/raw_channel_posix.cc
> +++ b/mojo/system/raw_channel_posix.cc
> @@ -296,8 +296,10 @@ void RawChannelPosix::OnFileCanReadWithoutBlocking(int
> fd) {
>
>    // Move data back to start.
>    if (read_buffer_start > 0) {
> -    memmove(&read_buffer_[0], &read_buffer_[read_buffer_start],
> -            read_buffer_num_valid_bytes_);
> +    if (read_buffer_num_valid_bytes_ > 0) {
> +      memmove(&read_buffer_[0], &read_buffer_[read_buffer_start],
> +              read_buffer_num_valid_bytes_);
> +    }
>      read_buffer_start = 0;
>    }
>  }
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2014-01-10 17:49:59]

Committed patchset #1 manually as r244194 (presubmit successful).