data_reg: fix verdict format approach

Patrick reports that the XML/JSON formats of the data_reg object
are not accuarate.

This patch updates these formats, so they are now as follow:

 * <data_reg type=value> with raw data (this doesn't change).
 * <data_reg type=verdict> with a concrete verdict (eg drop accept) and an
  optional <chain>, with destination.

In XML:
	<data_reg type="verdict">
		<verdict>goto</verdict>
		<chain>output</chain>
	</data_reg>

In JSON:
	"data_reg" : {
		"type" : "verdict",
		"verdict" : "goto"
		"chain" : "output",
	}

The default output format is updated to reflect these changes (minor collateral
thing).

When parsing set_elems, to know if we need to add the NFT_SET_ELEM_ATTR_CHAIN
flag, a basic check for the chain not being NULL is done, instead of evaluating
if the result of the parsing was DATA_CHAIN. The DATA_CHAIN symbol is no longer
used in the data_reg XML/JSON parsing zone.

While at it, I updated the error reporting stuff regarding data_reg/verdict, in
order to leave a consistent state in the library.

A JSON testfile is updated as well.

Reported-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/src/expr/data_reg.c b/src/expr/data_reg.c
index 8812dafbe5c0fd4f8096ead8f4c2911783575d06..a7559485cbc17165a243a587c9c41872188c6a32 100644 (file)
--- a/src/expr/data_reg.c
+++ b/src/expr/data_reg.c
@@ -32,10 +32,11 @@ static int nft_data_reg_verdict_json_parse(union nft_data_reg *reg, json_t *data
 {
        int verdict;
        const char *verdict_str;
+       const char *chain;
 
        verdict_str = nft_jansson_parse_str(data, "verdict", err);
        if (verdict_str == NULL)
-               return -1;
+               return DATA_NONE;
 
        if (nft_str2verdict(verdict_str, &verdict) != 0) {
                err->node_name = "verdict";
@@ -46,18 +47,15 @@ static int nft_data_reg_verdict_json_parse(union nft_data_reg *reg, json_t *data
 
        reg->verdict = (uint32_t)verdict;
 
-       return 0;
-}
+       if (nft_jansson_node_exist(data, "chain")) {
+               chain = nft_jansson_parse_str(data, "chain", err);
+               if (chain == NULL)
+                       return DATA_NONE;
 
-static int nft_data_reg_chain_json_parse(union nft_data_reg *reg, json_t *data,
-                                        struct nft_parse_err *err)
-{
-       reg->chain = strdup(nft_jansson_parse_str(data, "chain", err));
-       if (reg->chain == NULL) {
-               return -1;
+               reg->chain = strdup(chain);
        }
 
-       return 0;
+       return DATA_VERDICT;
 }
 
 static int nft_data_reg_value_json_parse(union nft_data_reg *reg, json_t *data,
@@ -67,17 +65,17 @@ static int nft_data_reg_value_json_parse(union nft_data_reg *reg, json_t *data,
        char node_name[6];
 
        if (nft_jansson_parse_val(data, "len", NFT_TYPE_U8, &reg->len, err) < 0)
-                       return -1;
+                       return DATA_NONE;
 
        for (i = 0; i < div_round_up(reg->len, sizeof(uint32_t)); i++) {
                sprintf(node_name, "data%d", i);
 
                if (nft_jansson_str2num(data, node_name, BASE_HEX,
                                        &reg->val[i], NFT_TYPE_U32, err) != 0)
-                       return -1;
+                       return DATA_NONE;
        }
 
-       return 0;
+       return DATA_VALUE;
 }
 #endif
 
@@ -93,15 +91,12 @@ int nft_data_reg_json_parse(union nft_data_reg *reg, json_t *data,
                return -1;
 
        /* Select what type of parsing is needed */
-       if (strcmp(type, "value") == 0) {
+       if (strcmp(type, "value") == 0)
                return nft_data_reg_value_json_parse(reg, data, err);
-       } else if (strcmp(type, "verdict") == 0) {
+       else if (strcmp(type, "verdict") == 0)
                return nft_data_reg_verdict_json_parse(reg, data, err);
-       } else if (strcmp(type, "chain") == 0) {
-               return nft_data_reg_chain_json_parse(reg, data, err);
-       }
 
-       return 0;
+       return DATA_NONE;
 #else
        errno = EOPNOTSUPP;
        return -1;
@@ -115,6 +110,7 @@ static int nft_data_reg_verdict_xml_parse(union nft_data_reg *reg,
 {
        int verdict;
        const char *verdict_str;
+       const char *chain;
 
        verdict_str = nft_mxml_str_parse(tree, "verdict", MXML_DESCEND_FIRST,
                                         NFT_XML_MAND, err);
@@ -130,25 +126,16 @@ static int nft_data_reg_verdict_xml_parse(union nft_data_reg *reg,
 
        reg->verdict = (uint32_t)verdict;
 
-       return DATA_VERDICT;
-}
-
-static int nft_data_reg_chain_xml_parse(union nft_data_reg *reg,
-                                       mxml_node_t *tree,
-                                       struct nft_parse_err *err)
-{
-       const char *chain;
-
        chain = nft_mxml_str_parse(tree, "chain", MXML_DESCEND_FIRST,
-                                  NFT_XML_MAND, err);
-       if (chain == NULL)
-               return DATA_NONE;
+                                  NFT_XML_OPT, err);
+       if (chain != NULL) {
+               if (reg->chain)
+                       xfree(reg->chain);
 
-       if (reg->chain)
-               xfree(reg->chain);
+               reg->chain = strdup(chain);
+       }
 
-       reg->chain = strdup(chain);
-       return DATA_CHAIN;
+       return DATA_VERDICT;
 }
 
 static int nft_data_reg_value_xml_parse(union nft_data_reg *reg,
@@ -195,25 +182,24 @@ int nft_data_reg_xml_parse(union nft_data_reg *reg, mxml_node_t *tree,
 
        node = mxmlFindElement(tree, tree, "data_reg", "type", NULL,
                               MXML_DESCEND_FIRST);
-       if (node == NULL) {
-               errno = EINVAL;
-               return DATA_NONE;
-       }
+       if (node == NULL)
+               goto err;
 
        type = mxmlElementGetAttr(node, "type");
 
-       if (type == NULL) {
-               errno = EINVAL;
-               return DATA_NONE;
-       }
+       if (type == NULL)
+               goto err;
 
        if (strcmp(type, "value") == 0)
                return nft_data_reg_value_xml_parse(reg, node, err);
        else if (strcmp(type, "verdict") == 0)
                return nft_data_reg_verdict_xml_parse(reg, node, err);
-       else if (strcmp(type, "chain") == 0)
-               return nft_data_reg_chain_xml_parse(reg, node, err);
 
+       return DATA_NONE;
+err:
+       errno = EINVAL;
+       err->node_name = "data_reg";
+       err->error = NFT_PARSE_EMISSINGNODE;
        return DATA_NONE;
 #else
        errno = EOPNOTSUPP;
@@ -308,6 +294,67 @@ nft_data_reg_value_snprintf_default(char *buf, size_t size,
        return offset;
 }
 
+static int
+nft_data_reg_verdict_snprintf_def(char *buf, size_t size,
+                                 union nft_data_reg *reg, uint32_t flags)
+{
+       int len = size, offset = 0, ret = 0;
+
+       ret = snprintf(buf, size, "%s ", nft_verdict2str(reg->verdict));
+       SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+       if (reg->chain != NULL) {
+               ret = snprintf(buf+offset, size, "-> %s ", reg->chain);
+               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+       }
+
+       return offset;
+}
+
+static int
+nft_data_reg_verdict_snprintf_xml(char *buf, size_t size,
+                                 union nft_data_reg *reg, uint32_t flags)
+{
+       int len = size, offset = 0, ret = 0;
+
+       ret = snprintf(buf, size, "<data_reg type=\"verdict\">"
+                      "<verdict>%s</verdict>", nft_verdict2str(reg->verdict));
+       SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+       if (reg->chain != NULL) {
+               ret = snprintf(buf+offset, size, "<chain>%s</chain>",
+                              reg->chain);
+               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+       }
+
+       ret = snprintf(buf+offset, size, "</data_reg>");
+       SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+       return offset;
+}
+
+static int
+nft_data_reg_verdict_snprintf_json(char *buf, size_t size,
+                                  union nft_data_reg *reg, uint32_t flags)
+{
+       int len = size, offset = 0, ret = 0;
+
+       ret = snprintf(buf, size, "\"data_reg\":{\"type\":\"verdict\","
+                      "\"verdict\":\"%s\"", nft_verdict2str(reg->verdict));
+       SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+       if (reg->chain != NULL) {
+               ret = snprintf(buf+offset, size, ",\"chain\":\"%s\"",
+                              reg->chain);
+               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+       }
+
+       ret = snprintf(buf+offset, size, "}");
+       SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+       return offset;
+}
+
 int nft_data_reg_snprintf(char *buf, size_t size, union nft_data_reg *reg,
                          uint32_t output_format, uint32_t flags, int reg_type)
 {
@@ -327,44 +374,24 @@ int nft_data_reg_snprintf(char *buf, size_t size, union nft_data_reg *reg,
                        break;
                }
        case DATA_VERDICT:
-               switch(output_format) {
-               case NFT_OUTPUT_DEFAULT:
-                       return snprintf(buf, size, "%d ", reg->verdict);
-               case NFT_OUTPUT_XML:
-                       return snprintf(buf, size,
-                                       "<data_reg type=\"verdict\">"
-                                               "<verdict>%s</verdict>"
-                                       "</data_reg>",
-                                       nft_verdict2str(reg->verdict));
-               case NFT_OUTPUT_JSON:
-                       return snprintf(buf, size,
-                                       "\"data_reg\":{"
-                                       "\"type\":\"verdict\","
-                                       "\"verdict\":\"%s\""
-                                       "}", nft_verdict2str(reg->verdict));
-               default:
-                       break;
-               }
        case DATA_CHAIN:
                switch(output_format) {
                case NFT_OUTPUT_DEFAULT:
-                       return snprintf(buf, size, "%s ", reg->chain);
+                       return nft_data_reg_verdict_snprintf_def(buf, size,
+                                                                reg, flags);
                case NFT_OUTPUT_XML:
-                       return snprintf(buf, size,
-                                       "<data_reg type=\"chain\">"
-                                               "<chain>%s</chain>"
-                                       "</data_reg>", reg->chain);
+                       return nft_data_reg_verdict_snprintf_xml(buf, size,
+                                                                reg, flags);
                case NFT_OUTPUT_JSON:
-                       return snprintf(buf, size,
-                                       "\"data_reg\":{\"type\":\"chain\","
-                                       "\"chain\":\"%s\""
-                                       "}", reg->chain);
+                       return nft_data_reg_verdict_snprintf_json(buf, size,
+                                                                 reg, flags);
                default:
                        break;
                }
        default:
                break;
        }
+
        return -1;
 }
 

diff --git a/src/jansson.c b/src/jansson.c
index f446e175a9724c341c26dbd4efac9323e1310d19..26bd7001820d4ac65f5a145f9d3082ceb56ac9f1 100644 (file)
--- a/src/jansson.c
+++ b/src/jansson.c
@@ -213,7 +213,6 @@ int nft_jansson_data_reg_parse(json_t *root, const char *node_name,
                               struct nft_parse_err *err)
 {
        json_t *data;
-       const char *type;
        int ret;
 
        data = json_object_get(root, node_name);
@@ -233,27 +232,12 @@ int nft_jansson_data_reg_parse(json_t *root, const char *node_name,
        }
 
        ret = nft_data_reg_json_parse(data_reg, data, err);
-       if (ret < 0) {
+       if (ret == DATA_NONE) {
                errno = EINVAL;
                return -1;
        }
 
-       type = nft_jansson_parse_str(data, "type", err);
-       if (type == NULL)
-               return -1;
-
-       if (strcmp(type, "value") == 0)
-               return DATA_VALUE;
-       else if (strcmp(type, "verdict") == 0)
-               return DATA_VERDICT;
-       else if (strcmp(type, "chain") == 0)
-               return DATA_CHAIN;
-       else {
-               err->error = NFT_PARSE_EBADTYPE;
-               err->node_name = "type";
-               errno = EINVAL;
-               return -1;
-       }
+       return ret;
 }
 
 int nft_jansson_set_elem_parse(struct nft_set_elem *e, json_t *root,
@@ -281,10 +265,11 @@ int nft_jansson_set_elem_parse(struct nft_set_elem *e, json_t *root,
                        break;
                case DATA_VERDICT:
                        e->flags |= (1 << NFT_SET_ELEM_ATTR_VERDICT);
+                       if (e->data.chain != NULL)
+                               e->flags |= (1 << NFT_SET_ELEM_ATTR_CHAIN);
+
                        break;
-               case DATA_CHAIN:
-                       e->flags |= (1 << NFT_SET_ELEM_ATTR_CHAIN);
-                       break;
+               case DATA_NONE:
                default:
                        return -1;
                }

diff --git a/src/set_elem.c b/src/set_elem.c
index 2bbfb0e2a62d8b85fae540b67f6dc0816b9098a5..26c11d0069092a9ee15031fc6977bf4eda76bd07 100644 (file)
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -384,9 +384,9 @@ int nft_mxml_set_elem_parse(mxml_node_t *tree, struct nft_set_elem *e,
                break;
        case DATA_VERDICT:
                e->flags |= (1 << NFT_SET_ELEM_ATTR_VERDICT);
-               break;
-       case DATA_CHAIN:
-               e->flags |= (1 << NFT_SET_ELEM_ATTR_CHAIN);
+               if (e->data.chain != NULL)
+                       e->flags |= (1 << NFT_SET_ELEM_ATTR_CHAIN);
+
                break;
        }
 

diff --git a/tests/jsonfiles/63-set.json b/tests/jsonfiles/63-set.json
index ea2fe3dbb4cd401395416c61ee363b83542dee00..62ccd2fdcead4407e45f081f5c5586223ddcfe6b 100644 (file)
--- a/tests/jsonfiles/63-set.json
+++ b/tests/jsonfiles/63-set.json
@@ -1 +1 @@
-{"nftables":[{"set":{"name":"map0","table":"filter","flags":11,"family":"ip","key_type":12,"key_len":2,"data_type":4294967040,"data_len":16,"set_elem":[{"flags":0,"key":{"data_reg":{"type":"value","len":2,"data0":"0x00001700"}},"data":{"data_reg":{"type":"chain","chain":"forward"}}},{"flags":0,"key":{"data_reg":{"type":"value","len":2,"data0":"0x00001600"}},"data":{"data_reg":{"type":"chain","chain":"chain1"}}}]}}]}
+{"nftables":[{"set":{"name":"map0","table":"f","flags":11,"family":"ip","key_type":12,"key_len":2,"data_type":4294967040,"data_len":16,"set_elem":[{"flags":0,"key":{"data_reg":{"type":"value","len":2,"data0":"0x00001700"}},"data":{"data_reg":{"type":"verdict","verdict":"goto","chain":"o"}}},{"flags":0,"key":{"data_reg":{"type":"value","len":2,"data0":"0x00001600"}},"data":{"data_reg":{"type":"verdict","verdict":"accept"}}}]}}]}

diff --git a/tests/nft-parsing-test.c b/tests/nft-parsing-test.c
index b2ad62e6316abd63191a78c892207b650493a4df..df981ad005ff5a5639cea76e87f548772914fe66 100644 (file)
--- a/tests/nft-parsing-test.c
+++ b/tests/nft-parsing-test.c
@@ -121,6 +121,7 @@ failparsing:
        fclose(fp);
        printf("parsing %s: ", filename);
        printf("\033[31mFAILED\e[0m (%s)\n", strerror(errno));
+       nft_parse_perror("fail", err);
        return -1;
 }