masarati@aero.polimi.it writes:

...

ldap_new_connection(), if ( connect ) and lconn_server->lud_exts contain the tls ext, first unlocks and then re-locks ld_req_mutex and ld_res_mutex. As far as I understand, while the former is actually held by the caller(s) of ldap_new_connection(), the latter is not. If this analysis is correct, I'll post an ITS.

It is only relevant when the 'connect' argument is != 0: In request.c:ldap_send_server_request() and open.c:ldap_open_defconn().

ldap_result() locks ldap_res_mutex, which is one road to the first case. Hopefully all roads do something similar, I haven't checked.

However, open.c doesn't lock either mutex: /* Caller should hold the req_mutex if simultaneous accesses are possible */ int ldap_open_defconn( LDAP *ld ) Looks like the "if" in the comment at best should have listed some more cases, to avoid code paths into using these mutexes.

OK, I've checked.

ldap_new_connection() must be called holding ld_req_mutex; ld_res_mutex must be also held if connection != 0 or bind != NULL

ldap_new_connection() is called by ldap_send_server_request() with connect = 1; this, in turn, is called holding ld_req_mutex but not ld_res_mutex

ldap_new_connection() is also called by:

- ldap_open_defconn() with connect = 1; this in turn must be called holding ld_req_mutex

- ldap_init_fd() with connect = 0 and bind = NULL; this function does not hold any mutex (nor any is required since the LDAP structure has just been created and thus cannot be shared)

- ldap_open_internal_connection() (same as above; never used in OpenLDAP code)

I guess we need to eliminate the unlock/lock of ld_res_mutex and inform ldap_new_connection() about whether ld_req_mutex is locked or not...

p.

masarati@aero.polimi.it writes:

...

I guess we need to eliminate the unlock/lock of ld_res_mutex and inform ldap_new_connection() about whether ld_req_mutex is locked or not...

How about lock order? Must ld_res_mutex be held when locking ld_req_mutex? Anyway, sounds like it does need an ITS, which the commits should refer to.

By now, I'm just adding thread debugging code, which is harmless in production, and is only meant to track the issue. I'll open an ITS and reference it as soon as I start introducing actual modifications to the code.

p.