[nft PATCH] evaluate: better error reporting in too long sets names

[nft PATCH] evaluate: better error reporting in too long sets names

[Arturo Borrero Gonzalez]

Currently, if we choose a set name larger than allowed, the error message is:
 Error: Could not process rule: Numerical result out of range

Let's inform the user with a better error message.

We can discuss later if length of set names should be increased, but I think
this better error reporting is necessary right now to avoid headaches to users.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/evaluate.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index 346e34f..b86e5b6 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2123,6 +2123,7 @@ static int setelem_evaluate(struct eval_ctx *ctx, struct expr **expr)
 
 static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 {
+	size_t namelen = IFNAMSIZ - 1;
 	struct table *table;
 	const char *type;
 
@@ -2136,6 +2137,10 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 
 	type = set->flags & SET_F_MAP ? "map" : "set";
 
+	if (strlen(set->handle.set) > namelen)
+		return cmd_error(ctx, "%s maximum allowed name length is %lu",
+				 type, namelen);
+
 	if (set->keytype == NULL)
 		return set_error(ctx, set, "%s definition does not specify "
 				 "key data type", type);

Re: [nft PATCH] evaluate: better error reporting in too long sets names

[Pablo Neira Ayuso]

On Wed, Apr 20, 2016 at 03:43:00PM +0200, Arturo Borrero Gonzalez wrote:
> Currently, if we choose a set name larger than allowed, the error message is:
>  Error: Could not process rule: Numerical result out of range
> 
> Let's inform the user with a better error message.
> 
> We can discuss later if length of set names should be increased, but I think
> this better error reporting is necessary right now to avoid headaches to users.

/* The max length of strings including NUL: set and type identifiers */
#define IPSET_MAXNAMELEN        32

I would like that we get the same length as ipset, this should make it
easier for people to migrate.

This would require a bit of work though since the interface name size
is limited by the register size. Not much a problem, but it would
require a bit of code adjustments from the kernel.

So let me postpone this userspace check.

Re: [nft PATCH] evaluate: better error reporting in too long sets names

[Jozsef Kadlecsik]

On Wed, 27 Apr 2016, Pablo Neira Ayuso wrote:

> On Wed, Apr 20, 2016 at 03:43:00PM +0200, Arturo Borrero Gonzalez wrote:
> > Currently, if we choose a set name larger than allowed, the error message is:
> >  Error: Could not process rule: Numerical result out of range
> > 
> > Let's inform the user with a better error message.
> > 
> > We can discuss later if length of set names should be increased, but I think
> > this better error reporting is necessary right now to avoid headaches to users.
> 
> /* The max length of strings including NUL: set and type identifiers */
> #define IPSET_MAXNAMELEN        32
> 
> I would like that we get the same length as ipset, this should make it
> easier for people to migrate.

I think it's all right if set names are longer in nftables. That won't 
cause incompatibilites, unless someone wants to move from nftables to 
ipset.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [nft PATCH] evaluate: better error reporting in too long sets names

[Pablo Neira Ayuso]

On Wed, Apr 27, 2016 at 07:36:38PM +0200, Jozsef Kadlecsik wrote:
> On Wed, 27 Apr 2016, Pablo Neira Ayuso wrote:
> 
> > On Wed, Apr 20, 2016 at 03:43:00PM +0200, Arturo Borrero Gonzalez wrote:
> > > Currently, if we choose a set name larger than allowed, the error message is:
> > >  Error: Could not process rule: Numerical result out of range
> > > 
> > > Let's inform the user with a better error message.
> > > 
> > > We can discuss later if length of set names should be increased, but I think
> > > this better error reporting is necessary right now to avoid headaches to users.
> > 
> > /* The max length of strings including NUL: set and type identifiers */
> > #define IPSET_MAXNAMELEN        32
> > 
> > I would like that we get the same length as ipset, this should make it
> > easier for people to migrate.
> 
> I think it's all right if set names are longer in nftables. That won't 
> cause incompatibilites, unless someone wants to move from nftables to 
> ipset.

Currently in nftables we have 16 bytes, so we're smaller than ipset.

Re: [nft PATCH] evaluate: better error reporting in too long sets names

[Arturo Borrero Gonzalez]

On 27 April 2016 at 19:14, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Apr 20, 2016 at 03:43:00PM +0200, Arturo Borrero Gonzalez wrote:
>> Currently, if we choose a set name larger than allowed, the error message is:
>>  Error: Could not process rule: Numerical result out of range
>>
>> Let's inform the user with a better error message.
>>
>> We can discuss later if length of set names should be increased, but I think
>> this better error reporting is necessary right now to avoid headaches to users.
>
> /* The max length of strings including NUL: set and type identifiers */
> #define IPSET_MAXNAMELEN        32
>
> I would like that we get the same length as ipset, this should make it
> easier for people to migrate.
>
> This would require a bit of work though since the interface name size
> is limited by the register size. Not much a problem, but it would
> require a bit of code adjustments from the kernel.
>
> So let me postpone this userspace check.

We would need the userspace check anyway to avoid the very misleading
error reporting from the kernel.
Then if tomorrow we change the name length, we just need an oneliner
here to update with the new size. I remember in the past we discussed
using set names as completely variable size strings, but that's
another discussion.

If loading a ruleset with 'nft -f' with lot of nested and included
files, the error is just very difficult to track down; I've hit it
several times already.

== t.nft ==
flush ruleset
table t {
    set abcdefghijklmopqrst {
         type ipv4_addr
    }
}
===========

% nft -f t.nft
t.nft:2:1-2: Error: Could not process rule: Numerical result out of range
table t {
^^

So I don't understand the point in not including some more informative
message right now.
-- 
Arturo Borrero González
--
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: [nft PATCH] evaluate: better error reporting in too long sets names

[Arturo Borrero Gonzalez]

On 28 April 2016 at 09:42, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> On 27 April 2016 at 19:14, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Wed, Apr 20, 2016 at 03:43:00PM +0200, Arturo Borrero Gonzalez wrote:
>>> Currently, if we choose a set name larger than allowed, the error message is:
>>>  Error: Could not process rule: Numerical result out of range
>>>
>>> Let's inform the user with a better error message.
>>>
>>> We can discuss later if length of set names should be increased, but I think
>>> this better error reporting is necessary right now to avoid headaches to users.
>>
>> /* The max length of strings including NUL: set and type identifiers */
>> #define IPSET_MAXNAMELEN        32
>>
>> I would like that we get the same length as ipset, this should make it
>> easier for people to migrate.
>>
>> This would require a bit of work though since the interface name size
>> is limited by the register size. Not much a problem, but it would
>> require a bit of code adjustments from the kernel.
>>
>> So let me postpone this userspace check.
>
> We would need the userspace check anyway to avoid the very misleading
> error reporting from the kernel.
> Then if tomorrow we change the name length, we just need an oneliner
> here to update with the new size. I remember in the past we discussed
> using set names as completely variable size strings, but that's
> another discussion.
>
> If loading a ruleset with 'nft -f' with lot of nested and included
> files, the error is just very difficult to track down; I've hit it
> several times already.
>
> == t.nft ==
> flush ruleset
> table t {
>     set abcdefghijklmopqrst {
>          type ipv4_addr
>     }
> }
> ===========
>
> % nft -f t.nft
> t.nft:2:1-2: Error: Could not process rule: Numerical result out of range
> table t {
> ^^
>
> So I don't understand the point in not including some more informative
> message right now.
> --


ping
-- 
Arturo Borrero González
--
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