(sorry, never mind about the -- and ++; they're all inside the lock) On Fri, Apr 27, 2018 at 2:05 AM, Stephen J. Butler <[email protected] > 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 < > [email protected]> wrote: > >> 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/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 >>>>> [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
