obj: fix memleak in parser_bison.y

Each object (secmark, synproxy, quota, limit, counter) is dynamically allocated
by the parser and not freed at exit.
However, there is no need to use dynamic allocation here because struct obj
already provides the required storage. Update the grammar to ensure that
obj_alloc() is called before config occurs.

This fixes the following memleak (secmark as example):

 # valgrind --leak-check=full nft add secmark inet raw ssh \"system_u:object_r:ssh_server_packet_t:s0\"
 ==14643== Memcheck, a memory error detector
 ==14643== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
 ==14643== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
 ==14643== Command: nft add secmark inet raw ssh "system_u:object_r:ssh_server_packet_t:s0"
 ==14643==
 ==14643==
 ==14643== HEAP SUMMARY:
 ==14643==     in use at exit: 256 bytes in 1 blocks
 ==14643==   total heap usage: 41 allocs, 40 frees, 207,809 bytes allocated
 ==14643==
 ==14643== 256 bytes in 1 blocks are definitely lost in loss record 1 of 1
 ==14643==    at 0x4C29EA3: malloc (vg_replace_malloc.c:309)
 ==14643==    by 0x4E72074: xmalloc (utils.c:36)
 ==14643==    by 0x4E72074: xzalloc (utils.c:65)
 ==14643==    by 0x4E89A31: nft_parse (parser_bison.y:3706)
 ==14643==    by 0x4E778E7: nft_parse_bison_buffer (libnftables.c:375)
 ==14643==    by 0x4E778E7: nft_run_cmd_from_buffer (libnftables.c:443)
 ==14643==    by 0x40170F: main (main.c:326)

Fixes: f44ab88b1088e ("src: add synproxy stateful object support")
Fixes: 3bc84e5c1fdd1 ("src: add support for setting secmark")
Fixes: c0697eabe832d ("src: add stateful object support for limit")
Fixes: 4d38878b39be4 ("src: add/create/delete stateful objects")
Signed-off-by: Eric Jallot <ejallot@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/src/parser_bison.y b/src/parser_bison.y
index cd249c82d93822210b76b60552f44ef35092707c..1e2b30015f78193b4fc781f42121bb57b9f9a040 100644 (file)
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -149,12 +149,7 @@ int nft_lex(void *, void *, void *);
        struct set              *set;
        struct obj              *obj;
        struct flowtable        *flowtable;
-       struct counter          *counter;
-       struct quota            *quota;
-       struct secmark          *secmark;
-       struct synproxy         *synproxy;
        struct ct               *ct;
-       struct limit            *limit;
        const struct datatype   *datatype;
        struct handle_spec      handle_spec;
        struct position_spec    position_spec;
@@ -782,16 +777,6 @@ int nft_lex(void *, void *, void *);
 %destructor { xfree($$); }     monitor_event
 %type <val>                    monitor_object  monitor_format
 
-%type <counter>                        counter_config
-%destructor { xfree($$); }     counter_config
-%type <quota>                  quota_config
-%destructor { xfree($$); }     quota_config
-%type <limit>                  limit_config
-%destructor { xfree($$); }     limit_config
-%type <secmark>                        secmark_config
-%destructor { xfree($$); }     secmark_config
-%type <synproxy>               synproxy_config
-%destructor { xfree($$); }     synproxy_config
 %type <val>                    synproxy_ts     synproxy_sack
 
 %type <expr>                   tcp_hdr_expr
@@ -989,17 +974,16 @@ add_cmd                   :       TABLE           table_spec
                                handle_merge(&obj->handle, &$2);
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_COUNTER, &$2, &@$, obj);
                        }
-                       |       COUNTER         obj_spec        counter_obj
+                       |       COUNTER         obj_spec        counter_obj     counter_config
                        {
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_COUNTER, &$2, &@$, $3);
                        }
-                       |       QUOTA           obj_spec        quota_obj
+                       |       QUOTA           obj_spec        quota_obj       quota_config
                        {
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_QUOTA, &$2, &@$, $3);
                        }
                        |       CT      HELPER  obj_spec        ct_obj_alloc    '{' ct_helper_block '}'
                        {
-
                                $$ = cmd_alloc_obj_ct(CMD_ADD, NFT_OBJECT_CT_HELPER, &$3, &@$, $4);
                        }
                        |       CT      TIMEOUT obj_spec        ct_obj_alloc    '{' ct_timeout_block '}'
@@ -1010,15 +994,15 @@ add_cmd                  :       TABLE           table_spec
                        {
                                $$ = cmd_alloc_obj_ct(CMD_ADD, NFT_OBJECT_CT_EXPECT, &$3, &@$, $4);
                        }
-                       |       LIMIT           obj_spec        limit_obj
+                       |       LIMIT           obj_spec        limit_obj       limit_config
                        {
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_LIMIT, &$2, &@$, $3);
                        }
-                       |       SECMARK         obj_spec        secmark_obj
+                       |       SECMARK         obj_spec        secmark_obj     secmark_config
                        {
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_SECMARK, &$2, &@$, $3);
                        }
-                       |       SYNPROXY        obj_spec        synproxy_obj
+                       |       SYNPROXY        obj_spec        synproxy_obj    synproxy_config
                        {
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_SYNPROXY, &$2, &@$, $3);
                        }
@@ -1087,11 +1071,11 @@ create_cmd              :       TABLE           table_spec
                                handle_merge(&obj->handle, &$2);
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_COUNTER, &$2, &@$, obj);
                        }
-                       |       COUNTER         obj_spec        counter_obj
+                       |       COUNTER         obj_spec        counter_obj     counter_config
                        {
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_COUNTER, &$2, &@$, $3);
                        }
-                       |       QUOTA           obj_spec        quota_obj
+                       |       QUOTA           obj_spec        quota_obj       quota_config
                        {
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_QUOTA, &$2, &@$, $3);
                        }
@@ -1107,15 +1091,15 @@ create_cmd              :       TABLE           table_spec
                        {
                                $$ = cmd_alloc_obj_ct(CMD_CREATE, NFT_OBJECT_CT_EXPECT, &$3, &@$, $4);
                        }
-                       |       LIMIT           obj_spec        limit_obj
+                       |       LIMIT           obj_spec        limit_obj       limit_config
                        {
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_LIMIT, &$2, &@$, $3);
                        }
-                       |       SECMARK         obj_spec        secmark_obj
+                       |       SECMARK         obj_spec        secmark_obj     secmark_config
                        {
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_SECMARK, &$2, &@$, $3);
                        }
-                       |       SYNPROXY        obj_spec        synproxy_obj
+                       |       SYNPROXY        obj_spec        synproxy_obj    synproxy_config
                        {
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_SYNPROXY, &$2, &@$, $3);
                        }
@@ -1911,7 +1895,6 @@ counter_block             :       /* empty */     { $$ = $<obj>-1; }
                        |       counter_block     stmt_separator
                        |       counter_block     counter_config
                        {
-                               $1->counter = *$2;
                                $$ = $1;
                        }
                        ;
@@ -1921,7 +1904,6 @@ quota_block               :       /* empty */     { $$ = $<obj>-1; }
                        |       quota_block     stmt_separator
                        |       quota_block     quota_config
                        {
-                               $1->quota = *$2;
                                $$ = $1;
                        }
                        ;
@@ -1958,7 +1940,6 @@ limit_block               :       /* empty */     { $$ = $<obj>-1; }
                        |       limit_block     stmt_separator
                        |       limit_block     limit_config
                        {
-                               $1->limit = *$2;
                                $$ = $1;
                        }
                        ;
@@ -1968,7 +1949,6 @@ secmark_block             :       /* empty */     { $$ = $<obj>-1; }
                        |       secmark_block     stmt_separator
                        |       secmark_block     secmark_config
                        {
-                               $1->secmark = *$2;
                                $$ = $1;
                        }
                        ;
@@ -1978,7 +1958,6 @@ synproxy_block            :       /* empty */     { $$ = $<obj>-1; }
                        |       synproxy_block  stmt_separator
                        |       synproxy_block  synproxy_config
                        {
-                               $1->synproxy = *$2;
                                $$ = $1;
                        }
                        ;
@@ -2883,7 +2862,7 @@ synproxy_config           :       MSS     NUM     WSCALE  NUM     synproxy_ts     synproxy_sack
                                struct synproxy *synproxy;
                                uint32_t flags = 0;
 
-                               synproxy = xzalloc(sizeof(*synproxy));
+                               synproxy = &$<obj>0->synproxy;
                                synproxy->mss = $2;
                                flags |= NF_SYNPROXY_OPT_MSS;
                                synproxy->wscale = $4;
@@ -2893,14 +2872,13 @@ synproxy_config         :       MSS     NUM     WSCALE  NUM     synproxy_ts     synproxy_sack
                                if ($6)
                                        flags |= $6;
                                synproxy->flags = flags;
-                               $$ = synproxy;
                        }
                        |       MSS     NUM     stmt_separator  WSCALE  NUM     stmt_separator  synproxy_ts     synproxy_sack
                        {
                                struct synproxy *synproxy;
                                uint32_t flags = 0;
 
-                               synproxy = xzalloc(sizeof(*synproxy));
+                               synproxy = &$<obj>0->synproxy;
                                synproxy->mss = $2;
                                flags |= NF_SYNPROXY_OPT_MSS;
                                synproxy->wscale = $5;
@@ -2910,15 +2888,13 @@ synproxy_config         :       MSS     NUM     WSCALE  NUM     synproxy_ts     synproxy_sack
                                if ($8)
                                        flags |= $8;
                                synproxy->flags = flags;
-                               $$ = synproxy;
                        }
                        ;
 
-synproxy_obj           :       synproxy_config
+synproxy_obj           :       /* empty */
                        {
                                $$ = obj_alloc(&@$);
                                $$->type = NFT_OBJECT_SYNPROXY;
-                               $$->synproxy = *$1;
                        }
                        ;
 
@@ -3655,18 +3631,16 @@ counter_config          :       PACKETS         NUM     BYTES   NUM
                        {
                                struct counter *counter;
 
-                               counter = xzalloc(sizeof(*counter));
+                               counter = &$<obj>0->counter;
                                counter->packets = $2;
                                counter->bytes = $4;
-                               $$ = counter;
                        }
                        ;
 
-counter_obj            :       counter_config
+counter_obj            :       /* empty */
                        {
                                $$ = obj_alloc(&@$);
                                $$->type = NFT_OBJECT_COUNTER;
-                               $$->counter = *$1;
                        }
                        ;
 
@@ -3683,19 +3657,17 @@ quota_config            :       quota_mode NUM quota_unit quota_used
                                        YYERROR;
                                }
 
-                               quota = xzalloc(sizeof(*quota));
+                               quota = &$<obj>0->quota;
                                quota->bytes    = $2 * rate;
                                quota->used     = $4;
                                quota->flags    = $1;
-                               $$ = quota;
                        }
                        ;
 
-quota_obj              :       quota_config
+quota_obj              :       /* empty */
                        {
                                $$ = obj_alloc(&@$);
                                $$->type = NFT_OBJECT_QUOTA;
-                               $$->quota = *$1;
                        }
                        ;
 
@@ -3703,21 +3675,22 @@ secmark_config          :       string
                        {
                                int ret;
                                struct secmark *secmark;
-                               secmark = xzalloc(sizeof(*secmark));
+
+                               secmark = &$<obj>0->secmark;
                                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);
+                                       xfree($1);
                                        YYERROR;
                                }
-                               $$ = secmark;
+                               xfree($1);
                        }
                        ;
 
-secmark_obj            :       secmark_config
+secmark_obj            :       /* empty */
                        {
                                $$ = obj_alloc(&@$);
                                $$->type = NFT_OBJECT_SECMARK;
-                               $$->secmark = *$1;
                        }
                        ;
 
@@ -3822,7 +3795,7 @@ ct_expect_config  :       PROTOCOL        ct_l4protoname  stmt_separator
                        }
                        ;
 
-ct_obj_alloc           :
+ct_obj_alloc           :       /* empty */
                        {
                                $$ = obj_alloc(&@$);
                        }
@@ -3831,13 +3804,13 @@ ct_obj_alloc            :
 limit_config           :       RATE    limit_mode      NUM     SLASH   time_unit       limit_burst_pkts
                        {
                                struct limit *limit;
-                               limit = xzalloc(sizeof(*limit));
+
+                               limit = &$<obj>0->limit;
                                limit->rate     = $3;
                                limit->unit     = $5;
                                limit->burst    = $6;
                                limit->type     = NFT_LIMIT_PKTS;
                                limit->flags    = $2;
-                               $$ = limit;
                        }
                        |       RATE    limit_mode      NUM     STRING  limit_burst_bytes
                        {
@@ -3851,21 +3824,19 @@ limit_config            :       RATE    limit_mode      NUM     SLASH   time_unit       limit_burst_pkts
                                        YYERROR;
                                }
 
-                               limit = xzalloc(sizeof(*limit));
+                               limit = &$<obj>0->limit;
                                limit->rate     = rate * $3;
                                limit->unit     = unit;
                                limit->burst    = $5;
                                limit->type     = NFT_LIMIT_PKT_BYTES;
                                limit->flags    = $2;
-                               $$ = limit;
                        }
                        ;
 
-limit_obj              :       limit_config
+limit_obj              :       /* empty */
                        {
                                $$ = obj_alloc(&@$);
                                $$->type = NFT_OBJECT_LIMIT;
-                               $$->limit = *$1;
                        }
                        ;