[omniORB] crash in openssl code

[Duncan Grisby]

On Fri, 2024-02-23 at 15:50 +0100, Michael Teske wrote:

> 
> 
> > It has always worked in the past with OpenSSL as well. Has
> > something
> > changed with OpenSSL that means it is no longer thread safe for
> > this?
> 
> Every reference I find tells me that this can cause severe problems
> and crashes. In the github issue I've opened

[...]
> It works surprisingly well until something unexpected happens, e.g. a
> handshake in beween... I get the crash almost exclusively on
> startup/opening of connection, to reproduce it I had to restart our
> system up to 100 times.

It is indeed surprising that this is not seen more often. It will only
happen with bidirectional GIOP, because normal GIOP strictly alternates
between writes and reads, and it appears to be the case that OpenSSL is
nearly thread safe, in that mostly you get away with overlapped reads
and writes. I suspect it might be a change in TLS 1.3 that makes
renegotiation happen more often that provokes this.

It will be very easy to change the omniORB code to always use non-
blocking sockets, and to have a per-connection lock around all calls
into OpenSSL. Aside from the lock, it already has all that
functionality, to be able to handle calls with timeouts. The question
is whether doing so will have a noticeable impact on performance. If it
is too costly to do it always, it will have to be something that is
only used with bidirectional connections, which is also possible, but a
bit more tricky.


> Another thing, apart from that, shouldn't 
>  
> sslConnection::Peek() 
> 
> lock pd_belong_to->pd_collection_lock
>  
> as well (before calling SSL_pending()) ?

Why do you think that?  Once there is a lock to protect the SSL object,
it certainly should acquire that, but it does not need to hold the
SocketCollection lock unless it calls into the base SocketHolder::Peek,
and that does acquire the lock.

Regards,

Duncan.

-- 
Duncan Grisby <duncan at grisby.org>