Welcome! Log In Create A New Profile

Advanced

[PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters

Posted by Daniel Corbett 
Hello,

When using table_* converters ref_cnt was incremented
and never decremented causing entries to not expire.

The root cause appears to be that stktable_lookup_key()
was called within all sample_conv_table_* functions which was
incrementing ref_cnt and not decrementing after completion.

Added stktable_release() to the end of each sample_conv_table_*
function.

This should be backported to 1.8.


Thanks,
-- Daniel
Hi Daniel,

On Thu, May 17, 2018 at 02:05:28PM -0400, Daniel Corbett wrote:
> Hello,
>
> When using table_* converters ref_cnt was incremented
> and never decremented causing entries to not expire.
>
> The root cause appears to be that stktable_lookup_key()
> was called within all sample_conv_table_* functions which was
> incrementing ref_cnt and not decrementing after completion.
>
> Added stktable_release() to the end of each sample_conv_table_*
> function.

Interesting one! However it's not correct, it doesn't decrement the
refcount on the return 0 path so the problem remains when a data
type is looked up in a table where it is not stored. For example :

ts = stktable_lookup_key(t, key);

## ref_cnt incremented here only if ts != NULL

smp->flags = SMP_F_VOL_TEST;
smp->data.type = SMP_T_SINT;
smp->data.u.sint = 0;

if (!ts) /* key not present */
return 1;

ptr = stktable_data_ptr(t, ts, STKTABLE_DT_CONN_CUR);
if (!ptr)

## here it's not decremented

return 0; /* parameter not stored */

smp->data.u.sint = stktable_data_cast(ptr, conn_cur);
+ stktable_release(t, ts);
return 1;

Given that all functions seem to be written the same way, I suggest
that we change the end and invert the !ptr condition to centralize
the release call. It would give this above :

ptr = stktable_data_ptr(t, ts, STKTABLE_DT_CONN_CUR);
if (ptr)
smp->data.u.sint = stktable_data_cast(ptr, conn_cur);
stktable_release(t, ts);
return !!ptr;

Could you please rework your patch to do this so that I can merge it ?

Thanks!
Willy
Hello Willy,


On 05/24/2018 04:16 PM, Willy Tarreau wrote:
>
> Interesting one! However it's not correct, it doesn't decrement the
> refcount on the return 0 path so the problem remains when a data
> type is looked up in a table where it is not stored. For example :

Nice catch.  Thanks for pointing this out.

> Given that all functions seem to be written the same way, I suggest
> that we change the end and invert the !ptr condition to centralize
> the release call. It would give this above :
>
> ptr = stktable_data_ptr(t, ts, STKTABLE_DT_CONN_CUR);
> if (ptr)
> smp->data.u.sint = stktable_data_cast(ptr, conn_cur);
> stktable_release(t, ts);
> return !!ptr;
>
> Could you please rework your patch to do this so that I can merge it ?
>

I have attached the latest patch with these changes.

Thanks,
-- Daniel
Hi Daniel,

On Sun, May 27, 2018 at 12:31:54PM -0400, Daniel Corbett wrote:
> I have attached the latest patch with these changes.

Now merged, thank you!

willy
Sorry, only registered users may post in this forum.

Click here to login