Simplify SSL shutdown

[falbrechtskirchinger]

There are multiple problems with the MaxTimeoutTest timeout exceedance. As a first step, this simplifies SSL shutdown and also makes it correct on Windows (where SSL_shutdown() was only called once, AFAICT).

To handle max timeout, we need to bring in the timeout values and request start time, which is messy. Worse, even if ssl_delete is measured to take 0ms, max timeout is still exceeded elsewhere. This was the case with both versions I've tested.

As a sneak peek, here's what I tried today and hinted at yesterday:

Code

// In nonblocking mode, SSL_shutdown() returns 0 until the close_notify
// alerts have been sent and recevied, with SSL_get_error() providing
// details
set_nonblocking(sock, true);

auto res = 0;
time_t timeout_sec, timeout_usec;
while ((res = SSL_shutdown(ssl)) != 1) {
  auto err = SSL_get_error(ssl, res);
  switch (err) {
  case SSL_ERROR_WANT_READ: {
    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(
                        std::chrono::steady_clock::now() - start_time)
                        .count();
    calc_actual_timeout(max_timeout_msec, duration, read_timeout_sec,
                        read_timeout_usec, timeout_sec, timeout_usec);
    if (timeout_sec == 0 && timeout_usec == 0) { break; }
    if (select_read(sock, timeout_sec, timeout_usec) > 0) { continue; }
    break;
  }
  case SSL_ERROR_WANT_WRITE: {
    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(
                        std::chrono::steady_clock::now() - start_time)
                        .count();
    calc_actual_timeout(max_timeout_msec, duration, write_timeout_sec,
                        write_timeout_usec, timeout_sec, timeout_usec);
    if (timeout_sec == 0 && timeout_usec == 0) { break; }
    if (select_write(sock, timeout_sec, timeout_usec) > 0) { continue; }
    break;
  }
  default: break;
  }
  break;
}

set_nonblocking(sock, false);

I can share both versions and the related changes in a PR for you to decide what to do with.

[yhirose]

@falbrechtskirchinger thanks for the pull request. I am a bit scared to touching ssl_delete, since the code fixed some very sensitive bugs. Could you please carefully check the following issues and code changes first, then make sure that your change doesn't break any of them? If there are not enough unit test cases, it's ok to add additional tests in 'test/test.cc'.

• Fix multiple threading bugs including #699 and #697 #701
• Fix #1481 (with content provider) #1527
• Fix KeepAliveTest.SSLClientReconnectionPost problem #1921

[falbrechtskirchinger]

@falbrechtskirchinger thanks for the pull request. I am a bit scared to touching ssl_delete, since the code fixed some very sensitive bugs. Could you please carefully check the following issues and code changes first, then make sure that your change doesn't break any of them? If there are not enough unit test cases, it's ok to add additional tests in 'test/test.cc'.

* [Fix multiple threading bugs including #699 and #697 #701](https://github.com/yhirose/cpp-httplib/pull/701)

* [Fix #1481 (with content provider) #1527](https://github.com/yhirose/cpp-httplib/pull/1527)

* [Fix KeepAliveTest.SSLClientReconnectionPost problem #1921](https://github.com/yhirose/cpp-httplib/pull/1921)

After some reading, I certainly understand your hesitation. Still, I believe this to be the correct solution per the man page SSL_shutdown(3):

Shutdown Lifecycle
   Regardless of whether a shutdown was initiated locally or by the peer, if
   the underlying BIO is blocking, a call to SSL_shutdown() will return firstly
   once a close_notify alert message is written to the peer (returning 0), and
   upon a second and subsequent call, once a corresponding message is received
   from the peer (returning 1 and completing the shutdown process). Calls to
   SSL_shutdown() with a blocking underlying BIO will also return if an error
   occurs.

In case of an error, we SSL_free() anyway, but otherwise, we attempt to shut down the TLS connection properly/gracefully by sending and receiving the close_notify alert message.

SSL_shutdown() should be invoked exactly twice with a blocking socket. The old code called once on Windows and looped unnecessarily everywhere else (or tried to, since 1 should be returned on the second call, unless an error occurs).

Concerning max timeout, I've tried using a blocking socket and a non-blocking socket. Both implementations worked, though I prefer the non-blocking solution, as it doesn't rely on the internals of SSL_shutdown() for timing out.

[yhirose]

@falbrechtskirchinger thanks for the answer and the reference. It seems this pull request change is better than the current code, and I like this simpler code. I'll merge it and see if any unexpected issue will arise. Thanks for the excellent change and research!