diff mbox series

[nft,3/3] netlink_linearize: avoid strict-overflow warning in netlink_gen_bitwise()

Message ID 20230927122744.3434851-4-thaller@redhat.com
State Accepted, archived
Headers show
Series Two fixes to avoid "-Wstrict-overflow" warnings | expand

Commit Message

Thomas Haller Sept. 27, 2023, 12:23 p.m. UTC
With gcc-13.2.1-1.fc38.x86_64:

  $ gcc -Iinclude -c -o tmp.o src/netlink_linearize.c -Werror -Wstrict-overflow=5 -O3
  src/netlink_linearize.c: In function ‘netlink_gen_bitwise’:
  src/netlink_linearize.c:1790:1: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
   1790 | }
        | ^
  cc1: all warnings being treated as errors

It also makes more sense this way, where "n" is the hight of the
"binops" stack, and we check for a non-empty stack with "n > 0" and pop
the last element with "binops[--n]".

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/netlink_linearize.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Thomas Haller Sept. 27, 2023, 5:06 p.m. UTC | #1
On Wed, 2023-09-27 at 14:23 +0200, Thomas Haller wrote:
> With gcc-13.2.1-1.fc38.x86_64:
> 
>   $ gcc -Iinclude -c -o tmp.o src/netlink_linearize.c -Werror -
> Wstrict-overflow=5 -O3
>   src/netlink_linearize.c: In function ‘netlink_gen_bitwise’:
>   src/netlink_linearize.c:1790:1: error: assuming signed overflow
> does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> Werror=strict-overflow]
>    1790 | }
>         | ^
>   cc1: all warnings being treated as errors
> 
> It also makes more sense this way, where "n" is the hight of the
> "binops" stack, and we check for a non-empty stack with "n > 0" and
> pop
> the last element with "binops[--n]".
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  src/netlink_linearize.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> index c91211582b3d..f2514b012a9d 100644
> --- a/src/netlink_linearize.c
> +++ b/src/netlink_linearize.c
> @@ -712,14 +712,13 @@ static void netlink_gen_bitwise(struct
> netlink_linearize_ctx *ctx,
>         while (left->etype == EXPR_BINOP && left->left != NULL &&
>                (left->op == OP_AND || left->op == OP_OR || left->op
> == OP_XOR))
>                 binops[n++] = left = left->left;

I wanted to ask, what ensures that binops buffer does not overflow?


> -       n--;
>  
> -       netlink_gen_expr(ctx, binops[n--], dreg);
> +       netlink_gen_expr(ctx, binops[--n], dreg);
>  
>         mpz_bitmask(mask, expr->len);
>         mpz_set_ui(xor, 0);
> -       for (; n >= 0; n--) {
> -               i = binops[n];
> +       while (n > 0) {
> +               i = binops[--n];
>                 mpz_set(val, i->right->value);
>  
>                 switch (i->op) {
Pablo Neira Ayuso Sept. 27, 2023, 5:14 p.m. UTC | #2
On Wed, Sep 27, 2023 at 07:06:26PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 14:23 +0200, Thomas Haller wrote:
> > With gcc-13.2.1-1.fc38.x86_64:
> > 
> >   $ gcc -Iinclude -c -o tmp.o src/netlink_linearize.c -Werror -
> > Wstrict-overflow=5 -O3
> >   src/netlink_linearize.c: In function ‘netlink_gen_bitwise’:
> >   src/netlink_linearize.c:1790:1: error: assuming signed overflow
> > does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >    1790 | }
> >         | ^
> >   cc1: all warnings being treated as errors
> > 
> > It also makes more sense this way, where "n" is the hight of the
> > "binops" stack, and we check for a non-empty stack with "n > 0" and
> > pop
> > the last element with "binops[--n]".
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  src/netlink_linearize.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> > index c91211582b3d..f2514b012a9d 100644
> > --- a/src/netlink_linearize.c
> > +++ b/src/netlink_linearize.c
> > @@ -712,14 +712,13 @@ static void netlink_gen_bitwise(struct
> > netlink_linearize_ctx *ctx,
> >         while (left->etype == EXPR_BINOP && left->left != NULL &&
> >                (left->op == OP_AND || left->op == OP_OR || left->op
> > == OP_XOR))
> >                 binops[n++] = left = left->left;
> 
> I wanted to ask, what ensures that binops buffer does not overflow?

This is stacking binops, then popping them out one by one to generate
code IIRC.

binops has 16 positions, if you manage to generate a large expression
with lots of bitwise, probably you can hit the buffer overflow.

Go explore :)
diff mbox series

Patch

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index c91211582b3d..f2514b012a9d 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -712,14 +712,13 @@  static void netlink_gen_bitwise(struct netlink_linearize_ctx *ctx,
 	while (left->etype == EXPR_BINOP && left->left != NULL &&
 	       (left->op == OP_AND || left->op == OP_OR || left->op == OP_XOR))
 		binops[n++] = left = left->left;
-	n--;
 
-	netlink_gen_expr(ctx, binops[n--], dreg);
+	netlink_gen_expr(ctx, binops[--n], dreg);
 
 	mpz_bitmask(mask, expr->len);
 	mpz_set_ui(xor, 0);
-	for (; n >= 0; n--) {
-		i = binops[n];
+	while (n > 0) {
+		i = binops[--n];
 		mpz_set(val, i->right->value);
 
 		switch (i->op) {