OK, I think I figured it out. PRIV_CALL is a call site specific storage. I ended up figuring this out by looking at the compiled VCL output from varnishd.
For headers there should probably be documentation to not use a variable argument for the pattern. In vmod_bodyaccess the same thing (no variables for patterns) but it should also lock around the VRE_compile, and the free function is incorrect and should be VRE_free, or it leaks memory during race conditions. On Fri, Apr 27, 2018 at 2:06 AM, Stephen J. Butler <stephen.but...@gmail.com > wrote: > (sorry, never mind about the -- and ++; they're all inside the lock) > > On Fri, Apr 27, 2018 at 2:05 AM, Stephen J. Butler < > stephen.but...@gmail.com> wrote: > >> In the varnish source code for the 6.0 release, I found one use of >> PRIV_CALL in vmod_std, vmod_fileread(). This function does have locks, but >> they're all to protect modifications of the global frlist variable. It does >> seem like the priv parameter is used as a call site value but no care is >> taken to make sure access is exclusive. >> >> In that case it also bothers me that there's an assumption that -- and ++ >> are atomic operations on all platforms and compilers... some quick Googling >> suggests that's not the case. >> >> >> On Fri, Apr 27, 2018 at 1:49 AM, Stephen J. Butler < >> stephen.but...@gmail.com> wrote: >> >>> Sorry, didn't reply to the list: >>> >>> On Fri, Apr 27, 2018 at 1:48 AM, Stephen J. Butler < >>> stephen.but...@gmail.com> 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 < >>>> guilla...@varnish-software.com> 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 < >>>>> stephen.but...@gmail.com> 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/v >>>>>> mod_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 >>>>>> varnish-dev@varnish-cache.org >>>>>> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev >>>>>> >>>>> >>>>> >>>> >>> >> >
_______________________________________________ varnish-dev mailing list varnish-dev@varnish-cache.org https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev