Hi there,
I'm sending this to openssl-dev so we can continue the thread there, and I
apologise in advance for the length of this post (there's no prize for
finishing either). As this is evolving into a wider discussion than just
one commit I thought it best to get it out of the faces of anyone who is
just looking for cvs commits. Please just reply just on -dev. TIA. BTW:
I'm not flame-baiting in case you find my comments obstinate, I'm just
interested in seeing an improved OpenSSL.
As for the patch in question - I'm not going to lose too much sleep over
which way the core developers go with it, it was put forward for comments
by me on the openssl-dev list a number of days ago and nobody (except
Steve) had anything to say about it at all. I happen to need that change
to make the API "safe" for my own purposes, so Mark commited it in lieu of
any objection or compelling reason not to. I can just as easily work with
an equivalent "patch" in my own work if the consensus is to reverse it or
patch it "out of harm's way".
On Tue, 16 Nov 1999, Bodo Moeller wrote:
> Maybe we should have a naming convention for ..._set_... calls too?
> There are already such ambiguities for them, e.g. SSL_CTX_set_tmp_rsa
> vs. SSL_CTX_set_tmp_dh.
It seems to be a play-off between backwards compatibility, and having a
good library-API. I prefer the latter, but I don't have as much legacy
code to maintain as some others do, and I probably find the current
"inconsistencies" a little more distasteful than some others do.
I assume the mix-and-match method of reference counting is evidence that
OpenSSL (continuing from SSLeay) has grown from very modest beginnings and
is showing signs of wearing a suit too small for itself. It seems that
everything works if you make the calls the way the authors had intended
rather than making the calls the way the authors made available.
> Except that you'd have to fix similar calls in other OpenSSL
> applications too, with "#if OPENSSL_VERSION_NUMBER ..." and
> everything. We cannot count on developers to notice changes like
> this.
Well I guess it's a catch-22. The current API seems to me to be faulty.
SSL_get_session() appears plainly to be an exported API function whose
role is pretty well self-explanatory. Just as so many other similar
functions are (SSL_get_certificate, SSL_CTX_use_certificate, etc) one is
operating on "objects" and supplying or obtaining pointers to other
"objects", and one should reasonably hope that the management of those
objects works in a coherent and logical way. One also assumes that the
library allows you to use them as you please provided you just follow
basic common sense rules (if you "get" one, don't forget to "free" one).
C++ makes those rules a touch easier to follow but C can cope. :-)
If 0.9.4 has faults, surely it is preferable to fix them in 0.9.5 and for
dependant code to stick with 0.9.4 until it has been brought in line with
0.9.5's fixes. Otherwise we end up stuck in a feedback loop of being able
to rapidly add functionality without ever being able to review design
deficiencies (or "behaviour" bugs) for fear of breaking yesterdays code.
This because people, for some reason I've never fully grasped, want to be
able to compile yesterdays code with todays snapshot even when yesterdays
code *should* be fixed first?
Perhaps this is the kind of thing that needs to wait for a new major
version number though? This is really up to the core developers and what
they consider to be the overriding goals - if any change in behaviour is
to be avoided at all costs then I'll continue to work around it or hoard
my own modifications. :-)
> > As for code out there using OpenSSL and doing the same thing; the change
> > doesn't break that code so much as replace one form of breakage with
> > another. Calls of the form in the above 4 c files run the risk (in a
> > multi-threaded situation certainly - multi-process too if shared cache's
> > are being used) of sessions being removed between the "internal" call to
> > SSL_get_session and the result being passed into SSL_set_session. [...]
>
> The session obtained from SSL_get_session(ssl) is used only as long as
> the thread holds the SSL structure ssl and does not do any other
> modifications to it (such as calling any of the SSL_... I/O
> functions), which guarantees that the session still exists. An SSL
> structure may not be used by multiple threads concurrently (it does
> have a reference count, but there's no compelling reasons for this --
> I think the count is always 1); with SSL_get_session not increasing
> the session's reference count, the same applies for the SSL structure
> and its session as returned by SSL_get_session.
I was doing something that I expect to be perfectly legitimate that breaks
this as I already explained at the outset on openssl-dev. I wanted to hold
the SSL_SESSION reference after the SSL had completed. I was then at the
mercy of anything else that might "expire" the session before I attempted
to refer to it again. As SSL_get_session is an exported API function and
SSL_SESSION has reference counting, it seemed only reasonable that what I
was returned would be a valid reference, and not just a copy of a pointer.
There seems no reason why there should be any assumption that the
SSL_SESSION lifespan sits within the SSL structure - it's an object with a
reference count!
Now why an SSL can't have a reference count higher than one, and be
relevant to multiple threads each wanting a safe reference to it is beyond
me. It seems to be a perfectly normal sort of expectation - one could have
a thread that is keeping an eye on all running SSLs whilst each SSL has a
dedicated thread running the selects, reads and writes. This would seem a
perfectly reasonable thing to do - the "observation" thread can simply see
when an SSL has been finished with (and its thread has stopped or exited)
and then free its own reference without fear of a race condition in which
it could attempt to examine deallocated memory before noticing the thread
has exited.
It seems the point you're making is that SSL_SESSIONs *are* safe provided
you work with them within the confines of the SSL structure (and
thread/process) they came from. However, I can't see any reasonable
explanation for why that must be the case. I believe I've also made a
reasonable case for why it should not be.
> Also the locking introduced in the patch to SSL_get_session
>
> SSL_SESSION *SSL_get_session(SSL *ssl)
> {
> - return(ssl->session);
> + SSL_SESSION *sess;
> + /* Need to lock this all up rather than just use CRYPTO_add so that
> + * somebody doesn't free ssl->session between when we check it's
> + * non-null and when we up the reference count. */
> + CRYPTO_r_lock(CRYPTO_LOCK_SSL_SESSION);
> + sess = ssl->session;
> + if(sess)
> + sess->references++;
> + CRYPTO_r_unlock(CRYPTO_LOCK_SSL_SESSION);
> + return(sess);
> }
>
> does not make too much sense because if you assume that other threads
> may free the session then I'd say you'd just as well have to take into
> account the possibility that they may free the SSL structure. The
No. If the reference counting mechanism was consistent then this would not
be the case. If you are making this call - then you must have a valid SSL
reference with which to make it! Sure, other threads may free *their*
references to the SSL structure but can not free *yours* (well they would
have to go out of their way to do so which is the real point). What you
say is only true if one performs the same inconsistent "reference-sharing"
on SSLs as SSL_get_session() currently does with SSL_SESSIONs (before
that commit anyway).
Why not allow multiple (safe) references to the same SSL?? (in different
threads too??). Same goes for SSL_SESSIONs, X509s, et al. The fact
that locking is there and reference counts in place seems to suggest that
this is precisely what is intended! I see no reason to persist with an
approach that cripples it and enforces strange usage restrictions
(ie. saying "but if you use it *this* way, you'll be ok").
That is my whole point - reference counting works and is very flexible if
you implement it. Half-implement it and you just end up with a can of
worms.
> usage scenario for the version of SSL_get_session that increases the
> reference count would rather be the thread owning the SSL structure
> calling SSL_rget_session(ssl) and then passing the point to the
> SSL_SESSION structure to some other thread; not the pointer to the SSL
> structure.
This approach baffles me ... why should any one thread own an SSL
structure at all? If each thread that is interested obtains a reference,
and then dutifully frees its own reference when done, then the SSL
structure will silently and politely be cleaned up at the appropriate
moment. And nobody will have to wonder whether their pointers point to
something which still exists or not.
Regards,
Geoff
----------------------------------------------------------------------
Geoff Thorpe Email: [EMAIL PROTECTED]
Cryptographic Software Engineer, C2Net Europe http://www.int.c2.net
----------------------------------------------------------------------
May I just take this opportunity to say that of all the people I have
EVER emailed, you are definitely one of them.
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
