Re: [PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters
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
Re: [PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters
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 >From e47065ac6cc7478d21cfa00c5c45a0ae7cd412bf Mon Sep 17 00:00:00 2001 From: Daniel Corbett Date: Sun, 27 May 2018 09:47:12 -0400 Subject: [PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters 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 and reworked the end logic to ensure that ref_cnt is always decremented after use. This should be backported to 1.8 --- src/stick_table.c | 170 +++--- 1 file changed, 86 insertions(+), 84 deletions(-) diff --git a/src/stick_table.c b/src/stick_table.c index fcc6fe6..101a4e2 100644 --- a/src/stick_table.c +++ b/src/stick_table.c @@ -875,6 +875,7 @@ static int sample_conv_in_table(const struct arg *arg_p, struct sample *smp, voi smp->data.type = SMP_T_BOOL; smp->data.u.sint = !!ts; smp->flags = SMP_F_VOL_TEST; + stktable_release(t, ts); return 1; } @@ -907,12 +908,12 @@ static int sample_conv_table_bytes_in_rate(const struct arg *arg_p, struct sampl return 1; ptr = stktable_data_ptr(t, ts, STKTABLE_DT_BYTES_IN_RATE); - if (!ptr) - return 0; /* parameter not stored */ + if (ptr) + smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, bytes_in_rate), + t->data_arg[STKTABLE_DT_BYTES_IN_RATE].u); - smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, bytes_in_rate), - t->data_arg[STKTABLE_DT_BYTES_IN_RATE].u); - return 1; + stktable_release(t, ts); + return !!ptr; } /* Casts sample to the type of the table specified in arg(0), and looks @@ -944,11 +945,11 @@ static int sample_conv_table_conn_cnt(const struct arg *arg_p, struct sample *sm return 1; ptr = stktable_data_ptr(t, ts, STKTABLE_DT_CONN_CNT); - if (!ptr) - return 0; /* parameter not stored */ + if (ptr) + smp->data.u.sint = stktable_data_cast(ptr, conn_cnt); - smp->data.u.sint = stktable_data_cast(ptr, conn_cnt); - return 1; + stktable_release(t, ts); + return !!ptr; } /* Casts sample to the type of the table specified in arg(0), and looks @@ -980,11 +981,11 @@ static int sample_conv_table_conn_cur(const struct arg *arg_p, struct sample *sm return 1; ptr = stktable_data_ptr(t, ts, STKTABLE_DT_CONN_CUR); - if (!ptr) - return 0; /* parameter not stored */ + if (ptr) + smp->data.u.sint = stktable_data_cast(ptr, conn_cur); - smp->data.u.sint = stktable_data_cast(ptr, conn_cur); - return 1; + stktable_release(t, ts); + return !!ptr; } /* Casts sample to the type of the table specified in arg(0), and looks @@ -1016,12 +1017,12 @@ static int sample_conv_table_conn_rate(const struct arg *arg_p, struct sample *s return 1; ptr = stktable_data_ptr(t, ts, STKTABLE_DT_CONN_RATE); - if (!ptr) - return 0; /* parameter not stored */ + if (ptr) + smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, conn_rate), + t->data_arg[STKTABLE_DT_CONN_RATE].u); - smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, conn_rate), - t->data_arg[STKTABLE_DT_CONN_RATE].u); - return 1; + stktable_release(t, ts); + return !!ptr; } /* Casts sample to the type of the table specified in arg(0), and looks @@ -1053,12 +1054,12 @@ static int sample_conv_table_bytes_out_rate(const struct arg *arg_p, struct samp return 1; ptr = stktable_data_ptr(t, ts, STKTABLE_DT_BYTES_OUT_RATE); - if (!ptr) - return 0; /* parameter not stored */ + if (ptr) + smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, bytes_out_rate), + t->data_arg[STKTABLE_DT_BYTES_OUT_RATE].u); - smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, bytes_out_
Re: [PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters
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
[PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters
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 >From 28530921746e62bb229880774a311bfebfcf7579 Mon Sep 17 00:00:00 2001 From: Daniel Corbett Date: Thu, 17 May 2018 13:17:54 -0400 Subject: [PATCH] BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters 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 --- src/stick_table.c | 19 +++ 1 file changed, 19 insertions(+) diff --git a/src/stick_table.c b/src/stick_table.c index 3e44747..f1ad347 100644 --- a/src/stick_table.c +++ b/src/stick_table.c @@ -912,6 +912,7 @@ static int sample_conv_table_bytes_in_rate(const struct arg *arg_p, struct sampl smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, bytes_in_rate), t->data_arg[STKTABLE_DT_BYTES_IN_RATE].u); + stktable_release(t, ts); return 1; } @@ -948,6 +949,7 @@ static int sample_conv_table_conn_cnt(const struct arg *arg_p, struct sample *sm return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, conn_cnt); + stktable_release(t, ts); return 1; } @@ -984,6 +986,7 @@ static int sample_conv_table_conn_cur(const struct arg *arg_p, struct sample *sm return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, conn_cur); + stktable_release(t, ts); return 1; } @@ -1021,6 +1024,7 @@ static int sample_conv_table_conn_rate(const struct arg *arg_p, struct sample *s smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, conn_rate), t->data_arg[STKTABLE_DT_CONN_RATE].u); + stktable_release(t, ts); return 1; } @@ -1058,6 +1062,7 @@ static int sample_conv_table_bytes_out_rate(const struct arg *arg_p, struct samp smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, bytes_out_rate), t->data_arg[STKTABLE_DT_BYTES_OUT_RATE].u); + stktable_release(t, ts); return 1; } @@ -1094,6 +1099,7 @@ static int sample_conv_table_gpt0(const struct arg *arg_p, struct sample *smp, v return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, gpt0); + stktable_release(t, ts); return 1; } @@ -1130,6 +1136,7 @@ static int sample_conv_table_gpc0(const struct arg *arg_p, struct sample *smp, v return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, gpc0); + stktable_release(t, ts); return 1; } @@ -1167,6 +1174,7 @@ static int sample_conv_table_gpc0_rate(const struct arg *arg_p, struct sample *s smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, gpc0_rate), t->data_arg[STKTABLE_DT_GPC0_RATE].u); + stktable_release(t, ts); return 1; } @@ -1203,6 +1211,7 @@ static int sample_conv_table_gpc1(const struct arg *arg_p, struct sample *smp, v return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, gpc1); + stktable_release(t, ts); return 1; } @@ -1240,6 +1249,7 @@ static int sample_conv_table_gpc1_rate(const struct arg *arg_p, struct sample *s smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, gpc1_rate), t->data_arg[STKTABLE_DT_GPC1_RATE].u); + stktable_release(t, ts); return 1; } @@ -1276,6 +1286,7 @@ static int sample_conv_table_http_err_cnt(const struct arg *arg_p, struct sample return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, http_err_cnt); + stktable_release(t, ts); return 1; } @@ -1313,6 +1324,7 @@ static int sample_conv_table_http_err_rate(const struct arg *arg_p, struct sampl smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, http_err_rate), t->data_arg[STKTABLE_DT_HTTP_ERR_RATE].u); + stktable_release(t, ts); return 1; } @@ -1349,6 +1361,7 @@ static int sample_conv_table_http_req_cnt(const struct arg *arg_p, struct sample return 0; /* parameter not stored */ smp->data.u.sint = stktable_data_cast(ptr, http_req_cnt); + stktable_release(t, ts); return 1; } @@ -1386,6 +1399,7 @@ static int sample_conv_table_http_req_rate(const struct arg *arg_p, struct sampl smp->data.u.sint = read_freq_ctr_period(&stktable_data_cast(ptr, http_req_rate),
