masarati(a)aero.polimi.it writes: > The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's > no > longer tests for gmtime_r() in configure. Is it time to cleanup a bit? Grepping some OpenLDAP versions, I'm not sure there ever was a test for it. #Define it if everything protected by gmtime_mutex can be avoided. (gmtime_r, localtime_r, anything else?) After checking that the #ifdef HAVE_GMTIME_R branches (still) work, of course. Should probably also check that the prototype is as expected, in case there have been multiple versions as with 'int strerror_r()' vs 'char *strerror_r()'.

The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit? p.

I'm inclined towards defining a private function, with an interface analogous to that of the tread-safe function, and use it all times, re-entrant as appropriate. If the re-entrant function is available, a #define could be used instead. Otherwise, the wrapper would take care of the rest. This would solve the issue once for all, and clean up quite a bit the code, as any mutex would no longer need to be exposed.

struct tm *ldap_pvt_gmtime(

time_t *t, struct tm *tm) { struct tm *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_lock( &gmtime_mutex ); #endif tm_ptr = gmtime( t );

*tm = *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_unlock( &gmtime_mutex ); #endif return tm; }

> But not exposing gmtime_mutex would break third-partly modules that > use gmtime()/localtime() correctly, which means protecting them with > gmtime_mutex. We could however register gmtime_mutex in libldap, > similar to registering which memory functions to use. Right. But as far as I can tell, this is already the case of other mutex'es, including ldap_int_ctime_mutex, which is static, and other ldap_int_*_mutex'es, that are internal to libldap.

I'd rather favor exposing an API that allows to lock/unlock (trylock?) hidden mutexes. Although I fear this could open a can of worms.

I'm seeing a lot of warnings about lutil_tm declaration inside function params when compiling a fresh HEAD now. Seems to me there's something missing from these recent commits.

masarati(a)aero.polimi.it writes: > Other time-related, and significantly those using the > lutil_tm struct, remained in liblutil. Probably ldap_pvt.h needs to > include lutil.h. Suggestions? I fixed it.

The gcc warning explained clearly enough what was wrong.

I still disagree that moving gmtime_mutex out of slapd was the right thing to do in the middle of RE24 though. Though i suppose we could '#define gmtime_mutex ldap_int_gmtime_mutex' so slapd at least remains source-code compatible.

masarati(a)aero.polimi.it writes:

>> I still disagree that moving gmtime_mutex out of slapd was the right >> thing to do in the middle of RE24 though. Though i suppose we could >> '#define gmtime_mutex ldap_int_gmtime_mutex' so slapd at least remains >> source-code compatible. > > Mmmmh, ldap_int_gmtime_mutex is (and should in the long term remain) > static. Um, this is blowing up to a bit bigger issue than warranted, but anyway: Long term, possibly - but only if we provide it to other packages somehow or accept another mutex from them. See below. But in RE24, it _was_ exported.

> I see your point in hiding a symbol within bugfix releases, but I > wonder how many applications cared about that in the first place, and > how > many developers are aware of the issue. Either we try to be upgrade-friendly or not. Probably very few third-party modules are affected by this particular issue, just like very few are affected by many other particular issues. But these improbabilities do add up until we get an upgrade-unfriendly package. That doesn't translate to an absolute freeze of e.g. slap.h between major releases, but we can at least avoid changes that could just as well be done in a compatible way. Previously there was one correct way to do it, now there is one different correct way, and - quite unnecessarily - there is no overlap beween the two. As for this particular mutex and the corresponding ctime mutex, all threaded packages using gmtime and localtime must cooperate in order to get correct behavior. For example: nm /usr/lib/lib<whatever>.a | egrep 'localtime|gmtime|ctime' | sort -u /usr/lib/libsasl2.a: ctime localtime /usr/lib/libssl.a: gmtime localtime /usr/lib/libcrypto.a OPENSSL_gmtime X509_gmtime_adj gmtime_r localtime I haven't looked too closely at the code, but possibly there is no way for a program using OpenSSL or Cyrus SASL to use the non-_r functions correctly. Didn't find any mutex protecting them, anyway. Come to think of it, this might explain some of the syncrepl timing problems. Anyway, if they all use the time functions, they must all use the same mutex or the same wrapper function. > In general, I realize the library design needs to take into account two > different issues related to re-entrancy: > > 1) the need for thread-safe behavior of libldap when usen in multithread > clients (libldap_r) Yup. Which we really need to get around to officially supporting. If nothing else, because it gets silly to complain about other non-reentrant third-party code. > 2) the need to cure potential flaws in non re-entrant libraries > OpenLDAP's > depend on. The general cure for (2) is to run slapd single-threaded. Specific such issues of re-entrancy will have to be handled on a case-by-case basis. > (...) > As an entirely separate issue, we need to provide means to make > thread-safe the use of potentially unsafe functions that we need. This > is > a bit harder, because: > > a) we may need to use potentially unsafe functions in pieces of code not > necessarily designed for thread-safety (e.g. liblutil in this case) > > b) we may need to cooperate with other protections put in place by > applications. Yes. And we need to _not_ insist that other packages do things Our Way. 3 packages that all insist that others do it their way cannot cooperate.