On Tue, Oct 07, 2025 at 09:24:29PM +0200, Ruediger Pluem wrote: > > > On 10/7/25 6:04 PM, Joe Orton wrote: > > On Tue, Oct 07, 2025 at 03:54:29PM +0200, Ruediger Pluem wrote: > >> > >> > >> On 10/7/25 11:50 AM, Joe Orton wrote: > >>> On Tue, Oct 07, 2025 at 10:45:27AM +0100, Joe Orton wrote: > >>>> There should already be a global lock held by API consumer, since the > >>>> socache provider is marked with AP_SOCACHE_FLAG_NOTMPSAFE. It looks like > >>>> mod_authn_socache is protecting entry to the socache provider inside a > >>>> global mutex without checking that flag - overkill but should be safe. > >>> > >>> Ah, not true, it only does it for ->store(). The ->retrieve() calls are > >>> then unsafe and racy, that's definitely a bug. > >> > >> Your proposal would be to setup check_password in mod_authn_socache in a > >> similar way > >> to ap_authn_cache_store? > >> Hence wrapping the ->retrieve in a apr_global_mutex_trylock / > >> apr_global_mutex_unlock. > > > > Yes, that is definitely necessary to use the socache API (and it's > > documented that way). > > I did create such a patch and asked the reporter to confirm.
Thank you! LGTM. > > I think the use of ctx->pool in mod_socache_dbm can be safely switched > > to the passed-in pool (r->pool in this case) in the > > store/retrieve/remove/status entry points too. > > Do you see any reason why we constantly need to get the driver via > apr_dbm_get_driver in case of a later apr-util and constantly need to > apr_dbm_open(2) / apr_dbm_close the DB? Can't we just open that once > in the beginning and keep it open thru the lifecycle of the server? Or > can only one thread have the DB open due to possible races / locking > issues? My experience from stress testing on mod_dav_fs was exactly that - using a global mutex and opening the apr_dbm_t in only one thread at a time was the only safe way to use that API. Storing the apr_dbm_driver_t in the instance_t struct might be safe, but unless you stress test it/read all the driver code I wouldn't change that part at all tbh. (I think Greg said when we talked about this, a better to answer to most of these questions is dropping apr_dbm in httpd and switching to sqlite instead, that definitely applies here too. It is crazy to lack safe concurrent read-only database access) Regards, Joe
