PRIV_CALL, VRT_re_init, and mutexes

[Stephen J. Butler]

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.butler at 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.butler at 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.butler at gmail.com> wrote:
>>
>>> 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/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 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/1e63cda4/attachment-0001.html>