Hi,
On Mon, 24 Jan 2000, Richard Levitte - VMS Whacker wrote:
> This morning, I got to pondering reference counts, and it dawned on
that used to happen to me in the middle of the night (with a cold sweat
and the sensation of something slimy crawling across my face).
> me to look if that reference count gets updated before or after we
> start to refer to it. I've only looked in a few places, but it looks
> like we update the reference counter (locked) after we start refering
> to the object in question. A little like this:
>
> struct foo {
> void * dummydata;
> int references;
> };
>
> /* ... */
>
> struct foo cookie;
>
> cookie = bar;
> CRYPTO_add(cookie->references, 1, CRYPTO_LOCK_FOO);
>
> This is, of course, just a random example. But if you look in the
> source, I'd say it's littered with things like that.
>
> So, what's the danger? Well, what if another thread happens to have a
> pointer to the same instance of struct foo (a copy of bar, basically),
> and just happens to do a FOO_free(bar_copy) right between the two
> statements above? What if references got decreased to 0 by that other
> thread and that instance of struct foo actually got free'd? With
> that, we get in trouble sooner or later...
Not unlike my old hobby-horse;
SSL_SESSION *sess;
sess = SSL_get_session(ssl);
CRYPTO_add(sess->references, 1, CRYPTO_LOCK_SSL_SESSION);
What's the problem in this case? Well between the get_session and adding
the reference count, the session may get freed. If another thread is
controlling the SSL streaming and this code is taking place as an
"innocent bystander" (eg. some administrative stat-gathering thread that
grabs info on all the running whatevers) then the session could disappear
between the two calls. Eg. the SSL connection closes and the session cache
expires the session because it has passed its used-by date. Of course
statistically it would be tough to create a situation in which this is
likely to happen - but it is there none-the-less. I think you've found
another (unless the code you didn't quote has already locked something
else that prevents the ref_count==0 case occuring at the wrong time), and
there are a few others lurking about. I think most of this has survived
because there was some unwritten rule that certain structures' life-spans
are assumed to lie strictly within others, and probably some similar
assumptions on what kind of structures are potentially up for some
multi-threaded fun-n-games.
It's not just that functions like SSL_get_session should increment the
reference count for consistency sake, they *must* increment it because
attempting to do it yourself after the fact leaves an (albeit slim) race
condition. Of course this would require dependant code to add a
SSL_SESSION_free (or equivalent) to avoid memory leaks ...
> Incidently, it looks like someone (rse, according to cvs annotate)
> found a case like that in ssl/s2_clnt.c(get_server_hello()) (possibly
> others as well, haven't looked further). Unfortunately, that fix was
> apparently not understood:
>
> #if 0 /* What is all this meant to accomplish?? */
> /* hmmm, can we have the problem of the other session with this
> * cert, Free's it before we increment the reference count. */
> CRYPTO_w_lock(CRYPTO_LOCK_X509);
> s->session->peer=s->session->sess_cert->key->x509;
> /* Shouldn't do this: already locked */
> /*CRYPTO_add(&s->session->peer->references,1,CRYPTO_LOCK_X509);*/
> s->session->peer->references++;
> CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> #else
> s->session->peer = s->session->sess_cert->peer_key->x509;
> /* peer_key->x509 has been set by ssl2_set_certificate. */
> CRYPTO_add(&s->session->peer->references, 1, CRYPTO_LOCK_X509);
> #endif
Regards,
Geoff
----------------------------------------------------------------------
Geoff Thorpe Email: [EMAIL PROTECTED]
Cryptographic Software Engineer, C2Net Europe http://www.int.c2.net
----------------------------------------------------------------------
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
