On 23 March 2016 at 17:08, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Wed, Mar 23, 2016 at 01:51:38PM +0100, Arturo Borrero Gonzalez wrote:
>> Improve checks (and error reporting) for basic rule management operations.
>>
>> This includes a fix for netfilter bug #965.
>
> Thanks for working on this.
>
> With a bit more work we can achieve better error reporting:
>
> # nft insert rule x y handle 10 position 10 ip saddr 1.1.1.1
> <cmdline>:1:17-25: Error: Wrong combination, use `position' instead
> insert rule x y handle 10 position 10 ip saddr 1.1.1.1
> ^^^^^^^^^ ~~~~~~~~~~~
>
> # nft insert rule x y handle 10 ip saddr 1.1.1.1
> <cmdline>:1:17-25: Error: Cannot use this, use `position' instead
> insert rule x y handle 10 ip saddr 1.1.1.1
> ^^^^^^^^^
>
> You will need to rework this patch applying this patch in first place:
>
> http://patchwork.ozlabs.org/patch/601270/
>
> I'm also attaching a quick patch for the two examples, but remaining
> spots are left unchanges. Please feel free to follow up on this.
>
> Thanks.
Hi,
I completed the patch, amended a bit the error messages.
Find it attached.
The applies in in this order:
* http://patchwork.ozlabs.org/patch/601270/ (src: store parser
location for handle and position IDs)
* http://patchwork.ozlabs.org/patch/601216/ (src/rule.c: don't print
trailing statement whitespace)
* the attached patch
* http://patchwork.ozlabs.org/patch/601218/ (tests/shell: add
testcases for Netfilter bug #965)
best regards.
--
Arturo Borrero González
From: Arturo Borrero Gonzalez <arturo.borrero.glez@xxxxxxxxx>
---
src/evaluate.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/evaluate.c b/src/evaluate.c
index 473f014..cf157f7 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -65,6 +65,12 @@ static int __fmtstring(4, 5) __stmt_binary_error(struct eval_ctx *ctx,
__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
#define cmd_error(ctx, fmt, args...) \
__stmt_binary_error(ctx, &(ctx->cmd)->location, NULL, fmt, ## args)
+#define handle_error(ctx, fmt, args...) \
+ __stmt_binary_error(ctx, &ctx->cmd->handle.handle.location, NULL, fmt, ## args)
+#define position_error(ctx, fmt, args...) \
+ __stmt_binary_error(ctx, &ctx->cmd->handle.position.location, NULL, fmt, ## args)
+#define handle_position_error(ctx, fmt, args...) \
+ __stmt_binary_error(ctx, &ctx->cmd->handle.handle.location, &ctx->cmd->handle.position.location, fmt, ## args)
static int __fmtstring(3, 4) set_error(struct eval_ctx *ctx,
const struct set *set,
@@ -2160,11 +2166,86 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
return 0;
}
+static int rule_evaluate_cmd(struct eval_ctx *ctx)
+{
+ struct handle *handle = &ctx->cmd->handle;
+
+ /* allowed:
+ * - insert [position] (no handle)
+ * - add [position] (no handle)
+ * - replace <handle> (no position)
+ * - delete <handle> (no position)
+ */
+
+ switch (ctx->cmd->op) {
+ case CMD_INSERT:
+ if (handle->handle.id && handle->position.id)
+ return handle_position_error(ctx, "Could not insert "
+ "rule: wrong combination"
+ ", use only `position' "
+ "instead");
+
+ if (handle->handle.id)
+ return handle_error(ctx, "Could not insert rule: "
+ "cannot use this, use "
+ "`position' instead");
+ break;
+ case CMD_ADD:
+ if (handle->handle.id && handle->position.id)
+ return handle_position_error(ctx, "Could not add "
+ "rule: wrong combination"
+ ", use only `position' "
+ "instead");
+
+ if (handle->handle.id)
+ return handle_error(ctx, "Could not add rule: "
+ "cannot use this, use "
+ "`position' instead");
+
+ break;
+ case CMD_REPLACE:
+ if (handle->handle.id && handle->position.id)
+ return handle_position_error(ctx, "Could not replace "
+ "rule: wrong combination"
+ ", use only `handle' "
+ "instead");
+ if (handle->position.id)
+ return position_error(ctx, "Could not replace rule: "
+ "cannot use this, use `handle' "
+ "instead");
+ if (!handle->handle.id)
+ return cmd_error(ctx, "Could not replace rule: missing"
+ " `handle'.");
+ break;
+ case CMD_DELETE:
+ if (handle->handle.id && handle->position.id)
+ return handle_position_error(ctx, "Could not replace "
+ "rule: wrong combination"
+ ", use only `handle' "
+ "instead");
+ if (handle->position.id)
+ return position_error(ctx, "Could not replace rule: "
+ "cannot use this, use `handle' "
+ "instead");
+ if (!handle->handle.id)
+ return cmd_error(ctx, "Could not replace rule: missing"
+ " `handle'.");
+ break;
+ default:
+ BUG("unkown command type %u\n", ctx->cmd->op);
+ }
+
+ return 0;
+}
+
static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
{
struct stmt *stmt, *tstmt = NULL;
struct error_record *erec;
+ if (rule_evaluate_cmd(ctx) < 0)
+ return -1;
+
proto_ctx_init(&ctx->pctx, rule->handle.family);
memset(&ctx->ectx, 0, sizeof(ctx->ectx));
@@ -2345,8 +2426,11 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
return ret;
return setelem_evaluate(ctx, &cmd->expr);
- case CMD_OBJ_SET:
case CMD_OBJ_RULE:
+ if (rule_evaluate_cmd(ctx) < 0)
+ return -1;
+ /* fall through */
+ case CMD_OBJ_SET:
case CMD_OBJ_CHAIN:
case CMD_OBJ_TABLE:
return 0;
