[mojo-edk] Fix uninitialized reads in EDK dispatchers

[mojo-edk] Fix uninitialized reads in EDK dispatchers

BUG=608434
R=amistry@chromium.org

Committed: https://crrev.com/ff3939d43c6977a4f34bd6e9a827b6823d731be2
Cr-Commit-Position: refs/heads/master@{#391447}

[Anand Mistry (off Chromium), 2016-05-03 05:51:54]

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
(port_status.has_messages && !in_transit_) {
I think this logic is subtle and unclear. Why not just zero-initialise
port_status?

Also, why is this logic different from the producer dispatcher, given the two
functions are largely the same?

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/message_pip...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/message_pip...
mojo/edk/system/message_pipe_dispatcher.cc:502: if
(node_controller_->node()->GetStatus(port_, &port_status) != ports::OK)
Now debug and non-debug builds have different semantics (because of the awake
below the #if), which is bad for testing. Either zero-initalise port_status, or
encapsulate the entire block below in a condition.

[Ken Rockot(use gerrit already), 2016-05-03 06:16:15]

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
(port_status.has_messages && !in_transit_) {
On 2016/05/03 at 05:51:54, Anand Mistry wrote:
> I think this logic is subtle and unclear. Why not just zero-initialise
port_status?
> 
> Also, why is this logic different from the producer dispatcher, given the two
functions are largely the same?

Sorry, are you asking why they aren't using shared implementation? If so, it's
because they didn't start out looking so similar, and I never took the time to
unify them.

Zero-initializing PortStatus is not the correct thing to do. If GetPortStatus
fails, the PortStatus value should not be used. For example, has_messages is
neither true nor false: it's unspecified.

I don't disagree that the logic is subtle and potentially unclear, but I don't
think I'm making it worse here. Fixing that is beyond the scope of this CL.

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/message_pip...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/message_pip...
mojo/edk/system/message_pipe_dispatcher.cc:502: if
(node_controller_->node()->GetStatus(port_, &port_status) != ports::OK)
On 2016/05/03 at 05:51:54, Anand Mistry wrote:
> Now debug and non-debug builds have different semantics (because of the awake
below the #if), which is bad for testing. Either zero-initalise port_status, or
encapsulate the entire block below in a condition.

Good catch, fixed

[Anand Mistry (off Chromium), 2016-05-03 07:19:10]

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
(port_status.has_messages && !in_transit_) {
On 2016/05/03 06:16:14, Ken Rockot wrote:
> On 2016/05/03 at 05:51:54, Anand Mistry wrote:
> > I think this logic is subtle and unclear. Why not just zero-initialise
> port_status?
> > 
> > Also, why is this logic different from the producer dispatcher, given the
two
> functions are largely the same?
> 
> Sorry, are you asking why they aren't using shared implementation? If so, it's
> because they didn't start out looking so similar, and I never took the time to
> unify them.

I mean that these two functions are mostly the same. But you're changing this to
an "else if" and the producer's condition to just an "else", even though they're
currently both the same "if" condition.

> 
> Zero-initializing PortStatus is not the correct thing to do. If GetPortStatus
> fails, the PortStatus value should not be used. For example, has_messages is
> neither true nor false: it's unspecified.

Good point.

> 
> I don't disagree that the logic is subtle and potentially unclear, but I don't
> think I'm making it worse here. Fixing that is beyond the scope of this CL.

I think using the result of the "if" condition in the "else if" branch is
subtle. I would strongly prefer if the condition was more explicit. It's
currently two separate conditionals, which is quite clear when using the result
of the first in the second.

[Ken Rockot(use gerrit already), 2016-05-03 15:19:07]

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/1/mojo/edk/system/data_pipe_c...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
(port_status.has_messages && !in_transit_) {
On 2016/05/03 at 07:19:10, Anand Mistry wrote:
> On 2016/05/03 06:16:14, Ken Rockot wrote:
> > On 2016/05/03 at 05:51:54, Anand Mistry wrote:
> > > I think this logic is subtle and unclear. Why not just zero-initialise
> > port_status?
> > > 
> > > Also, why is this logic different from the producer dispatcher, given the
two
> > functions are largely the same?
> > 
> > Sorry, are you asking why they aren't using shared implementation? If so,
it's
> > because they didn't start out looking so similar, and I never took the time
to
> > unify them.
> 
> I mean that these two functions are mostly the same. But you're changing this
to an "else if" and the producer's condition to just an "else", even though
they're currently both the same "if" condition.

Oh... oh god. Yikes. I stared at this for like 3 minutes and didn't notice. The
difference was unintentional. Fixed. Thanks!

> 
> > 
> > Zero-initializing PortStatus is not the correct thing to do. If
GetPortStatus
> > fails, the PortStatus value should not be used. For example, has_messages is
> > neither true nor false: it's unspecified.
> 
> Good point.
> 
> > 
> > I don't disagree that the logic is subtle and potentially unclear, but I
don't
> > think I'm making it worse here. Fixing that is beyond the scope of this CL.
> 
> I think using the result of the "if" condition in the "else if" branch is
subtle. I would strongly prefer if the condition was more explicit. It's
currently two separate conditionals, which is quite clear when using the result
of the first in the second.

[Anand Mistry (off Chromium), 2016-05-04 00:50:03]

https://codereview.chromium.org/1936333002/diff/40001/mojo/edk/system/data_pi...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/40001/mojo/edk/system/data_pi...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
(port_status.has_messages && !in_transit_) {
I'm still pushing back on this construct because I think it's terrible. It may
be fine for us that are very familiar with C++ and this code base, but it's
going to confuse anyone that's new to this.

Basically, my opinion is that it should be possible to evaluate any branch of an
if block independently. There shouldn't be a sequential dependency in the
evaluation of the branch conditions. If you need a result in multiple branches,
pull it out of the if block.

Sorry, but that's my opinion on this.

[Ken Rockot(use gerrit already), 2016-05-04 00:56:34]

Oook then
On May 3, 2016 5:50 PM, <amistry@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1936333002/diff/40001/mojo/edk/system/data_pi...
> File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):
>
>
>
https://codereview.chromium.org/1936333002/diff/40001/mojo/edk/system/data_pi...
> mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
> (port_status.has_messages && !in_transit_) {
> I'm still pushing back on this construct because I think it's terrible.
> It may be fine for us that are very familiar with C++ and this code
> base, but it's going to confuse anyone that's new to this.
>
> Basically, my opinion is that it should be possible to evaluate any
> branch of an if block independently. There shouldn't be a sequential
> dependency in the evaluation of the branch conditions. If you need a
> result in multiple branches, pull it out of the if block.
>
> Sorry, but that's my opinion on this.
>
> https://codereview.chromium.org/1936333002/
>

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

[Ken Rockot(use gerrit already), 2016-05-04 02:57:05]

https://codereview.chromium.org/1936333002/diff/40001/mojo/edk/system/data_pi...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1936333002/diff/40001/mojo/edk/system/data_pi...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:528: } else if
(port_status.has_messages && !in_transit_) {
On 2016/05/04 at 00:50:03, Anand Mistry wrote:
> I'm still pushing back on this construct because I think it's terrible. It may
be fine for us that are very familiar with C++ and this code base, but it's
going to confuse anyone that's new to this.
> 
> Basically, my opinion is that it should be possible to evaluate any branch of
an if block independently. There shouldn't be a sequential dependency in the
evaluation of the branch conditions. If you need a result in multiple branches,
pull it out of the if block.
> 
> Sorry, but that's my opinion on this.

done

[Anand Mistry (off Chromium), 2016-05-04 04:55:05]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-05-04 04:55:06]

lgtm

[commit-bot: I haz the power, 2016-05-04 04:55:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936333002/60001

[commit-bot: I haz the power, 2016-05-04 04:59:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-04 05:01:02]

Description was changed from

==========
[mojo-edk] Fix uninitialized reads in EDK dispatchers

BUG=608434
R=amistry@chromium.org
==========

to

==========
[mojo-edk] Fix uninitialized reads in EDK dispatchers

BUG=608434
R=amistry@chromium.org

Committed: https://crrev.com/ff3939d43c6977a4f34bd6e9a827b6823d731be2
Cr-Commit-Position: refs/heads/master@{#391447}
==========

[commit-bot: I haz the power, 2016-05-04 05:01:03]

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