You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/12/11 09:42:36 UTC

Review Request 41253: Added SSL_SENT_SHUTDOWN option to SSL_set_shutdown.

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

Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
-------

It has been observed that when only the receive side of the shutdown option is
set, the current file descriptor, when reused in another open file ends up with
data from the ssl socket. This change is to ensure that both send and receive
sides of the socket are terminated.


Diffs
-----

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 55b91dd47bb5bd5e97147d0af91c7899fd42702c 

Diff: https://reviews.apache.org/r/41253/diff/


Testing
-------

make check.


Thanks,

Jojy Varghese

Re: Review Request 41253: Added SSL_SENT_SHUTDOWN option to SSL_set_shutdown.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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


Patch looks great!

Reviews applied: [41253]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 11, 2015, 8:42 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41253/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2015, 8:42 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3773
>     https://issues.apache.org/jira/browse/MESOS-3773
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> It has been observed that when only the receive side of the shutdown option is
> set, the current file descriptor, when reused in another open file ends up with
> data from the ssl socket. This change is to ensure that both send and receive
> sides of the socket are terminated.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 55b91dd47bb5bd5e97147d0af91c7899fd42702c 
> 
> Diff: https://reviews.apache.org/r/41253/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41253: Changed ownership semantics of ssl connect socket.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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


Bad patch!

Reviews applied: [41252]

Failed command: ./support/apply-review.sh -n -r 41252

Error:
 2015-12-14 23:54:48 URL:https://reviews.apache.org/r/41252/diff/raw/ [4489/4489] -> "41252.patch" [1]
error: patch failed: src/tests/containerizer/provisioner_docker_tests.cpp:691
error: src/tests/containerizer/provisioner_docker_tests.cpp: patch does not apply

- Mesos ReviewBot


On Dec. 14, 2015, 9:27 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41253/
> -----------------------------------------------------------
> 
> (Updated Dec. 14, 2015, 9:27 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3773
>     https://issues.apache.org/jira/browse/MESOS-3773
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> libprocess Socket shares the ownership of the file descriptor with libevent. In
> the destructor of the libprocess libevent_ssl socket, we call ssl shutdown which
> is executed asynchronously. This causes the libprocess socket file descriptor to
> be closed (and possibly reused) when the same file descriptor could be used by
> libevent/ssl. Since we set the shutdown options as SSL_RECEIVED_SHUTDOWN, we
> leave the any write operations to continue with possibly closed file descriptor.
>     
> This change solves the above issue by copying(dup) the original file descriptor
> and hands over the copy to libevent ssl. The copied descriptor is then managed
> by libprocess Socket.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 2669b1a1d8f275b89c75d5f12fc696be2b277220 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 55b91dd47bb5bd5e97147d0af91c7899fd42702c 
> 
> Diff: https://reviews.apache.org/r/41253/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41253: Changed ownership semantics of ssl connect socket.

Posted by Jojy Varghese <jo...@mesosphere.io>.

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

(Updated Dec. 14, 2015, 9:27 p.m.)

Review request for mesos and Joris Van Remoortere.

Changes
-------

fixed ownership semantics.

Summary (updated)
-----------------

Changed ownership semantics of ssl connect socket.

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

Repository: mesos

Description (updated)
-------

libprocess Socket shares the ownership of the file descriptor with libevent. In
the destructor of the libprocess libevent_ssl socket, we call ssl shutdown which
is executed asynchronously. This causes the libprocess socket file descriptor to
be closed (and possibly reused) when the same file descriptor could be used by
libevent/ssl. Since we set the shutdown options as SSL_RECEIVED_SHUTDOWN, we
leave the any write operations to continue with possibly closed file descriptor.

This change solves the above issue by copying(dup) the original file descriptor
and hands over the copy to libevent ssl. The copied descriptor is then managed
by libprocess Socket.

Diffs (updated)
-----

3rdparty/libprocess/src/libevent_ssl_socket.hpp 2669b1a1d8f275b89c75d5f12fc696be2b277220
3rdparty/libprocess/src/libevent_ssl_socket.cpp 55b91dd47bb5bd5e97147d0af91c7899fd42702c

Diff: https://reviews.apache.org/r/41253/diff/

Testing
-------

make check.

Thanks,

Jojy Varghese

Re: Review Request 41253: Added SSL_SENT_SHUTDOWN option to SSL_set_shutdown.

Posted by Jojy Varghese <jo...@mesosphere.io>.

(Updated Dec. 14, 2015, 6:13 p.m.)

Review request for mesos and Joris Van Remoortere.

Changes
-------

updated description to make the change more understandable.

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

Repository: mesos

Description (updated)
-------

libprocess Socket shares the ownership of the file descriptor with libevent. In the destructor of the libprocess libevent_ssl socket, we call ssl shutdown which is executed asynchronously. This causes the libprocess socket file descriptor to be closed (and possibly reused) when the same file descriptor could be used by libevent/ssl. Since we set the shutdown options as SSL_RECEIVED_SHUTDOWN, we leave the any write operations to continue with possibly closed file descriptor.

This change is to ensure that both send and receivesides of the socket are terminated so that we avoid the race condition between libevent continuing with writes when the libprocess socket is closed.

An alternate/better solution is to ::dup the libprocess socket's file descriptor and then manage its life cycle. This solution is a little more involved and needs more time.

Diffs
-----

3rdparty/libprocess/src/libevent_ssl_socket.cpp 55b91dd47bb5bd5e97147d0af91c7899fd42702c

Diff: https://reviews.apache.org/r/41253/diff/

Testing
-------

make check.

Thanks,

Jojy Varghese