Re: [PATCH] libxtables: change option precedence order to be intuitive

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

The following changes since commit 648fd1ad68ae2ec675ac07efee80783912535404:
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  libxt_TOS: avoid an undesired overflowing computation (2010-11-02 09:17:09 +0100)

are available in the git repository at:
  git://dev.medozas.de/iptables master

Jan Engelhardt (1):
      iptables: fix longopt reecognition and workaround getopt(3) behavior

 ip6tables.c |    1 +
 iptables.c  |    1 +
 xtables.c   |    3 ++-
 3 files changed, 4 insertions(+), 1 deletions(-)

[PATCH] iptables: fix longopt reecognition and workaround getopt(3) behavior

[Jan Engelhardt]

* On the first call to getopt, opts was NULL, so long options would
not be recognized until a match/target was loaded.

Whacky getopt behavior:

* If the longopts parameter is NULL, getopt fails to recognize unknown
options, such that `iptables-multi main --append` will print a garbage
help message ("main needs an argument").

* If the longopts parameter is NULL on the first call, but not on
subsequent calls, it completely screws up option parsing, taking
the --dport in `iptables-multi main -A INPUT -p tcp --dport 1000`
as --destination instead, but not accepting "--destination 1.2.3.4"
either.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 ip6tables.c |    1 +
 iptables.c  |    1 +
 xtables.c   |    3 ++-
 3 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/ip6tables.c b/ip6tables.c
index 150893d..8318f91 100644
--- a/ip6tables.c
+++ b/ip6tables.c
@@ -147,6 +147,7 @@ void ip6tables_exit_error(enum xtables_exittype status, const char *msg, ...) __
 struct xtables_globals ip6tables_globals = {
 	.option_offset = 0,
 	.program_version = IPTABLES_VERSION,
+	.opts = original_opts,
 	.orig_opts = original_opts,
 	.exit_err = ip6tables_exit_error,
 };
diff --git a/iptables.c b/iptables.c
index 4c8bd77..c800fff 100644
--- a/iptables.c
+++ b/iptables.c
@@ -147,6 +147,7 @@ void iptables_exit_error(enum xtables_exittype status, const char *msg, ...) __a
 struct xtables_globals iptables_globals = {
 	.option_offset = 0,
 	.program_version = IPTABLES_VERSION,
+	.opts = original_opts,
 	.orig_opts = original_opts,
 	.exit_err = iptables_exit_error,
 };
diff --git a/xtables.c b/xtables.c
index 7658038..d0aa868 100644
--- a/xtables.c
+++ b/xtables.c
@@ -75,7 +75,8 @@ void basic_exit_err(enum xtables_exittype status, const char *msg, ...)
 
 void xtables_free_opts(int unused)
 {
-	free(xt_params->opts);
+	if (xt_params->opts != xt_params->orig_opts)
+		free(xt_params->opts);
 }
 
 struct option *xtables_merge_options(struct option *orig_opts,
-- 
1.7.1

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Patrick McHardy]

On 15.11.2010 13:28, Jan Engelhardt wrote:
> The following changes since commit 648fd1ad68ae2ec675ac07efee80783912535404:
>                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>   libxt_TOS: avoid an undesired overflowing computation (2010-11-02 09:17:09 +0100)
> 
> are available in the git repository at:
>   git://dev.medozas.de/iptables master

Well, as I'm sure you're aware this doesn't apply to the
current tree, so please rebase.

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

On Monday 2010-11-15 13:56, Patrick McHardy wrote:

>On 15.11.2010 13:28, Jan Engelhardt wrote:
>> The following changes since commit 648fd1ad68ae2ec675ac07efee80783912535404:
>>                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 
>>   libxt_TOS: avoid an undesired overflowing computation (2010-11-02 09:17:09 +0100)
>> 
>> are available in the git repository at:
>>   git://dev.medozas.de/iptables master
>
>Well, as I'm sure you're aware this doesn't apply to the
>current tree, so please rebase.

Revert the revert and merge?

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Patrick McHardy]

On 15.11.2010 14:13, Jan Engelhardt wrote:
> On Monday 2010-11-15 13:56, Patrick McHardy wrote:
> 
>> On 15.11.2010 13:28, Jan Engelhardt wrote:
>>> The following changes since commit 648fd1ad68ae2ec675ac07efee80783912535404:
>>>                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>>   libxt_TOS: avoid an undesired overflowing computation (2010-11-02 09:17:09 +0100)
>>>
>>> are available in the git repository at:
>>>   git://dev.medozas.de/iptables master
>>
>> Well, as I'm sure you're aware this doesn't apply to the
>> current tree, so please rebase.
> 
> Revert the revert and merge?
> 

Yes please.

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

On Monday 2010-11-15 14:15, Patrick McHardy wrote:
>>>>
>>>> are available in the git repository at:
>>>>   git://dev.medozas.de/iptables master
>> 
>> Revert the revert and merge?
>
>Yes please.

git://dev.medozas.de/iptables master

 include/xtables.h.in |    5 +++--
 ip6tables.c          |    6 +++---
 iptables.c           |   12 +++++++++---
 xtables.c            |   49 ++++++++++++++++++++++++++++---------------------
 4 files changed, 43 insertions(+), 29 deletions(-)

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Patrick McHardy]

On 15.11.2010 14:41, Jan Engelhardt wrote:
> git://dev.medozas.de/iptables master

Pulled, tested and pushed out again. Thanks Jan.

iptables: option precedence order change

[Jan Engelhardt]

The following changes since commit 81defdb2c3ad0e461f6487dc75abb8d4fc77519b:

  libiptc: add Libs.private to pkgconfig files (2010-09-13 23:43:12 +0200)

are available in the git repository at:
  git://dev.medozas.de/iptables master

Jan Engelhardt (1):
      libxtables: change option precedence order to be intuitive

 include/xtables.h.in |    5 +++--
 ip6tables.c          |    7 +++----
 iptables.c           |   13 +++++++++----
 xtables.c            |   50 ++++++++++++++++++++++++++++----------------------
 4 files changed, 43 insertions(+), 32 deletions(-)

[PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
gets an error about the (libxt_mark) --mark option being used twice.
This is because libxt_connmark's option table does not override any
previous options. This patch changes this behavior, since the current
behavior does not allow connmark's option to be used at all, which is
illogical.

Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/xtables.h.in |    5 +++--
 ip6tables.c          |    7 +++----
 iptables.c           |   13 +++++++++----
 xtables.c            |   50 ++++++++++++++++++++++++++++----------------------
 4 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/include/xtables.h.in b/include/xtables.h.in
index 9e47c2d..c3d34af 100644
--- a/include/xtables.h.in
+++ b/include/xtables.h.in
@@ -219,8 +219,9 @@ extern int xtables_insmod(const char *, const char *, bool);
 extern int xtables_load_ko(const char *, bool);
 extern int xtables_set_params(struct xtables_globals *xtp);
 extern void xtables_free_opts(int reset_offset);
-extern struct option *xtables_merge_options(struct option *oldopts,
-	const struct option *newopts, unsigned int *option_offset);
+extern struct option *xtables_merge_options(struct option *origopts,
+	struct option *oldopts, const struct option *newopts,
+	unsigned int *option_offset);
 
 extern int xtables_init_all(struct xtables_globals *xtp, uint8_t nfproto);
 extern struct xtables_match *xtables_find_match(const char *name,
diff --git a/ip6tables.c b/ip6tables.c
index 15067da..150893d 100644
--- a/ip6tables.c
+++ b/ip6tables.c
@@ -147,7 +147,6 @@ void ip6tables_exit_error(enum xtables_exittype status, const char *msg, ...) __
 struct xtables_globals ip6tables_globals = {
 	.option_offset = 0,
 	.program_version = IPTABLES_VERSION,
-	.opts = original_opts,
 	.orig_opts = original_opts,
 	.exit_err = ip6tables_exit_error,
 };
@@ -1561,7 +1560,7 @@ int do_command6(int argc, char *argv[], char **table, struct ip6tc_handle **hand
 				target->t->u.user.revision = target->revision;
 				if (target->init != NULL)
 					target->init(target->t);
-				opts = xtables_merge_options(opts,
+				opts = xtables_merge_options(ip6tables_globals.orig_opts, opts,
 						     target->extra_opts,
 						     &target->option_offset);
 				if (opts == NULL)
@@ -1615,7 +1614,7 @@ int do_command6(int argc, char *argv[], char **table, struct ip6tc_handle **hand
 				m->init(m->m);
 			if (m != m->next)
 				/* Merge options for non-cloned matches */
-				opts = xtables_merge_options(opts, m->extra_opts, &m->option_offset);
+				opts = xtables_merge_options(ip6tables_globals.orig_opts, opts, m->extra_opts, &m->option_offset);
 		}
 		break;
 
@@ -1762,7 +1761,7 @@ int do_command6(int argc, char *argv[], char **table, struct ip6tc_handle **hand
 					if (m->init != NULL)
 						m->init(m->m);
 
-					opts = xtables_merge_options(opts,
+					opts = xtables_merge_options(ip6tables_globals.orig_opts, opts,
 					    m->extra_opts, &m->option_offset);
 
 					optind--;
diff --git a/iptables.c b/iptables.c
index 840dd3e..4c8bd77 100644
--- a/iptables.c
+++ b/iptables.c
@@ -147,7 +147,6 @@ void iptables_exit_error(enum xtables_exittype status, const char *msg, ...) __a
 struct xtables_globals iptables_globals = {
 	.option_offset = 0,
 	.program_version = IPTABLES_VERSION,
-	.opts = original_opts,
 	.orig_opts = original_opts,
 	.exit_err = iptables_exit_error,
 };
@@ -1576,7 +1575,9 @@ int do_command(int argc, char *argv[], char **table, struct iptc_handle **handle
 				target->t->u.user.revision = target->revision;
 				if (target->init != NULL)
 					target->init(target->t);
-				opts = xtables_merge_options(opts,
+				opts = xtables_merge_options(
+						     iptables_globals.orig_opts,
+						     opts,
 						     target->extra_opts,
 						     &target->option_offset);
 				if (opts == NULL)
@@ -1636,7 +1637,9 @@ int do_command(int argc, char *argv[], char **table, struct iptc_handle **handle
 				m->init(m->m);
 			if (m != m->next) {
 				/* Merge options for non-cloned matches */
-				opts = xtables_merge_options(opts,
+				opts = xtables_merge_options(
+						     iptables_globals.orig_opts,
+						     opts,
 						     m->extra_opts,
 						     &m->option_offset);
 				if (opts == NULL)
@@ -1790,7 +1793,9 @@ int do_command(int argc, char *argv[], char **table, struct iptc_handle **handle
 					if (m->init != NULL)
 						m->init(m->m);
 
-					opts = xtables_merge_options(opts,
+					opts = xtables_merge_options(
+							     iptables_globals.orig_opts,
+							     opts,
 							     m->extra_opts,
 							     &m->option_offset);
 					if (opts == NULL)
diff --git a/xtables.c b/xtables.c
index 47a0d9c..7658038 100644
--- a/xtables.c
+++ b/xtables.c
@@ -73,44 +73,50 @@ void basic_exit_err(enum xtables_exittype status, const char *msg, ...)
 	exit(status);
 }
 
-
-void xtables_free_opts(int reset_offset)
+void xtables_free_opts(int unused)
 {
-	if (xt_params->opts != xt_params->orig_opts) {
-		free(xt_params->opts);
-		xt_params->opts = xt_params->orig_opts;
-		if (reset_offset)
-			xt_params->option_offset = 0;
-	}
+	free(xt_params->opts);
 }
 
-struct option *xtables_merge_options(struct option *oldopts,
+struct option *xtables_merge_options(struct option *orig_opts,
+				     struct option *oldopts,
 				     const struct option *newopts,
 				     unsigned int *option_offset)
 {
-	unsigned int num_old, num_new, i;
-	struct option *merge;
+	unsigned int num_oold = 0, num_old = 0, num_new = 0, i;
+	struct option *merge, *mp;
 
 	if (newopts == NULL)
 		return oldopts;
 
-	for (num_old = 0; oldopts[num_old].name; num_old++) ;
+	for (num_oold = 0; orig_opts[num_oold].name; num_oold++) ;
+	if (oldopts != NULL)
+		for (num_old = 0; oldopts[num_old].name; num_old++) ;
 	for (num_new = 0; newopts[num_new].name; num_new++) ;
 
+	merge = malloc(sizeof(*mp) * (num_oold + num_old + num_new + 1));
+	if (merge == NULL)
+		return NULL;
+
+	/* Let the base options -[ADI...] have precedence over everything */
+	memcpy(merge, orig_opts, sizeof(*mp) * num_oold);
+	mp = merge + num_oold;
+
+	/* Second, the new options */
 	xt_params->option_offset += 256;
 	*option_offset = xt_params->option_offset;
+	memcpy(mp, newopts, sizeof(*mp) * num_new);
 
-	merge = malloc(sizeof(struct option) * (num_new + num_old + 1));
-	if (merge == NULL)
-		return NULL;
-	memcpy(merge, oldopts, num_old * sizeof(struct option));
-	xtables_free_opts(0);	/* Release any old options merged  */
-	for (i = 0; i < num_new; i++) {
-		merge[num_old + i] = newopts[i];
-		merge[num_old + i].val += *option_offset;
-	}
-	memset(merge + num_old + num_new, 0, sizeof(struct option));
+	for (i = 0; i < num_new; ++i, ++mp)
+		mp->val += *option_offset;
+
+	/* Third, the old options */
+	memcpy(mp, oldopts, sizeof(*mp) * num_old);
+	mp += num_old;
+	xtables_free_opts(0);
 
+	/* Clear trailing entry */
+	memset(mp, 0, sizeof(*mp));
 	return merge;
 }
 
-- 
1.7.1

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

On Tuesday 2010-10-19 17:16, Jan Engelhardt wrote:

>When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>gets an error about the (libxt_mark) --mark option being used twice.
>This is because libxt_connmark's option table does not override any
>previous options. This patch changes this behavior, since the current
>behavior does not allow connmark's option to be used at all, which is
>illogical.

As per fw's suggestion, I should nag you every 48 hours ;-)

Really, this should be in the next tag.

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Patrick McHardy]

On 25.10.2010 23:57, Jan Engelhardt wrote:
> On Tuesday 2010-10-19 17:16, Jan Engelhardt wrote:
> 
>> When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>> gets an error about the (libxt_mark) --mark option being used twice.
>> This is because libxt_connmark's option table does not override any
>> previous options. This patch changes this behavior, since the current
>> behavior does not allow connmark's option to be used at all, which is
>> illogical.
> 
> As per fw's suggestion, I should nag you every 48 hours ;-)
> 
> Really, this should be in the next tag.

This stuff is pretty fragile and whenever we changed it, something broke
for users. Why not simply add new options  (--connmark etc). That would
also be more consistent with the other extensions.

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

On Thursday 2010-10-28 11:54, Patrick McHardy wrote:

>On 25.10.2010 23:57, Jan Engelhardt wrote:
>> On Tuesday 2010-10-19 17:16, Jan Engelhardt wrote:
>> 
>>> When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>>> gets an error about the (libxt_mark) --mark option being used twice.
>>> This is because libxt_connmark's option table does not override any
>>> previous options. This patch changes this behavior, since the current
>>> behavior does not allow connmark's option to be used at all, which is
>>> illogical.
>> 
>> As per fw's suggestion, I should nag you every 48 hours ;-)
>> 
>> Really, this should be in the next tag.
>
>This stuff is pretty fragile and whenever we changed it, something broke
>for users. Why not simply add new options  (--connmark etc). That would
>also be more consistent with the other extensions.

Adding new options does not fix the real problem, and you cannot easily 
ensure that no options will clash with another.
And what do you do when you use -m hashlimit --hashlimit-name abc -m 
hashlimit --hashlimit-name def? You can't use new options here.

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Changli Gao]

On Thu, Oct 28, 2010 at 6:54 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
> On Thursday 2010-10-28 11:54, Patrick McHardy wrote:
>
>>On 25.10.2010 23:57, Jan Engelhardt wrote:
>>> On Tuesday 2010-10-19 17:16, Jan Engelhardt wrote:
>>>
>>>> When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>>>> gets an error about the (libxt_mark) --mark option being used twice.
>>>> This is because libxt_connmark's option table does not override any
>>>> previous options. This patch changes this behavior, since the current
>>>> behavior does not allow connmark's option to be used at all, which is
>>>> illogical.
>>>
>>> As per fw's suggestion, I should nag you every 48 hours ;-)
>>>
>>> Really, this should be in the next tag.
>>
>>This stuff is pretty fragile and whenever we changed it, something broke
>>for users. Why not simply add new options  (--connmark etc). That would
>>also be more consistent with the other extensions.
>
> Adding new options does not fix the real problem, and you cannot easily
> ensure that no options will clash with another.
> And what do you do when you use -m hashlimit --hashlimit-name abc -m
> hashlimit --hashlimit-name def? You can't use new options here.
>

I think this issue was discussed in this thread:
http://marc.info/?l=netfilter-devel&m=121881908205975&w=2 . And the
above command should be OK. Did you test it?

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Jan Engelhardt]

On Friday 2010-10-29 07:26, Changli Gao wrote:
>>>>
>>>>> When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>>>>> gets an error about the (libxt_mark) --mark option being used twice.
>>>>> This is because libxt_connmark's option table does not override any
>>>>> previous options. This patch changes this behavior, since the current
>>>>> behavior does not allow connmark's option to be used at all, which is
>>>>> illogical.
>>>>
>>>> As per fw's suggestion, I should nag you every 48 hours ;-)
>>>>
>>>> Really, this should be in the next tag.
>>>
>>>This stuff is pretty fragile and whenever we changed it, something broke
>>>for users. Why not simply add new options  (--connmark etc). That would
>>>also be more consistent with the other extensions.
>>
>> Adding new options does not fix the real problem, and you cannot easily
>> ensure that no options will clash with another.
>> And what do you do when you use -m hashlimit --hashlimit-name abc -m
>> hashlimit --hashlimit-name def? You can't use new options here.
>
>I think this issue was discussed in this thread:
>http://marc.info/?l=netfilter-devel&m=121881908205975&w=2 . And the
>above command should be OK. Did you test it?

Well ok bad example; -m hashlimit twice works, as does your linked 
example when replacing -m a and -m b with -m mac.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Patrick McHardy]

Am 29.10.2010 14:59, schrieb Jan Engelhardt:
> On Friday 2010-10-29 07:26, Changli Gao wrote:
>>>>>
>>>>>> When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>>>>>> gets an error about the (libxt_mark) --mark option being used twice.
>>>>>> This is because libxt_connmark's option table does not override any
>>>>>> previous options. This patch changes this behavior, since the current
>>>>>> behavior does not allow connmark's option to be used at all, which is
>>>>>> illogical.
>>>>>
>>>>> As per fw's suggestion, I should nag you every 48 hours ;-)
>>>>>
>>>>> Really, this should be in the next tag.
>>>>
>>>> This stuff is pretty fragile and whenever we changed it, something broke
>>>> for users. Why not simply add new options  (--connmark etc). That would
>>>> also be more consistent with the other extensions.
>>>
>>> Adding new options does not fix the real problem, and you cannot easily
>>> ensure that no options will clash with another.
>>> And what do you do when you use -m hashlimit --hashlimit-name abc -m
>>> hashlimit --hashlimit-name def? You can't use new options here.
>>
>> I think this issue was discussed in this thread:
>> http://marc.info/?l=netfilter-devel&m=121881908205975&w=2 . And the
>> above command should be OK. Did you test it?
> 
> Well ok bad example; -m hashlimit twice works, as does your linked 
> example when replacing -m a and -m b with -m mac.

I've applied the patch to iptables-next, but if something breaks
I'll revert it and add --connmark- prefixed options.

Re: [PATCH] libxtables: change option precedence order to be intuitive

[Patrick McHardy]

On 29.10.2010 14:59, Jan Engelhardt wrote:
> On Friday 2010-10-29 07:26, Changli Gao wrote:
>>>>>
>>>>>> When using `-m mark --mark 2 -m connmark --mark 2`, the user currently
>>>>>> gets an error about the (libxt_mark) --mark option being used twice.
>>>>>> This is because libxt_connmark's option table does not override any
>>>>>> previous options. This patch changes this behavior, since the current
>>>>>> behavior does not allow connmark's option to be used at all, which is
>>>>>> illogical.
>>>>>
>>>>> As per fw's suggestion, I should nag you every 48 hours ;-)
>>>>>
>>>>> Really, this should be in the next tag.
>>>>
>>>> This stuff is pretty fragile and whenever we changed it, something broke
>>>> for users. Why not simply add new options  (--connmark etc). That would
>>>> also be more consistent with the other extensions.

Something very non-intuitive is happening with this patch:

# iptables -A OUTPUT -p tcp --dport 10000
iptables v1.4.9: host/network `port' not found
Try `iptables -h' or 'iptables --help' for more information.

port is interpreted as parameter to '-d'. I'm reverting this for
now, please resend if you can come up with a way to fix this.