PRIV_CALL, VRT_re_init, and mutexes

[Stephen J. Butler]

Sorry, didn't reply to the list:

On Fri, Apr 27, 2018 at 1:48 AM, Stephen J. Butler <stephen.butler at 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 <
> guillaume at 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.butler at 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/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
>>> varnish-dev at varnish-cache.org
>>> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20180427/020e5cb4/attachment-0001.html>