[PATCH] evaluate: print error for null string befort assert statement

[PATCH] evaluate: print error for null string befort assert statement

[Harsha Sharma]

Print error "Null string is not allowed" before assert statement.
For e.g.
nft add rule filter input meta iifname '""'
Error: Null String is not allowed
add rule filter input meta iifname ""

Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
---
 src/evaluate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index fd61e75..ad044a4 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -235,6 +235,10 @@ static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 	memset(data + len, 0, data_len - len);
 	mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len);
 
+	if (strlen(data) == 0) {
+		return expr_error(ctx->msgs, expr,
+				  "Null String is not allowed");
+	}
 	assert(strlen(data) > 0);
 	datalen = strlen(data) - 1;
 	if (data[datalen] != '*') {
-- 
2.11.0

Re: [PATCH] evaluate: print error for null string befort assert statement

[Florian Westphal]

Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
> Print error "Null string is not allowed" before assert statement.
> For e.g.
> nft add rule filter input meta iifname '""'
> Error: Null String is not allowed
> add rule filter input meta iifname ""

Is there any case where "" should be allowed?

If not, I'd rather change scanner.l to not recognize "" as
a quoted string.

Re: [PATCH] evaluate: print error for null string befort assert statement

[Pablo Neira Ayuso]

On Thu, Nov 23, 2017 at 10:25:51PM +0100, Florian Westphal wrote:
> Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
> > Print error "Null string is not allowed" before assert statement.
> > For e.g.
> > nft add rule filter input meta iifname '""'
> > Error: Null String is not allowed
> > add rule filter input meta iifname ""
> 
> Is there any case where "" should be allowed?
> 
> If not, I'd rather change scanner.l to not recognize "" as
> a quoted string.

We'll get error reporting like this if we handle this from the
scanner:

# nft add rule x y ct label \"\"
Error: syntax error, unexpected junk
add rule x y ct label ""
                      ^

This is just two extra lines in the validation step and it is not
worth to optimize an error case, ie. do it earlier. So I'm feeling
inclined to take this one.

I will mangle the error message to "Empty string is not allowed".
"Null" probably sounds too programmer thing.

Thanks!

Re: [PATCH] evaluate: print error for null string befort assert statement

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Nov 23, 2017 at 10:25:51PM +0100, Florian Westphal wrote:
> > Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
> > > Print error "Null string is not allowed" before assert statement.
> > > For e.g.
> > > nft add rule filter input meta iifname '""'
> > > Error: Null String is not allowed
> > > add rule filter input meta iifname ""
> > 
> > Is there any case where "" should be allowed?
> > 
> > If not, I'd rather change scanner.l to not recognize "" as
> > a quoted string.
> 
> We'll get error reporting like this if we handle this from the
> scanner:
> 
> # nft add rule x y ct label \"\"
> Error: syntax error, unexpected junk
> add rule x y ct label ""

Right, we'd have to add dummy empty_string rule to avoid this.

> This is just two extra lines in the validation step and it is not
> worth to optimize an error case, ie. do it earlier. So I'm feeling
> inclined to take this one.

Makes sense to me.

> I will mangle the error message to "Empty string is not allowed".
> "Null" probably sounds too programmer thing.

Can you also get rid of the assert(strlen ... line?
Its useless after this patch.

Thanks!

Re: [PATCH] evaluate: print error for null string befort assert statement

[Pablo Neira Ayuso]

On Mon, Nov 27, 2017 at 11:48:03PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Nov 23, 2017 at 10:25:51PM +0100, Florian Westphal wrote:
> > > Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
> > > > Print error "Null string is not allowed" before assert statement.
> > > > For e.g.
> > > > nft add rule filter input meta iifname '""'
> > > > Error: Null String is not allowed
> > > > add rule filter input meta iifname ""
> > > 
> > > Is there any case where "" should be allowed?
> > > 
> > > If not, I'd rather change scanner.l to not recognize "" as
> > > a quoted string.
> > 
> > We'll get error reporting like this if we handle this from the
> > scanner:
> > 
> > # nft add rule x y ct label \"\"
> > Error: syntax error, unexpected junk
> > add rule x y ct label ""
> 
> Right, we'd have to add dummy empty_string rule to avoid this.
> 
> > This is just two extra lines in the validation step and it is not
> > worth to optimize an error case, ie. do it earlier. So I'm feeling
> > inclined to take this one.
> 
> Makes sense to me.
> 
> > I will mangle the error message to "Empty string is not allowed".
> > "Null" probably sounds too programmer thing.
> 
> Can you also get rid of the assert(strlen ... line?
> Its useless after this patch.

Indeed, done!