Might we have a potential MT-safety problem?

[Richard Levitte - VMS Whacker]

This morning, I got to pondering reference counts, and it dawned on
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...

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


Personally, I'd like to do the then clause rather than the else
clause, for MT safety's sake.

Comments?

-- 
Richard Levitte   \ Spannv�gen 38, II \ [EMAIL PROTECTED]
Redakteur@Stacken  \ S-161 43  BROMMA  \ T: +46-8-26 52 47
                    \      SWEDEN       \ or +46-708-26 53 44
Procurator Odiosus Ex Infernis             -- [EMAIL PROTECTED]

Unsolicited commercial email is subject to an archival fee of $400.
See <http://www.stacken.kth.se/~levitte/mail/> for more info.
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [EMAIL PROTECTED]
Automated List Manager                           [EMAIL PROTECTED]