* [RFC] iproute2: Fix meta match u32 with 0xffffffff
@ 2011-04-11 18:52 Stephen Hemminger
2011-04-12 7:56 ` Thomas Graf
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2011-04-11 18:52 UTC (permalink / raw)
To: Thomas Graf; +Cc: netdev
The value 0xffffffff is a valid mask and bstrtoul() would return
ULONG_MAX which was the error value. Resolve the problem by separating
return value and error indication.
---
tc/em_cmp.c | 12 ++++--------
tc/em_meta.c | 9 +++------
tc/em_nbyte.c | 6 ++----
tc/em_u32.c | 19 +++++++++----------
tc/m_ematch.c | 17 +++++++++++------
tc/m_ematch.h | 2 +-
6 files changed, 30 insertions(+), 35 deletions(-)
diff --git a/tc/em_cmp.c b/tc/em_cmp.c
index 6addce0..af3e591 100644
--- a/tc/em_cmp.c
+++ b/tc/em_cmp.c
@@ -69,8 +69,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "cmp: missing argument");
a = bstr_next(a);
- offset = bstrtoul(a);
- if (offset == ULONG_MAX)
+ if (bstrtoul(a, &offset) < 0)
return PARSE_ERR(a, "cmp: invalid offset, " \
"must be numeric");
@@ -82,8 +81,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
layer = parse_layer(a);
if (layer == INT_MAX) {
- layer = bstrtoul(a);
- if (layer == ULONG_MAX)
+ if (bstrtoul(a, &layer) < 0)
return PARSE_ERR(a, "cmp: invalid " \
"layer");
}
@@ -96,8 +94,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "cmp: missing argument");
a = bstr_next(a);
- mask = bstrtoul(a);
- if (mask == ULONG_MAX)
+ if (bstrtoul(a, &mask) < 0)
return PARSE_ERR(a, "cmp: invalid mask");
} else if (!bstrcmp(a, "trans")) {
cmp.flags |= TCF_EM_CMP_TRANS;
@@ -115,8 +112,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "cmp: missing argument");
a = bstr_next(a);
- value = bstrtoul(a);
- if (value == ULONG_MAX)
+ if (bstrtoul(a, &value) < 0)
return PARSE_ERR(a, "cmp: invalid value");
value_present = 1;
diff --git a/tc/em_meta.c b/tc/em_meta.c
index 033e29f..276223a 100644
--- a/tc/em_meta.c
+++ b/tc/em_meta.c
@@ -260,8 +260,7 @@ parse_object(struct bstr *args, struct bstr *arg, struct tcf_meta_val *obj,
return bstr_next(arg);
}
- num = bstrtoul(arg);
- if (num != ULONG_MAX) {
+ if (bstrtoul(arg, &num) < 0) {
obj->kind = TCF_META_TYPE_INT << 12;
obj->kind |= TCF_META_ID_VALUE;
*dst = (unsigned long) num;
@@ -318,8 +317,7 @@ compatible:
}
a = bstr_next(a);
- shift = bstrtoul(a);
- if (shift == ULONG_MAX) {
+ if (bstrtoul(a, &shift) < 0) {
PARSE_ERR(a, "meta: invalid shift, must " \
"be numeric");
return PARSE_FAILURE;
@@ -336,8 +334,7 @@ compatible:
}
a = bstr_next(a);
- mask = bstrtoul(a);
- if (mask == ULONG_MAX) {
+ if (bstrtoul(a, &mask) < 0) {
PARSE_ERR(a, "meta: invalid mask, must be " \
"numeric");
return PARSE_FAILURE;
diff --git a/tc/em_nbyte.c b/tc/em_nbyte.c
index 87f3e9d..9a52ffc 100644
--- a/tc/em_nbyte.c
+++ b/tc/em_nbyte.c
@@ -63,8 +63,7 @@ static int nbyte_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "nbyte: missing argument");
a = bstr_next(a);
- offset = bstrtoul(a);
- if (offset == ULONG_MAX)
+ if (bstrtoul(a, &offset) < 0)
return PARSE_ERR(a, "nbyte: invalid offset, " \
"must be numeric");
@@ -76,8 +75,7 @@ static int nbyte_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
layer = parse_layer(a);
if (layer == INT_MAX) {
- layer = bstrtoul(a);
- if (layer == ULONG_MAX)
+ if (bstrtoul(a, &layer) < 0)
return PARSE_ERR(a, "nbyte: invalid " \
"layer");
}
diff --git a/tc/em_u32.c b/tc/em_u32.c
index 21ed70f..88b5fa1 100644
--- a/tc/em_u32.c
+++ b/tc/em_u32.c
@@ -33,6 +33,7 @@ static void u32_print_usage(FILE *fd)
"Example: u32(u16 0x1122 0xffff at nexthdr+4)\n");
}
+
static int u32_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
struct bstr *args)
{
@@ -62,16 +63,14 @@ static int u32_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
if (a == NULL)
return PARSE_ERR(a, "u32: missing key");
- key = bstrtoul(a);
- if (key == ULONG_MAX)
+ if (bstrtoul(a, &key) < 0)
return PARSE_ERR(a, "u32: invalid key, must be numeric");
a = bstr_next(a);
if (a == NULL)
return PARSE_ERR(a, "u32: missing mask");
- mask = bstrtoul(a);
- if (mask == ULONG_MAX)
+ if (bstrtoul(a, &mask) < 0)
return PARSE_ERR(a, "u32: invalid mask, must be numeric");
a = bstr_next(a);
@@ -92,12 +91,12 @@ static int u32_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
a = bstr_next(a);
if (a == NULL)
return PARSE_ERR(a, "u32: missing offset");
- offset = bstrtoul(a);
- } else
- offset = bstrtoul(a);
-
- if (offset == ULONG_MAX)
- return PARSE_ERR(a, "u32: invalid offset");
+ if (bstrtoul(a, &offset) < 0)
+ return PARSE_ERR(a, "u32: invalid offset");
+ } else {
+ if (bstrtoul(a, &offset) < 0)
+ return PARSE_ERR(a, "u32: invalid offset");
+ }
if (a->next)
return PARSE_ERR(a->next, "u32: unexpected trailer");
diff --git a/tc/m_ematch.c b/tc/m_ematch.c
index 4c3acf8..92600ad 100644
--- a/tc/m_ematch.c
+++ b/tc/m_ematch.c
@@ -510,20 +510,25 @@ struct bstr * bstr_alloc(const char *text)
return b;
}
-unsigned long bstrtoul(const struct bstr *b)
+int bstrtoul(const struct bstr *b, unsigned long *lp)
{
char *inv = NULL;
- unsigned long l;
char buf[b->len+1];
+ if (b->len == 0)
+ return -EINVAL;
+
memcpy(buf, b->data, b->len);
buf[b->len] = '\0';
- l = strtoul(buf, &inv, 0);
- if (l == ULONG_MAX || inv == buf)
- return ULONG_MAX;
+ *lp = strtoul(buf, &inv, 0);
+ if (inv == buf)
+ return -EINVAL;
+
+ if (*lp == ULONG_MAX || errno == ERANGE)
+ return -ERANGE;
- return l;
+ return 0;
}
void bstr_print(FILE *fd, const struct bstr *b, int ascii)
diff --git a/tc/m_ematch.h b/tc/m_ematch.h
index 5036e9b..e676290 100644
--- a/tc/m_ematch.h
+++ b/tc/m_ematch.h
@@ -49,7 +49,7 @@ static inline struct bstr *bstr_next(struct bstr *b)
return b->next;
}
-extern unsigned long bstrtoul(const struct bstr *b);
+extern int bstrtoul(const struct bstr *b, unsigned long *lp);
extern void bstr_print(FILE *fd, const struct bstr *b, int ascii);
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC] iproute2: Fix meta match u32 with 0xffffffff
2011-04-11 18:52 [RFC] iproute2: Fix meta match u32 with 0xffffffff Stephen Hemminger
@ 2011-04-12 7:56 ` Thomas Graf
2011-04-12 15:19 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Graf @ 2011-04-12 7:56 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
On Mon, 2011-04-11 at 11:52 -0700, Stephen Hemminger wrote:
> The value 0xffffffff is a valid mask and bstrtoul() would return
> ULONG_MAX which was the error value. Resolve the problem by separating
> return value and error indication.
>
> -unsigned long bstrtoul(const struct bstr *b)
> +int bstrtoul(const struct bstr *b, unsigned long *lp)
> {
> char *inv = NULL;
> - unsigned long l;
> char buf[b->len+1];
>
> + if (b->len == 0)
> + return -EINVAL;
> +
> memcpy(buf, b->data, b->len);
> buf[b->len] = '\0';
>
> - l = strtoul(buf, &inv, 0);
> - if (l == ULONG_MAX || inv == buf)
> - return ULONG_MAX;
> + *lp = strtoul(buf, &inv, 0);
> + if (inv == buf)
> + return -EINVAL;
> +
> + if (*lp == ULONG_MAX || errno == ERANGE)
> + return -ERANGE;
>
> - return l;
> + return 0;
> }
This is definitely much better but we still can't parse ULONG_MAX
as string representative. Checking glibc docs, the only way to do it is
to ignore the return value for error checking and look errno.
So I guess we should do something like this:
errno = 0;
*lp = strtoul(buf, &inv, 0);
if (*inv != '\0')
return -EINVAL;
else if (errno)
return errno;
return 0;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC] iproute2: Fix meta match u32 with 0xffffffff
2011-04-12 7:56 ` Thomas Graf
@ 2011-04-12 15:19 ` Stephen Hemminger
2011-04-12 17:22 ` Thomas Graf
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2011-04-12 15:19 UTC (permalink / raw)
To: tgraf; +Cc: netdev
On Tue, 12 Apr 2011 09:56:49 +0200
Thomas Graf <tgraf@redhat.com> wrote:
> On Mon, 2011-04-11 at 11:52 -0700, Stephen Hemminger wrote:
> > The value 0xffffffff is a valid mask and bstrtoul() would return
> > ULONG_MAX which was the error value. Resolve the problem by separating
> > return value and error indication.
> >
> > -unsigned long bstrtoul(const struct bstr *b)
> > +int bstrtoul(const struct bstr *b, unsigned long *lp)
> > {
> > char *inv = NULL;
> > - unsigned long l;
> > char buf[b->len+1];
> >
> > + if (b->len == 0)
> > + return -EINVAL;
> > +
> > memcpy(buf, b->data, b->len);
> > buf[b->len] = '\0';
> >
> > - l = strtoul(buf, &inv, 0);
> > - if (l == ULONG_MAX || inv == buf)
> > - return ULONG_MAX;
> > + *lp = strtoul(buf, &inv, 0);
> > + if (inv == buf)
> > + return -EINVAL;
> > +
> > + if (*lp == ULONG_MAX || errno == ERANGE)
> > + return -ERANGE;
> >
> > - return l;
> > + return 0;
> > }
>
> This is definitely much better but we still can't parse ULONG_MAX
> as string representative. Checking glibc docs, the only way to do it is
> to ignore the return value for error checking and look errno.
>
I think the error case is ret == ULONG_MAX && errno == ERANGE
If there is no error, then strtoul doesn't set errno.
--
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC] iproute2: Fix meta match u32 with 0xffffffff
2011-04-12 15:19 ` Stephen Hemminger
@ 2011-04-12 17:22 ` Thomas Graf
2011-04-12 17:31 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Graf @ 2011-04-12 17:22 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
On Tue, 2011-04-12 at 08:19 -0700, Stephen Hemminger wrote:
> >
> > This is definitely much better but we still can't parse ULONG_MAX
> > as string representative. Checking glibc docs, the only way to do it is
> > to ignore the return value for error checking and look errno.
> >
>
> I think the error case is ret == ULONG_MAX && errno == ERANGE
> If there is no error, then strtoul doesn't set errno.
That's ok too but your patch adds ret == ULONG_MAX || errno == ERANGE
which will not allow to parse ULONG_MAX as a string. You probably
still have to clear errno to 0 before calling strtoul in case
ULONG_MAX is meant as legitimate return value but a previous glibc
function has left errno set to ERANGE.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] iproute2: Fix meta match u32 with 0xffffffff
2011-04-12 17:22 ` Thomas Graf
@ 2011-04-12 17:31 ` Stephen Hemminger
0 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2011-04-12 17:31 UTC (permalink / raw)
To: tgraf; +Cc: netdev
On Tue, 12 Apr 2011 19:22:05 +0200
Thomas Graf <tgraf@redhat.com> wrote:
> On Tue, 2011-04-12 at 08:19 -0700, Stephen Hemminger wrote:
>
> > >
> > > This is definitely much better but we still can't parse ULONG_MAX
> > > as string representative. Checking glibc docs, the only way to do it is
> > > to ignore the return value for error checking and look errno.
> > >
> >
> > I think the error case is ret == ULONG_MAX && errno == ERANGE
> > If there is no error, then strtoul doesn't set errno.
>
> That's ok too but your patch adds ret == ULONG_MAX || errno == ERANGE
> which will not allow to parse ULONG_MAX as a string. You probably
> still have to clear errno to 0 before calling strtoul in case
> ULONG_MAX is meant as legitimate return value but a previous glibc
> function has left errno set to ERANGE.
>
I changed to && in final version.
Still needs more work because most of the code assumes that
unsigned long can be used for mask etc, when infact the code only
allows for u32.
>From 63da1ed89970af18383c7318deb38bdb062fe74e Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 11 Apr 2011 11:48:55 -0700
Subject: [PATCH 1/2] Fix meta match u32 with 0xffffffff
The value 0xffffffff is a valid mask and bstrtoul() would return
ULONG_MAX which was the error value.
---
tc/em_cmp.c | 12 ++++--------
tc/em_meta.c | 9 +++------
tc/em_nbyte.c | 6 ++----
tc/em_u32.c | 19 +++++++++----------
tc/m_ematch.c | 17 +++++++++++------
tc/m_ematch.h | 2 +-
6 files changed, 30 insertions(+), 35 deletions(-)
diff --git a/tc/em_cmp.c b/tc/em_cmp.c
index 6addce0..af3e591 100644
--- a/tc/em_cmp.c
+++ b/tc/em_cmp.c
@@ -69,8 +69,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "cmp: missing argument");
a = bstr_next(a);
- offset = bstrtoul(a);
- if (offset == ULONG_MAX)
+ if (bstrtoul(a, &offset) < 0)
return PARSE_ERR(a, "cmp: invalid offset, " \
"must be numeric");
@@ -82,8 +81,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
layer = parse_layer(a);
if (layer == INT_MAX) {
- layer = bstrtoul(a);
- if (layer == ULONG_MAX)
+ if (bstrtoul(a, &layer) < 0)
return PARSE_ERR(a, "cmp: invalid " \
"layer");
}
@@ -96,8 +94,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "cmp: missing argument");
a = bstr_next(a);
- mask = bstrtoul(a);
- if (mask == ULONG_MAX)
+ if (bstrtoul(a, &mask) < 0)
return PARSE_ERR(a, "cmp: invalid mask");
} else if (!bstrcmp(a, "trans")) {
cmp.flags |= TCF_EM_CMP_TRANS;
@@ -115,8 +112,7 @@ static int cmp_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "cmp: missing argument");
a = bstr_next(a);
- value = bstrtoul(a);
- if (value == ULONG_MAX)
+ if (bstrtoul(a, &value) < 0)
return PARSE_ERR(a, "cmp: invalid value");
value_present = 1;
diff --git a/tc/em_meta.c b/tc/em_meta.c
index 033e29f..276223a 100644
--- a/tc/em_meta.c
+++ b/tc/em_meta.c
@@ -260,8 +260,7 @@ parse_object(struct bstr *args, struct bstr *arg, struct tcf_meta_val *obj,
return bstr_next(arg);
}
- num = bstrtoul(arg);
- if (num != ULONG_MAX) {
+ if (bstrtoul(arg, &num) < 0) {
obj->kind = TCF_META_TYPE_INT << 12;
obj->kind |= TCF_META_ID_VALUE;
*dst = (unsigned long) num;
@@ -318,8 +317,7 @@ compatible:
}
a = bstr_next(a);
- shift = bstrtoul(a);
- if (shift == ULONG_MAX) {
+ if (bstrtoul(a, &shift) < 0) {
PARSE_ERR(a, "meta: invalid shift, must " \
"be numeric");
return PARSE_FAILURE;
@@ -336,8 +334,7 @@ compatible:
}
a = bstr_next(a);
- mask = bstrtoul(a);
- if (mask == ULONG_MAX) {
+ if (bstrtoul(a, &mask) < 0) {
PARSE_ERR(a, "meta: invalid mask, must be " \
"numeric");
return PARSE_FAILURE;
diff --git a/tc/em_nbyte.c b/tc/em_nbyte.c
index 87f3e9d..9a52ffc 100644
--- a/tc/em_nbyte.c
+++ b/tc/em_nbyte.c
@@ -63,8 +63,7 @@ static int nbyte_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
return PARSE_ERR(a, "nbyte: missing argument");
a = bstr_next(a);
- offset = bstrtoul(a);
- if (offset == ULONG_MAX)
+ if (bstrtoul(a, &offset) < 0)
return PARSE_ERR(a, "nbyte: invalid offset, " \
"must be numeric");
@@ -76,8 +75,7 @@ static int nbyte_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
layer = parse_layer(a);
if (layer == INT_MAX) {
- layer = bstrtoul(a);
- if (layer == ULONG_MAX)
+ if (bstrtoul(a, &layer) < 0)
return PARSE_ERR(a, "nbyte: invalid " \
"layer");
}
diff --git a/tc/em_u32.c b/tc/em_u32.c
index 21ed70f..88b5fa1 100644
--- a/tc/em_u32.c
+++ b/tc/em_u32.c
@@ -33,6 +33,7 @@ static void u32_print_usage(FILE *fd)
"Example: u32(u16 0x1122 0xffff at nexthdr+4)\n");
}
+
static int u32_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
struct bstr *args)
{
@@ -62,16 +63,14 @@ static int u32_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
if (a == NULL)
return PARSE_ERR(a, "u32: missing key");
- key = bstrtoul(a);
- if (key == ULONG_MAX)
+ if (bstrtoul(a, &key) < 0)
return PARSE_ERR(a, "u32: invalid key, must be numeric");
a = bstr_next(a);
if (a == NULL)
return PARSE_ERR(a, "u32: missing mask");
- mask = bstrtoul(a);
- if (mask == ULONG_MAX)
+ if (bstrtoul(a, &mask) < 0)
return PARSE_ERR(a, "u32: invalid mask, must be numeric");
a = bstr_next(a);
@@ -92,12 +91,12 @@ static int u32_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
a = bstr_next(a);
if (a == NULL)
return PARSE_ERR(a, "u32: missing offset");
- offset = bstrtoul(a);
- } else
- offset = bstrtoul(a);
-
- if (offset == ULONG_MAX)
- return PARSE_ERR(a, "u32: invalid offset");
+ if (bstrtoul(a, &offset) < 0)
+ return PARSE_ERR(a, "u32: invalid offset");
+ } else {
+ if (bstrtoul(a, &offset) < 0)
+ return PARSE_ERR(a, "u32: invalid offset");
+ }
if (a->next)
return PARSE_ERR(a->next, "u32: unexpected trailer");
diff --git a/tc/m_ematch.c b/tc/m_ematch.c
index 4c3acf8..b7eed18 100644
--- a/tc/m_ematch.c
+++ b/tc/m_ematch.c
@@ -510,20 +510,25 @@ struct bstr * bstr_alloc(const char *text)
return b;
}
-unsigned long bstrtoul(const struct bstr *b)
+int bstrtoul(const struct bstr *b, unsigned long *lp)
{
char *inv = NULL;
- unsigned long l;
char buf[b->len+1];
+ if (b->len == 0)
+ return -EINVAL;
+
memcpy(buf, b->data, b->len);
buf[b->len] = '\0';
- l = strtoul(buf, &inv, 0);
- if (l == ULONG_MAX || inv == buf)
- return ULONG_MAX;
+ *lp = strtoul(buf, &inv, 0);
+ if (inv == buf)
+ return -EINVAL;
+
+ if (*lp == ULONG_MAX && errno == ERANGE)
+ return -ERANGE;
- return l;
+ return 0;
}
void bstr_print(FILE *fd, const struct bstr *b, int ascii)
diff --git a/tc/m_ematch.h b/tc/m_ematch.h
index 5036e9b..e676290 100644
--- a/tc/m_ematch.h
+++ b/tc/m_ematch.h
@@ -49,7 +49,7 @@ static inline struct bstr *bstr_next(struct bstr *b)
return b->next;
}
-extern unsigned long bstrtoul(const struct bstr *b);
+extern int bstrtoul(const struct bstr *b, unsigned long *lp);
extern void bstr_print(FILE *fd, const struct bstr *b, int ascii);
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-04-12 17:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-11 18:52 [RFC] iproute2: Fix meta match u32 with 0xffffffff Stephen Hemminger
2011-04-12 7:56 ` Thomas Graf
2011-04-12 15:19 ` Stephen Hemminger
2011-04-12 17:22 ` Thomas Graf
2011-04-12 17:31 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).