Sorry, didn't reply to the list: On Fri, Apr 27, 2018 at 1:48 AM, Stephen J. Butler <[email protected] > wrote:
> Hmm. If PRIV_CALL is private to the call site, then I think > in vmod_bodyaccess.c, vmod_rematch_req_body()there's a problem. In that > case it calls VRE_compile()to initiate a regex without a lock on a > PRIV_CALL structure. > > On Fri, Apr 27, 2018 at 1:37 AM, Guillaume Quintard < > [email protected]> wrote: > >> Hi, and welcome! >> >> In this case, priv comes from PRIV_CALL, meaning it's going to be the >> same for all the request/vcl that call the function from the same spot. >> This is done to cache the regex, so you only compile for the first call. >> >> However, I'm thinking that you may have the same priv with a different >> string, and then everything goes to hell. Could anyone familiar with >> PRIV_CALL confirm? >> >> Cheers, >> >> -- >> Guillaume Quintard >> >> On Fri, Apr 27, 2018 at 8:05 AM, Stephen J. Butler < >> [email protected]> wrote: >> >>> I'm a new developer/user for Varnish, and was looking to extend a module >>> with some regex support (vmod_cookie if you must know). I don't know >>> anything about vmod development or Varnish threading, so to get an idea of >>> how to do this I looked at the vmod_header module. >>> >>> But I'm confused about why they thought they needed a mutex to protected >>> a call to VRT_re_init. Is this just a useless, historic thing that got left >>> in for some reason? >>> >>> https://github.com/varnish/varnish-modules/blob/master/src/vmod_header.c >>> >>> If you look at vmod_remove it has a 2nd parameter that's in the vcc as >>> PRIV_CALL. This parameter is initialized with the string value from the >>> last parameter (a regex pattern). It does this by calling >>> header_init_re(). And if you look at that function you see: >>> >>> /* >>> * Initialize the regex *s on priv, if it hasn't already been done. >>> * XXX: We have to recheck the condition after grabbing the lock to avoid >>> a >>> * XXX: race condition. >>> */ >>> static void >>> header_init_re(struct vmod_priv *priv, const char *s) >>> { >>> if (priv->priv == NULL) { >>> assert(pthread_mutex_lock(&header_mutex) == 0); >>> if (priv->priv == NULL) { >>> VRT_re_init(&priv->priv, s); >>> priv->free = VRT_re_fini; >>> } >>> pthread_mutex_unlock(&header_mutex); >>> } >>> } >>> My reading of the docs makes me think that this "priv" only lives for >>> each single call of the function. There should be no reason to protect it >>> with a global mutex. Do the internals of VRT_re_init() require this? I >>> can't imagine how, because if it did then this method of protecting it is >>> broken anyway. >>> >>> I suspect this is leftover from some rewrite or updating of the module >>> but wanted to check. >>> >>> >>> _______________________________________________ >>> varnish-dev mailing list >>> [email protected] >>> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev >>> >> >> >
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
