[Pkg-openssl-devel] Bug#534687: libssl0.9.8: lh_retrieve modifies global data, bug in valgrind or OpenSSL?

[Sebastian Andrzej Siewior]

On 2009-06-26 21:09:13 [+1000], Russell Coker wrote:
> ==27681== Possible data race during write of size 4 at 0x652e2f0 by thread #5
> ==27681==    at 0x5330C84: lh_retrieve (lhash.c:254)
> ==27681==    by 0x52D12E1: def_get_class (ex_data.c:301)
> ==27681==    by 0x52D180A: int_new_ex_data (ex_data.c:404)
> ==27681==    by 0x5318BD7: RSA_new_method (rsa_lib.c:185)
> ==27681==    by 0x531B76C: rsa_cb (rsa_asn1.c:80)
> ==27681==    by 0x534CB42: asn1_item_ex_combine_new (tasn_new.c:177)
> ==27681==    by 0x53501E4: ASN1_item_ex_d2i (tasn_dec.c:399)
> ==27681==    by 0x53502B3: ASN1_item_d2i (tasn_dec.c:134)
> ==27681==    by 0x534863C: d2i_PublicKey (d2i_pu.c:96)
> ==27681==    by 0x534624F: X509_PUBKEY_get (x_pubkey.c:364)
> ==27681==    by 0x5346C07: d2i_PUBKEY (x_pubkey.c:390)
> ==27681==    by 0x40D480: SelectorInfo::Parse(char*) (dkimverify.cpp:1312)
> 
> The above is from a helgrind run on my AMD64 system.
> 
> void *lh_retrieve(LHASH *lh, const void *data)
>         {
>         lh->error=0;
> 
> The relevant code from lhash.c is above, it writes to data in the LHASH
> structure pointed to by the first parameter.
> 
> static EX_CLASS_ITEM *def_get_class(int class_index)
>         {
>         CRYPTO_w_lock(CRYPTO_LOCK_EX_DATA);
>         p = lh_retrieve(ex_data, &d);
> 
> The relevant code from ex_data.c is above.  It seems that the lock
> CRYPTO_LOCK_EX_DATA will protect the data, so maybe valgrind is getting this
> wrong.  Valgrind 3.3.1 seemed to miss the CRYPTO_w_lock() type calls, but
> I'm using 3.4.1.  Maybe this is a bug in valgrind?  If you think so then feel
> free to reassign it.

I am tempted to close this. Can you still reproduce this with current
openssl & valgrind? RSA_new_method() is no longer using the class thingy
/ lhash so your old testcase won't do. And then 1.1.0c is using
pthread's locking functions so valgrind should know by now when a lock
is held.

Sebastian