Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172265
---



Patch looks great!

Reviews applied: [58512]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 18, 2017, 6:04 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 18, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/1/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172291
---



AFAICT the same problem can occur in other places (eg. 
``StreamingResponseDecoderon_headers_complete``).

- James Peach


On April 18, 2017, 6:04 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 18, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/1/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172304
---



Looks good! Thanks for reporting the issue and also fixing it.

Can you add a test to go along with the fix? Otherwise, under the "Testing 
Done" section, the tests are _always_ expected to pass with/without the fix.


3rdparty/libprocess/src/decoder.hpp
Lines 699-701 (patched)


Kill this comment. 

In the decoder/encoder code using the http parser, it's fairly 
self-explanatory that the next callback is only invoked if the previous one 
succeeded. We do however, want to add a comment in the `on_message_complete()` 
handler though when this is not true.



3rdparty/libprocess/src/decoder.hpp
Line 711 (original), 714-716 (patched)


```cpp
// Note that `decoder->writer` can be `None()`.
```
hmm, this is evident from the conditional check later. 

Let's add a comment around when this can happen. How about:

```cpp
// This can happen if the callback `on_headers_complete()` had failed
// earlier (e.g., due to invalid query parameters).
```



3rdparty/libprocess/src/decoder.hpp
Lines 987-989 (patched)


Ditto as above.



3rdparty/libprocess/src/decoder.hpp
Line 1006 (original), 1017-1019 (patched)


Ditto as above.


- Anand Mazumdar


On April 18, 2017, 6:04 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 18, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/1/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anand Mazumdar]

> On April 19, 2017, 12:12 a.m., James Peach wrote:
> > AFAICT the same problem can occur in other places (eg. 
> > ``StreamingResponseDecoderon_headers_complete``).

James, the patch seems to address both the streaming request/response decoders.


- Anand


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172291
---


On April 18, 2017, 6:04 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 18, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/1/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/
---

(Updated April 19, 2017, 4:02 p.m.)


Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.


Changes
---

Addressed review comments.


Bugs: MESOS-7400
https://issues.apache.org/jira/browse/MESOS-7400


Repository: mesos


Description
---

If the callback `on_headers_complete()` fails, the decoder's writer
object is `None()`. So, when the callback `on_message_complete()` is
called, we should not crash in that case.
Note that the `CHECK(decoder->writer)` is valid for the callback
`on_body()` is valid since this callback is called only on a success
in `on_headers_complete()` callback.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
c0efef571815752cc28e5a0dc7a07adc82bb31d5 


Diff: https://reviews.apache.org/r/58512/diff/2/

Changes: https://reviews.apache.org/r/58512/diff/1-2/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

> On April 19, 2017, 4:02 a.m., Anand Mazumdar wrote:
> > Looks good! Thanks for reporting the issue and also fixing it.
> > 
> > Can you add a test to go along with the fix? Otherwise, under the "Testing 
> > Done" section, the tests are _always_ expected to pass with/without the fix.

Absolutely. Added 2 tests for this change. Confirmed the tests fail without the 
code changes, and pass with these changes.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172304
---


On April 19, 2017, 4:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 19, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172387
---



Patch looks great!

Reviews applied: [58512]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 19, 2017, 4:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 19, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172504
---




3rdparty/libprocess/src/decoder.hpp
Lines 208 (patched)


hmm, these seem not related to the review i.e., avoiding the crash. Can you 
create a separate patch with these changes and mark this as dependent on it?

Good catch!



3rdparty/libprocess/src/decoder.hpp
Lines 715 (patched)


set `decoder->failed=true` here? If it's already set earlier, add an 
explicit invariant check here before this line?



3rdparty/libprocess/src/decoder.hpp
Lines 1017 (patched)


Ditto as above.


- Anand Mazumdar


On April 19, 2017, 4:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 19, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

> On April 20, 2017, 4:06 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/decoder.hpp
> > Lines 208 (patched)
> > 
> >
> > hmm, these seem not related to the review i.e., avoiding the crash. Can 
> > you create a separate patch with these changes and mark this as dependent 
> > on it?
> > 
> > Good catch!

Separated these changes into https://reviews.apache.org/r/58580/ and made this 
RR depend on that one.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172504
---


On April 19, 2017, 4:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 19, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/
---

(Updated April 20, 2017, 5:18 p.m.)


Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.


Bugs: MESOS-7400
https://issues.apache.org/jira/browse/MESOS-7400


Repository: mesos


Description
---

If the callback `on_headers_complete()` fails, the decoder's writer
object is `None()`. So, when the callback `on_message_complete()` is
called, we should not crash in that case.
Note that the `CHECK(decoder->writer)` is valid for the callback
`on_body()` is valid since this callback is called only on a success
in `on_headers_complete()` callback.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
c0efef571815752cc28e5a0dc7a07adc82bb31d5 


Diff: https://reviews.apache.org/r/58512/diff/3/

Changes: https://reviews.apache.org/r/58512/diff/2-3/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172520
---


Fix it, then Ship it!





3rdparty/libprocess/src/decoder.hpp
Line 441 (original)


Nit: Can you do these cleanups in the previous review?


- Anand Mazumdar


On April 20, 2017, 5:18 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 20, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/
---

(Updated April 20, 2017, 5:40 p.m.)


Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.


Changes
---

Addressed review comments.


Bugs: MESOS-7400
https://issues.apache.org/jira/browse/MESOS-7400


Repository: mesos


Description
---

If the callback `on_headers_complete()` fails, the decoder's writer
object is `None()`. So, when the callback `on_message_complete()` is
called, we should not crash in that case.
Note that the `CHECK(decoder->writer)` is valid for the callback
`on_body()` is valid since this callback is called only on a success
in `on_headers_complete()` callback.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
c0efef571815752cc28e5a0dc7a07adc82bb31d5 


Diff: https://reviews.apache.org/r/58512/diff/4/

Changes: https://reviews.apache.org/r/58512/diff/3-4/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58512/#review172532
---



Patch looks great!

Reviews applied: [58580, 58512]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 20, 2017, 5:40 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 20, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>