Access Violation in SSL_accept() when deactivating TIdTCPServer

[rlebeau]

A user is experiencing an access violation in SSL_accept() (see https://stackoverflow.com/questions/51150235/) when TIdTCPServer is setup to use OpenSSL and there is an open connection that has NOT yet negotiated an SSL/TLS session when TIdTCPServer is being deactivated.

This can be reproduced using the following code, with Putty making a RAW connection:

procedure TSslCrash.HandlerOnExecute(AContext: TIdContext);
begin
  //
end;

procedure TSslCrash.HandlerOnConnect(AContext: TIdContext);
begin
  TIdSSLIOHandlerSocketBase(AContext.Connection.IOHandler).PassThrough := False;
end;

procedure TSslCrash.ButtonStartClick(Sender: TObject);
begin
  LServer := TIdTCPServer.Create;
  LIOHandler := TIdServerIOHandlerSSLOpenSSL.Create;

  LIOHandler.SSLOptions.Mode := sslmServer;
  LIOHandler.SSLOptions.Method := sslvTLSv1_2;
  LIOHandler.SSLOptions.VerifyMode := [];
  LIOHandler.SSLOptions.VerifyDepth := 0;
  LIOHandler.SSLOptions.CertFile := 'localhost.crt';
  LIOHandler.SSLOptions.RootCertFile := 'localhost.crt';
  LIOHandler.SSLOptions.KeyFile := 'localhost.key';

  LServer.Bindings.Add.Port := 10000;
  LServer.IOHandler := LIOHandler;
  LServer.OnExecute := HandlerOnExecute;
  LServer.OnConnect := HandlerOnConnect;
  LServer.Active := True;

  //Now open a RAW connection with Putty on port 10000 and keep it open
end;

procedure TSslCrash.ButtonStopClick(Sender: TObject);
begin
  if Assigned(LServer) then begin
    LServer.Active := False;  //This causes an AV in TIdSSLSocket.Accept

    FreeAndNil(LIOHandler);
    FreeAndNil(LServer);
  end;
end;

[rlebeau]

When TIdTCPServer is being deactivated, it disconnects active socket connections, failing any blocking socket operations in progress in other threads. In the case of SSL_accept(), that should unblock it so it can exit with an error code that TIdSSLSocket.Accept() can then detect and wrap into a raised exception (EIdOSSLUnderlyingCryptoError, EIdOSSLAcceptError, EIdSocketError, etc depending on the nature of the error code) in the context of the client thread that is waiting for the handshake to complete.

However, when TIdTCPServer is disconnecting a socket connection during deactivation, TIdTCPConnection.Disconnect() is called, which calls TIdIOHandler.Close(), which TIdSSLIOhandlerSocketOpenSSL has overridden to free its internal TIdSSLSocket object - the same object that is calling SSL_accept(). So it is quite likely that the underlying OpenSSL SSL object is being freed in TIdSSLSocket.Destroy() (which calls SSL_shutdown() and SSL_free()) in the context of the deactivating thread while still actively being used in TIdSSLObject.Accept() (which calls SSL_accept()) in the context of the client thread, thus causing an Access Violation.

A possible fix might be to change TIdCustomTCPServer.DoTerminateContext() to call AContext.Binding.CloseSocket() instead of AContext.Connection.Disconnect(False) so the IOHandler itself is not closed, just the underlying socket (similar to what TIdCustomTCPServer.StopListening() does when terminating its listening accept() threads).

Otherwise, TIdSSLSocket will have to take extra measures to protect its underlying SSL object from being destroyed while in use across thread boundaries.

[rlebeau]

Didn't mean to close the ticket

[chuacw]

Tested the fix. Overriding DoTerminateContext and calling AContext.Binding.CloseSocket; worked.

[SARTrack]

I also tested this, and it works. I assume somebody should change this in the master code.

[rlebeau]

Now that two different users have verified it is working, I have checked in the change.