Skip to contentSkip to conversation

iptables: fix error reporting with wrong/missing arguments

PATCH In review Pablo Neira Ayuso 2 participants 7 messages Nov 18, 2008 – Nov 19, 2008 netfilter

lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 1/3 iptables: fix error reporting with wrong/missing arguments

This patch fixes wrong error reporting when arguments are missing:

# iptables -I INPUT -m state --state
iptables v1.4.2-rc1: Unknown arg `(null)'
Try `iptables -h' or 'iptables --help' for more information.

or wrong:

# iptables -I INPUT -m state --xyz
iptables v1.4.2-rc1: Unknown arg `(null)'
Try `iptables -h' or 'iptables --help' for more information.
Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 ip6tables.c |   19 ++++++++++++++++++-
 iptables.c  |   19 ++++++++++++++++++-
 2 files changed, 36 insertions(+), 2 deletions(-)
ip6tables.c +18 -1
index 12298ca..9ce1074 100644
@@ -1888,9 +1888,26 @@ int do_command6(int argc, char *argv[], char **table, ip6tc_handle_t *handle)
 					continue;
 				}
 
-				if (!m)
+				if (!m) {
+					if (c == '?') {
+						if (optopt) {
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "option `%s' "
+							   "requires an "
+							   "argument",
+							   argv[optind-1]);
+						} else {
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "unknown option "
+							   "`%s'",
+							   argv[optind-1]);
+						}
+					}
 					exit_error(PARAMETER_PROBLEM,
 						   "Unknown arg `%s'", optarg);
+				}
 			}
 		}
 		invert = FALSE;
iptables.c +18 -1
index b927a11..d2b9081 100644
@@ -1909,9 +1909,26 @@ int do_command(int argc, char *argv[], char **table, iptc_handle_t *handle)
 					optind--;
 					continue;
 				}
-				if (!m)
+				if (!m) {
+					if (c == '?') {
+						if (optopt) {
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "option `%s' "
+							   "requires an "
+							   "argument",
+							   argv[optind-1]);
+						} else {
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "unknown option "
+							   "`%s'",
+							   argv[optind-1]);
+						}
+					}
 					exit_error(PARAMETER_PROBLEM,
 						   "Unknown arg `%s'", optarg);
+				}
 			}
 		}
 		invert = FALSE;
author maintainer
#1 lore raw

PATCH 2/3 state: report spaces in the state list parsing

This patch adds better error reporting when the user inserts a space between two states with the --state option.

iptables -I INPUT -m state ESTABLISHED, RELATED
^
mind the space

results in:

iptables v1.4.2-rc1: Bad state `'
Try `iptables -h' or 'iptables --help' for more information.

Now this returns:

iptables v1.4.2-rc1: `--state' requires a list of states with no spaces, e.g. ESTABLISHED,RELATED

Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 extensions/libxt_state.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
extensions/libxt_state.c +4 -1
index 3af2e48..ae8ae7b 100644
@@ -54,7 +54,10 @@ state_parse_states(const char *arg, struct xt_state_info *sinfo)
 			exit_error(PARAMETER_PROBLEM, "Bad state `%s'", arg);
 		arg = comma+1;
 	}
-
+	if (!*arg)
+		exit_error(PARAMETER_PROBLEM, "`--state' requires a list of "
+					      "states with no spaces, e.g. "
+					      "ESTABLISHED,RELATED");
 	if (strlen(arg) == 0 || !state_parse_state(arg, strlen(arg), sinfo))
 		exit_error(PARAMETER_PROBLEM, "Bad state `%s'", arg);
 }
#2 lore raw

Re: [PATCH 2/3] state: report spaces in the state list parsing

Quoted 10 lines · Pablo Neira Ayuso

Now this returns:

iptables v1.4.2-rc1: `--state' requires a list of states with no spaces, e.g. ESTABLISHED,RELATED

Signed-off-by Pablo Neira Ayuso <[email protected]>

---

extensions/libxt_state.c | 5 ++++-

1 files changed, 4 insertions(+), 1 deletions(-)

This also needs to be done for libxt_conntrack.c I guess.

author maintainer
#3 lore raw

Re: [PATCH 2/3] state: report spaces in the state list parsing

Quoted 7 lines · Jan Engelhardt
Quoted 5 lines · Pablo Neira Ayuso

Now this returns:

iptables v1.4.2-rc1: `--state' requires a list of states with no spaces, e.g. ESTABLISHED,RELATED

Quoted 7 lines
Quoted 5 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>

---

extensions/libxt_state.c | 5 ++++-

1 files changed, 4 insertions(+), 1 deletions(-)

This also needs to be done for libxt_conntrack.c I guess.

Thanks for the spot, new patch attached.

Signature · 1 line
"Los honestos son inadaptados sociales" -- Les Luthiers

state: report spaces in the state list parsing

From: Pablo Neira Ayuso <[email protected]>

This patch adds better error reporting when the user inserts a space between two states with the --state option.

iptables -I INPUT -m state ESTABLISHED, RELATED
^
mind the space

results in:

iptables v1.4.2-rc1: Bad state `'
Try `iptables -h' or 'iptables --help' for more information.

Now this returns:

iptables v1.4.2-rc1: `--state' requires a list of states with no spaces, e.g. ESTABLISHED,RELATED

This patch also applies to libxt_conntrack which has a copy of the function.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 extensions/libxt_conntrack.c |    5 ++++-
 extensions/libxt_state.c     |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)
extensions/libxt_conntrack.c +4 -1
index 79ed3b8..5f3257c 100644
@@ -107,7 +107,10 @@ parse_states(const char *arg, struct xt_conntrack_info *sinfo)
 			exit_error(PARAMETER_PROBLEM, "Bad ctstate `%s'", arg);
 		arg = comma+1;
 	}
-
+	if (!*arg)
+		exit_error(PARAMETER_PROBLEM, "`--ctstate' requires a list of "
+					      "states with no spaces, e.g. "
+					      "ESTABLISHED,RELATED");
 	if (strlen(arg) == 0 || !parse_state(arg, strlen(arg), sinfo))
 		exit_error(PARAMETER_PROBLEM, "Bad ctstate `%s'", arg);
 }
extensions/libxt_state.c +4 -1
index 3af2e48..ae8ae7b 100644
@@ -54,7 +54,10 @@ state_parse_states(const char *arg, struct xt_state_info *sinfo)
 			exit_error(PARAMETER_PROBLEM, "Bad state `%s'", arg);
 		arg = comma+1;
 	}
-
+	if (!*arg)
+		exit_error(PARAMETER_PROBLEM, "`--state' requires a list of "
+					      "states with no spaces, e.g. "
+					      "ESTABLISHED,RELATED");
 	if (strlen(arg) == 0 || !state_parse_state(arg, strlen(arg), sinfo))
 		exit_error(PARAMETER_PROBLEM, "Bad state `%s'", arg);
 }
author maintainer
#4 lore raw

PATCH 3/3 iptables: refer to dmesg when we hit error

In reply to Pablo Neira Ayuso

This does not make any better, but at least refer to dmesg which is the common source of information to diagnose kernel-side problems. This is helpful for newbie users.

# iptables -I INPUT -j CLUSTERIP
iptables: Invalid argument. Run `dmesg' for more information.
Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 ip6tables-standalone.c |    3 ++-
 iptables-standalone.c  |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)
ip6tables-standalone.c +2 -1
index 5bdcd4f..6c20bd8 100644
@@ -70,7 +70,8 @@ main(int argc, char *argv[])
 		ret = ip6tc_commit(&handle);
 
 	if (!ret)
-		fprintf(stderr, "ip6tables: %s\n",
+		fprintf(stderr, "ip6tables: %s. "
+				"Run `dmesg' for more information.\n",
 			ip6tc_strerror(errno));
 
 	exit(!ret);
iptables-standalone.c +2 -1
index 55d9bbe..630c586 100644
@@ -71,7 +71,8 @@ main(int argc, char *argv[])
 		ret = iptc_commit(&handle);
 
 	if (!ret) {
-		fprintf(stderr, "iptables: %s\n",
+		fprintf(stderr, "iptables: %s. "
+				"Run `dmesg' for more information.\n",
 			iptc_strerror(errno));
 		if (errno == EAGAIN) {
 			exit(RESOURCE_PROBLEM);
#5 lore raw
In reply to Pablo Neira Ayuso
Quoted 15 lines · Pablo Neira Ayuso
ip6tables.c +10 -1
@@ -1888,9 +1888,26 @@ int do_command6(int argc, char *argv[], char **table, ip6tc_handle_t *handle)
 					continue;
 				}
 
-				if (!m)
+				if (!m) {
+					if (c == '?') {
+						if (optopt) {
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "option `%s' "
+							   "requires an "
+							   "argument",
+							   argv[optind-1]);
+						} else {

ip6tables.c int do_command6(int argc, char *argv[], char **table, ip6tc_handle_t *handle) · line 1910

Quoted 10 lines
ip6tables.c +8 -0
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "unknown option "
+							   "`%s'",
+							   argv[optind-1]);
+						}
+					}
 					exit_error(PARAMETER_PROBLEM,
 						   "Unknown arg `%s'", optarg);
+				}

I think it is time to give attention to CodingStyle section 1:

“The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.”

Maybe this could be put into a separate function somehow than trying to squeeze everything between colums 64 and 80?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#6 lore raw
Quoted 29 lines · Jan Engelhardt
Quoted 15 lines · Pablo Neira Ayuso
ip6tables.c +10 -1
@@ -1888,9 +1888,26 @@ int do_command6(int argc, char *argv[], char **table, ip6tc_handle_t *handle)
 					continue;
 				}
 
-				if (!m)
+				if (!m) {
+					if (c == '?') {
+						if (optopt) {
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "option `%s' "
+							   "requires an "
+							   "argument",
+							   argv[optind-1]);
+						} else {

ip6tables.c int do_command6(int argc, char *argv[], char **table, ip6tc_handle_t *handle) · line 1910

Quoted 10 lines
ip6tables.c +8 -0
+							exit_error(
+							   PARAMETER_PROBLEM,
+							   "unknown option "
+							   "`%s'",
+							   argv[optind-1]);
+						}
+					}
 					exit_error(PARAMETER_PROBLEM,
 						   "Unknown arg `%s'", optarg);
+				}

I think it is time to give attention to CodingStyle section 1:

Quoted 6 lines

“The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.”

Maybe this could be put into a separate function somehow than trying to squeeze everything between colums 64 and 80?

Putting this code into a function will not make any better for the whole section of code under the "default" tag in the switch - which is indeed already horrible as it looks like a real Spanish chorizo [1].

Of course, you are free to send a patch to cleanup not only this part, but the whole default section of the switch. I'd be please to apply it.

[1] http://en.wikipedia.org/wiki/Chorizo

Signature · 5 lines
"Los honestos son inadaptados sociales" -- Les Luthiers
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html