mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.
Date Fri, 09 Dec 2016 19:35:57 GMT


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > I went over this with Greg.
> > 
> > This looks good, some suggestions around some of the existing logic that we should
fix.
> > 
> > Also, some other items:
> > 
> > (1) It looks like the shutdown override is no longer overriding. The code in shutdown()
looks unnecessary? Do we need to clear buffers or something?
> > (2) It looks like we don't need the SSL_set_shutdown workaround code since we don't
use BEV_OPT_CLOSE_ON_FREE?

Thanks for the review, Ben! I've addressed the comments below, and I'll submit follow-up patches
for your two points above, as well as for the calls to `EVUTIL_SOCKET_ERROR` in `accept_SSL_callback`.


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 366-367
> > <https://reviews.apache.org/r/53802/diff/6/?file=1573360#file1573360line366>
> >
> >     Let's remove this part of the case:
> >     
> >     ```
> >     events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0)
> >     ```
> >     
> >     and take the CHECK out of the final error case. We can just call evutil_socket_error_to_string
even if it's 0 AFAICT (although you should check on windows if it's ok).

I checked out the windows code in libevent and calling `evutil_socket_error_to_string(0)`
looks safe:

```
const char *
evutil_socket_error_to_string(int errcode)
{
  /* XXXX Is there really no built-in function to do this? */
  int i;
  for (i=0; windows_socket_errors[i].code >= 0; ++i) {
    if (errcode == windows_socket_errors[i].code)
      return windows_socket_errors[i].msg;
  }
  return strerror(errcode);
}
```


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 417-425
> > <https://reviews.apache.org/r/53802/diff/6/?file=1573360#file1573360line417>
> >
> >     Should we also pull out the CHECKs for recv and send requests being null? It
seems we actually allow the caller to do this, and the idea being CHECKs is generally to check
our internal invariants. So the caller could induce a CHECK failure here by calling send()
without waiting for a connect to finish.
> >     
> >     Unfortunately, I'm not sure how libevent handles the case where the socket is
not connected and we write to its buffer (hopefully that would give us a EVUTIL_SOCKET_ERROR
back in the event_callback).

OK, I removed the null checks. As we discovered yesterday in the libevent documentation, writing
to the buffer before the connection is established is actually allowed: "It is okay to add
data to the output buffer before the connect is done." (http://www.wangafu.net/~nickm/libevent-book/Ref6_bufferevent.html)


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 367
> > <https://reviews.apache.org/r/53802/diff/6/?file=1573360#file1573360line367>
> >
> >     It's not clear if this case is possible. At least looking at the documentation
and some of the example code it seems to suggest that we are expected to see a non-zero EVUTIL_SOCKET_ERROR
whenever BEV_EVENT_ERROR is set. This is because EVUTIL_SOCKET_ERROR just returns errno (or
WSAGetLastError on windows) and unless libevent is clearing these it wouldn't be safe to look
at errno since it might still be set to a stale value.
> >     
> >     Also, libevent says that EVUTIL_SOCKET_ERROR isn't idempotent on all platforms
so we should pull this out into a variable.

Since I've removed the `events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0`
check, this function has only a single invocation of `EVUTIL_SOCKET_ERROR`. For this reason,
I moved the `if` branch which calls `EVUTIL_SOCKET_ERROR` up to the top so that we invoke
the macro ASAP without the need to store it in a local variable.


- Greg


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


On Dec. 9, 2016, 7:10 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2016, 7:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
>     https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>    there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>    EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 57eaf4f607d0628db466cc1a139772eeeaa51136

>   3rdparty/libprocess/src/libevent_ssl_socket.cpp dddd0e292a8b0d470f4e199db08f09a0c863d73c

> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> -------
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message