* [PATCH nft 1/3] parser: don't mark "string" as const
@ 2023-11-09 18:59 Thomas Haller
2023-11-09 18:59 ` [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy() Thomas Haller
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Thomas Haller @ 2023-11-09 18:59 UTC (permalink / raw)
To: NetFilter; +Cc: Thomas Haller
The "string" field is allocated, and the bison actions are expected to
take/free them. It's not const, and it should not be freed with free_const().
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
src/parser_bison.y | 148 ++++++++++++++++++++++-----------------------
1 file changed, 74 insertions(+), 74 deletions(-)
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 1e8169c44f62..ca0851c915d2 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -148,19 +148,19 @@ static bool already_set(const void *attr, const struct location *loc,
static struct expr *ifname_expr_alloc(const struct location *location,
struct list_head *queue,
- const char *name)
+ char *name)
{
unsigned int length = strlen(name);
struct expr *expr;
if (length == 0) {
- free_const(name);
+ free(name);
erec_queue(error(location, "empty interface name"), queue);
return NULL;
}
if (length >= IFNAMSIZ) {
- free_const(name);
+ free(name);
erec_queue(error(location, "interface name too long"), queue);
return NULL;
}
@@ -168,7 +168,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
expr = constant_expr_alloc(location, &ifname_type, BYTEORDER_HOST_ENDIAN,
length * BITS_PER_BYTE, name);
- free_const(name);
+ free(name);
return expr;
}
@@ -207,7 +207,7 @@ int nft_lex(void *, void *, void *);
uint64_t val;
uint32_t val32;
uint8_t val8;
- const char * string;
+ char * string;
struct list_head *list;
struct cmd *cmd;
@@ -358,7 +358,7 @@ int nft_lex(void *, void *, void *);
%token <string> STRING "string"
%token <string> QUOTED_STRING "quoted string"
%token <string> ASTERISK_STRING "string with a trailing asterisk"
-%destructor { free_const($$); } STRING QUOTED_STRING ASTERISK_STRING
+%destructor { free($$); } STRING QUOTED_STRING ASTERISK_STRING
%token LL_HDR "ll"
%token NETWORK_HDR "nh"
@@ -674,7 +674,7 @@ int nft_lex(void *, void *, void *);
%type <limit_rate> limit_rate_bytes
%type <string> identifier type_identifier string comment_spec
-%destructor { free_const($$); } identifier type_identifier string comment_spec
+%destructor { free($$); } identifier type_identifier string comment_spec
%type <val> time_spec time_spec_or_num_s quota_used
@@ -709,7 +709,7 @@ int nft_lex(void *, void *, void *);
%type <val32> int_num chain_policy
%type <prio_spec> extended_prio_spec prio_spec
%type <string> extended_prio_name quota_unit basehook_device_name
-%destructor { free_const($$); } extended_prio_name quota_unit basehook_device_name
+%destructor { free($$); } extended_prio_name quota_unit basehook_device_name
%type <expr> dev_spec
%destructor { free($$); } dev_spec
@@ -928,7 +928,7 @@ int nft_lex(void *, void *, void *);
%type <val> markup_format
%type <string> monitor_event
-%destructor { free_const($$); } monitor_event
+%destructor { free($$); } monitor_event
%type <val> monitor_object monitor_format
%type <val> synproxy_ts synproxy_sack
@@ -1053,10 +1053,10 @@ close_scope_xt : { scanner_pop_start_cond(nft->scanner, PARSER_SC_XT); }
common_block : INCLUDE QUOTED_STRING stmt_separator
{
if (scanner_include_file(nft, scanner, $2, &@$) < 0) {
- free_const($2);
+ free($2);
YYERROR;
}
- free_const($2);
+ free($2);
}
| DEFINE identifier '=' initializer_expr stmt_separator
{
@@ -1066,19 +1066,19 @@ common_block : INCLUDE QUOTED_STRING stmt_separator
erec_queue(error(&@2, "redefinition of symbol '%s'", $2),
state->msgs);
expr_free($4);
- free_const($2);
+ free($2);
YYERROR;
}
symbol_bind(scope, $2, $4);
- free_const($2);
+ free($2);
}
| REDEFINE identifier '=' initializer_expr stmt_separator
{
struct scope *scope = current_scope(state);
symbol_bind(scope, $2, $4);
- free_const($2);
+ free($2);
}
| UNDEFINE identifier stmt_separator
{
@@ -1087,10 +1087,10 @@ common_block : INCLUDE QUOTED_STRING stmt_separator
if (symbol_unbind(scope, $2) < 0) {
erec_queue(error(&@2, "undefined symbol '%s'", $2),
state->msgs);
- free_const($2);
+ free($2);
YYERROR;
}
- free_const($2);
+ free($2);
}
| error stmt_separator
{
@@ -1879,21 +1879,21 @@ table_options : FLAGS STRING
{
if (strcmp($2, "dormant") == 0) {
$<table>0->flags |= TABLE_F_DORMANT;
- free_const($2);
+ free($2);
} else if (strcmp($2, "owner") == 0) {
$<table>0->flags |= TABLE_F_OWNER;
- free_const($2);
+ free($2);
} else {
erec_queue(error(&@2, "unknown table option %s", $2),
state->msgs);
- free_const($2);
+ free($2);
YYERROR;
}
}
| comment_spec
{
if (already_set($<table>0->comment, &@$, state)) {
- free_const($1);
+ free($1);
YYERROR;
}
$<table>0->comment = $1;
@@ -2064,7 +2064,7 @@ chain_block : /* empty */ { $$ = $<chain>-1; }
| chain_block comment_spec stmt_separator
{
if (already_set($1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$1->comment = $2;
@@ -2190,7 +2190,7 @@ set_block : /* empty */ { $$ = $<set>-1; }
| set_block comment_spec stmt_separator
{
if (already_set($1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$1->comment = $2;
@@ -2307,7 +2307,7 @@ map_block : /* empty */ { $$ = $<set>-1; }
| map_block comment_spec stmt_separator
{
if (already_set($1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$1->comment = $2;
@@ -2346,10 +2346,10 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; }
if ($$->hook.name == NULL) {
erec_queue(error(&@3, "unknown chain hook"),
state->msgs);
- free_const($3);
+ free($3);
YYERROR;
}
- free_const($3);
+ free($3);
$$->priority = $4;
}
@@ -2423,12 +2423,12 @@ data_type_atom_expr : type_identifier
if (dtype == NULL) {
erec_queue(error(&@1, "unknown datatype %s", $1),
state->msgs);
- free_const($1);
+ free($1);
YYERROR;
}
$$ = constant_expr_alloc(&@1, dtype, dtype->byteorder,
dtype->size, NULL);
- free_const($1);
+ free($1);
}
| TIME
{
@@ -2465,7 +2465,7 @@ counter_block : /* empty */ { $$ = $<obj>-1; }
| counter_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2482,7 +2482,7 @@ quota_block : /* empty */ { $$ = $<obj>-1; }
| quota_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2499,7 +2499,7 @@ ct_helper_block : /* empty */ { $$ = $<obj>-1; }
| ct_helper_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2520,7 +2520,7 @@ ct_timeout_block : /*empty */
| ct_timeout_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2537,7 +2537,7 @@ ct_expect_block : /*empty */ { $$ = $<obj>-1; }
| ct_expect_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2554,7 +2554,7 @@ limit_block : /* empty */ { $$ = $<obj>-1; }
| limit_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2571,7 +2571,7 @@ secmark_block : /* empty */ { $$ = $<obj>-1; }
| secmark_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2588,7 +2588,7 @@ synproxy_block : /* empty */ { $$ = $<obj>-1; }
| synproxy_block comment_spec
{
if (already_set($<obj>1->comment, &@2, state)) {
- free_const($2);
+ free($2);
YYERROR;
}
$<obj>1->comment = $2;
@@ -2609,12 +2609,12 @@ hook_spec : TYPE close_scope_type STRING HOOK STRING dev_spec prio_spec
if (chain_type == NULL) {
erec_queue(error(&@3, "unknown chain type"),
state->msgs);
- free_const($3);
+ free($3);
YYERROR;
}
$<chain>0->type.loc = @3;
$<chain>0->type.str = xstrdup(chain_type);
- free_const($3);
+ free($3);
$<chain>0->loc = @$;
$<chain>0->hook.loc = @5;
@@ -2622,10 +2622,10 @@ hook_spec : TYPE close_scope_type STRING HOOK STRING dev_spec prio_spec
if ($<chain>0->hook.name == NULL) {
erec_queue(error(&@5, "unknown chain hook"),
state->msgs);
- free_const($5);
+ free($5);
YYERROR;
}
- free_const($5);
+ free($5);
$<chain>0->dev_expr = $6;
$<chain>0->priority = $7;
@@ -2672,7 +2672,7 @@ extended_prio_spec : int_num
BYTEORDER_HOST_ENDIAN,
strlen($1) * BITS_PER_BYTE,
$1);
- free_const($1);
+ free($1);
$$ = spec;
}
| extended_prio_name PLUS NUM
@@ -2685,7 +2685,7 @@ extended_prio_spec : int_num
BYTEORDER_HOST_ENDIAN,
strlen(str) * BITS_PER_BYTE,
str);
- free_const($1);
+ free($1);
$$ = spec;
}
| extended_prio_name DASH NUM
@@ -2698,7 +2698,7 @@ extended_prio_spec : int_num
BYTEORDER_HOST_ENDIAN,
strlen(str) * BITS_PER_BYTE,
str);
- free_const($1);
+ free($1);
$$ = spec;
}
;
@@ -2783,7 +2783,7 @@ time_spec : STRING
uint64_t res;
erec = time_parse(&@1, $1, &res);
- free_const($1);
+ free($1);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -2984,7 +2984,7 @@ comment_spec : COMMENT string
erec_queue(error(&@2, "comment too long, %d characters maximum allowed",
NFTNL_UDATA_COMMENT_MAXLEN),
state->msgs);
- free_const($2);
+ free($2);
YYERROR;
}
$$ = $2;
@@ -3085,8 +3085,8 @@ stmt : verdict_stmt
xt_stmt : XT STRING string
{
$$ = NULL;
- free_const($2);
- free_const($3);
+ free($2);
+ free($3);
erec_queue(error(&@$, "unsupported xtables compat expression, use iptables-nft with this ruleset"),
state->msgs);
YYERROR;
@@ -3244,7 +3244,7 @@ log_arg : PREFIX string
expr = constant_expr_alloc(&@$, &string_type,
BYTEORDER_HOST_ENDIAN,
(strlen($2) + 1) * BITS_PER_BYTE, $2);
- free_const($2);
+ free($2);
$<stmt>0->log.prefix = expr;
$<stmt>0->log.flags |= STMT_LOG_PREFIX;
break;
@@ -3318,7 +3318,7 @@ log_arg : PREFIX string
state->msgs);
}
expr_free(expr);
- free_const($2);
+ free($2);
YYERROR;
}
item = variable_expr_alloc(&@$, scope, sym);
@@ -3348,7 +3348,7 @@ log_arg : PREFIX string
}
}
- free_const($2);
+ free($2);
$<stmt>0->log.prefix = expr;
$<stmt>0->log.flags |= STMT_LOG_PREFIX;
}
@@ -3401,10 +3401,10 @@ level_type : string
else {
erec_queue(error(&@1, "invalid log level"),
state->msgs);
- free_const($1);
+ free($1);
YYERROR;
}
- free_const($1);
+ free($1);
}
;
@@ -3494,7 +3494,7 @@ quota_used : /* empty */ { $$ = 0; }
uint64_t rate;
erec = data_unit_parse(&@$, $3, &rate);
- free_const($3);
+ free($3);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -3509,7 +3509,7 @@ quota_stmt : QUOTA quota_mode NUM quota_unit quota_used close_scope_quota
uint64_t rate;
erec = data_unit_parse(&@$, $4, &rate);
- free_const($4);
+ free($4);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -3553,7 +3553,7 @@ limit_rate_bytes : NUM STRING
uint64_t rate, unit;
erec = rate_parse(&@$, $2, &rate, &unit);
- free_const($2);
+ free($2);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -3575,7 +3575,7 @@ limit_bytes : NUM BYTES { $$ = $1; }
uint64_t rate;
erec = data_unit_parse(&@$, $2, &rate);
- free_const($2);
+ free($2);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -3604,7 +3604,7 @@ reject_with_expr : STRING
{
$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
current_scope(state), $1);
- free_const($1);
+ free($1);
}
| integer_expr { $$ = $1; }
;
@@ -4268,12 +4268,12 @@ variable_expr : '$' identifier
erec_queue(error(&@2, "unknown identifier '%s'", $2),
state->msgs);
}
- free_const($2);
+ free($2);
YYERROR;
}
$$ = variable_expr_alloc(&@$, scope, sym);
- free_const($2);
+ free($2);
}
;
@@ -4283,7 +4283,7 @@ symbol_expr : variable_expr
$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
current_scope(state),
$1);
- free_const($1);
+ free($1);
}
;
@@ -4296,7 +4296,7 @@ set_ref_symbol_expr : AT identifier close_scope_at
$$ = symbol_expr_alloc(&@$, SYMBOL_SET,
current_scope(state),
$2);
- free_const($2);
+ free($2);
}
;
@@ -4393,10 +4393,10 @@ osf_ttl : /* empty */
else {
erec_queue(error(&@2, "invalid ttl option"),
state->msgs);
- free_const($2);
+ free($2);
YYERROR;
}
- free_const($2);
+ free($2);
}
;
@@ -4566,7 +4566,7 @@ set_elem_option : TIMEOUT time_spec
| comment_spec
{
if (already_set($<expr>0->comment, &@1, state)) {
- free_const($1);
+ free($1);
YYERROR;
}
$<expr>0->comment = $1;
@@ -4648,7 +4648,7 @@ set_elem_stmt : COUNTER close_scope_counter
uint64_t rate;
erec = data_unit_parse(&@$, $4, &rate);
- free_const($4);
+ free($4);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -4681,7 +4681,7 @@ set_elem_expr_option : TIMEOUT time_spec
| comment_spec
{
if (already_set($<expr>0->comment, &@1, state)) {
- free_const($1);
+ free($1);
YYERROR;
}
$<expr>0->comment = $1;
@@ -4733,7 +4733,7 @@ quota_config : quota_mode NUM quota_unit quota_used
uint64_t rate;
erec = data_unit_parse(&@$, $3, &rate);
- free_const($3);
+ free($3);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -4762,10 +4762,10 @@ secmark_config : string
ret = snprintf(secmark->ctx, sizeof(secmark->ctx), "%s", $1);
if (ret <= 0 || ret >= (int)sizeof(secmark->ctx)) {
erec_queue(error(&@1, "invalid context '%s', max length is %u\n", $1, (int)sizeof(secmark->ctx)), state->msgs);
- free_const($1);
+ free($1);
YYERROR;
}
- free_const($1);
+ free($1);
}
;
@@ -4802,7 +4802,7 @@ ct_helper_config : TYPE QUOTED_STRING PROTOCOL ct_l4protoname stmt_separator cl
erec_queue(error(&@2, "invalid name '%s', max length is %u\n", $2, (int)sizeof(ct->name)), state->msgs);
YYERROR;
}
- free_const($2);
+ free($2);
ct->l4proto = $4;
}
@@ -5197,7 +5197,7 @@ chain_expr : variable_expr
BYTEORDER_HOST_ENDIAN,
strlen($1) * BITS_PER_BYTE,
$1);
- free_const($1);
+ free($1);
}
;
@@ -5215,7 +5215,7 @@ meta_expr : META meta_key close_scope_meta
unsigned int key;
erec = meta_key_parse(&@$, $2, &key);
- free_const($2);
+ free($2);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -5292,7 +5292,7 @@ meta_stmt : META meta_key SET stmt_expr close_scope_meta
unsigned int key;
erec = meta_key_parse(&@$, $2, &key);
- free_const($2);
+ free($2);
if (erec != NULL) {
erec_queue(erec, state->msgs);
YYERROR;
@@ -5603,10 +5603,10 @@ payload_base_spec : LL_HDR { $$ = PROTO_BASE_LL_HDR; }
$$ = PROTO_BASE_INNER_HDR;
} else {
erec_queue(error(&@1, "unknown raw payload base"), state->msgs);
- free_const($1);
+ free($1);
YYERROR;
}
- free_const($1);
+ free($1);
}
;
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy()
2023-11-09 18:59 [PATCH nft 1/3] parser: don't mark "string" as const Thomas Haller
@ 2023-11-09 18:59 ` Thomas Haller
2023-11-15 9:42 ` Florian Westphal
2023-11-09 18:59 ` [PATCH nft 3/3] parser: use size_t type for strlen() results Thomas Haller
2023-11-09 19:22 ` [PATCH nft 1/3] parser: don't mark "string" as const Pablo Neira Ayuso
2 siblings, 1 reply; 9+ messages in thread
From: Thomas Haller @ 2023-11-09 18:59 UTC (permalink / raw)
To: NetFilter; +Cc: Thomas Haller
It's not a const pointer, as the destroy() function clearly
modifies/free is. Drop the const from the argument of
input_descriptor_destroy().
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
src/scanner.l | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/scanner.l b/src/scanner.l
index 00a09485d420..31284d7358fa 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -1258,11 +1258,11 @@ void *scanner_init(struct parser_state *state)
return scanner;
}
-static void input_descriptor_destroy(const struct input_descriptor *indesc)
+static void input_descriptor_destroy(struct input_descriptor *indesc)
{
if (indesc->name)
free_const(indesc->name);
- free_const(indesc);
+ free(indesc);
}
static void input_descriptor_list_destroy(struct parser_state *state)
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH nft 3/3] parser: use size_t type for strlen() results
2023-11-09 18:59 [PATCH nft 1/3] parser: don't mark "string" as const Thomas Haller
2023-11-09 18:59 ` [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy() Thomas Haller
@ 2023-11-09 18:59 ` Thomas Haller
2023-11-15 9:41 ` Florian Westphal
2023-11-09 19:22 ` [PATCH nft 1/3] parser: don't mark "string" as const Pablo Neira Ayuso
2 siblings, 1 reply; 9+ messages in thread
From: Thomas Haller @ 2023-11-09 18:59 UTC (permalink / raw)
To: NetFilter; +Cc: Thomas Haller
strlen() has a "size_t" as result. Use the correct type.
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
src/parser_bison.y | 2 +-
src/scanner.l | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/parser_bison.y b/src/parser_bison.y
index ca0851c915d2..12031c831353 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -150,7 +150,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
struct list_head *queue,
char *name)
{
- unsigned int length = strlen(name);
+ size_t length = strlen(name);
struct expr *expr;
if (length == 0) {
diff --git a/src/scanner.l b/src/scanner.l
index 31284d7358fa..aba8ef1c6b54 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -1117,7 +1117,7 @@ static int include_glob(struct nft_ctx *nft, void *scanner, const char *pattern,
ret = glob(pattern, flags, NULL, &glob_data);
if (ret == 0) {
char *path;
- int len;
+ size_t len;
/* reverse alphabetical order due to stack */
for (i = glob_data.gl_pathc; i > 0; i--) {
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH nft 1/3] parser: don't mark "string" as const
2023-11-09 18:59 [PATCH nft 1/3] parser: don't mark "string" as const Thomas Haller
2023-11-09 18:59 ` [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy() Thomas Haller
2023-11-09 18:59 ` [PATCH nft 3/3] parser: use size_t type for strlen() results Thomas Haller
@ 2023-11-09 19:22 ` Pablo Neira Ayuso
2023-11-09 20:34 ` Thomas Haller
2 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2023-11-09 19:22 UTC (permalink / raw)
To: Thomas Haller; +Cc: NetFilter
On Thu, Nov 09, 2023 at 07:59:47PM +0100, Thomas Haller wrote:
> The "string" field is allocated, and the bison actions are expected to
> take/free them. It's not const, and it should not be freed with free_const().
This ifname_expr_alloc() function does not modify the 'name' argument,
this is really const.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nft 1/3] parser: don't mark "string" as const
2023-11-09 19:22 ` [PATCH nft 1/3] parser: don't mark "string" as const Pablo Neira Ayuso
@ 2023-11-09 20:34 ` Thomas Haller
2023-11-09 23:10 ` Florian Westphal
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Haller @ 2023-11-09 20:34 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: NetFilter
On Thu, 2023-11-09 at 20:22 +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 09, 2023 at 07:59:47PM +0100, Thomas Haller wrote:
> > The "string" field is allocated, and the bison actions are expected
> > to
> > take/free them. It's not const, and it should not be freed with
> > free_const().
>
> This ifname_expr_alloc() function does not modify the 'name'
> argument,
> this is really const.
ifname_expr_alloc() calls `free(name)`, destroying the thing that is
pointed at. That's a modification.
The code required to cast away the constness, which also indicates that
it is not actually const.
Thomas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nft 1/3] parser: don't mark "string" as const
2023-11-09 20:34 ` Thomas Haller
@ 2023-11-09 23:10 ` Florian Westphal
2023-11-10 8:09 ` Thomas Haller
0 siblings, 1 reply; 9+ messages in thread
From: Florian Westphal @ 2023-11-09 23:10 UTC (permalink / raw)
To: Thomas Haller; +Cc: Pablo Neira Ayuso, NetFilter
Thomas Haller <thaller@redhat.com> wrote:
> ifname_expr_alloc() calls `free(name)`, destroying the thing that is
> pointed at. That's a modification.
>
> The code required to cast away the constness, which also indicates that
> it is not actually const.
It it. I hate that free() isn't const void *.
I prefer to have const used as much as humanly possible.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nft 1/3] parser: don't mark "string" as const
2023-11-09 23:10 ` Florian Westphal
@ 2023-11-10 8:09 ` Thomas Haller
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Haller @ 2023-11-10 8:09 UTC (permalink / raw)
To: Florian Westphal; +Cc: Pablo Neira Ayuso, NetFilter
On Fri, 2023-11-10 at 00:10 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > ifname_expr_alloc() calls `free(name)`, destroying the thing that
> > is
> > pointed at. That's a modification.
> >
> > The code required to cast away the constness, which also indicates
> > that
> > it is not actually const.
>
> It it. I hate that free() isn't const void *.
That would work too. But C choose instead to disallow that.
Note that in nftables, ~75% of the free() operate on non-const just
fine. And the compiler is helpful confirming that.
>
> I prefer to have const used as much as humanly possible.
>
Definitely. Especially for long-lived data.
The "string" in question is a short-lived data allocated by "scanner.l"
and handed over to "parser_bison.y".
See:
»·······»·······»·······|»······rule_alloc»·····comment_spec
»·······»·······»·······{
»·······»·······»·······»·······$$->cOmment = $2;
»·······»·······»·······}
Here no free() is involved, but ownership is passed on without
strdup(). If "string" is const, this is another ugliness where you pass
on a string that you seemingly don't own.
There are places where it's ugly and const-correctness breaks down
(during destruction/ref/unref). See free_const() and
datatype_get()/datatype_free(). But those places should be at a minimum
by choosing const (and free_const()) when it makes sense.
Thomas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nft 3/3] parser: use size_t type for strlen() results
2023-11-09 18:59 ` [PATCH nft 3/3] parser: use size_t type for strlen() results Thomas Haller
@ 2023-11-15 9:41 ` Florian Westphal
0 siblings, 0 replies; 9+ messages in thread
From: Florian Westphal @ 2023-11-15 9:41 UTC (permalink / raw)
To: Thomas Haller; +Cc: NetFilter
Thomas Haller <thaller@redhat.com> wrote:
> strlen() has a "size_t" as result. Use the correct type.
I've applied this one, but seriously, seems quite a bit of
bikeshedding.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy()
2023-11-09 18:59 ` [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy() Thomas Haller
@ 2023-11-15 9:42 ` Florian Westphal
0 siblings, 0 replies; 9+ messages in thread
From: Florian Westphal @ 2023-11-15 9:42 UTC (permalink / raw)
To: Thomas Haller; +Cc: NetFilter
Thomas Haller <thaller@redhat.com> wrote:
> It's not a const pointer, as the destroy() function clearly
> modifies/free is. Drop the const from the argument of
> input_descriptor_destroy().
>
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
> src/scanner.l | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/scanner.l b/src/scanner.l
> index 00a09485d420..31284d7358fa 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -1258,11 +1258,11 @@ void *scanner_init(struct parser_state *state)
> return scanner;
> }
>
> -static void input_descriptor_destroy(const struct input_descriptor *indesc)
> +static void input_descriptor_destroy(struct input_descriptor *indesc)
> {
> if (indesc->name)
> free_const(indesc->name);
> - free_const(indesc);
> + free(indesc);
I don't agree, this is fine as-is.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-15 9:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-09 18:59 [PATCH nft 1/3] parser: don't mark "string" as const Thomas Haller
2023-11-09 18:59 ` [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy() Thomas Haller
2023-11-15 9:42 ` Florian Westphal
2023-11-09 18:59 ` [PATCH nft 3/3] parser: use size_t type for strlen() results Thomas Haller
2023-11-15 9:41 ` Florian Westphal
2023-11-09 19:22 ` [PATCH nft 1/3] parser: don't mark "string" as const Pablo Neira Ayuso
2023-11-09 20:34 ` Thomas Haller
2023-11-09 23:10 ` Florian Westphal
2023-11-10 8:09 ` Thomas Haller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).