Re: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
On Thu, Nov 24, 2016 at 09:00:51PM +0100, Christopher Faulet wrote: > New patch attached. thanks. Applied now, thanks! Willy
Re: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
Le 24/11/2016 à 19:50, Willy Tarreau a écrit : Hi Christopher, On Thu, Nov 24, 2016 at 03:06:13PM +0100, Christopher Faulet wrote: >From 7ed3c2942d57ea2ddfc8973cce9cc1c94bca01da Mon Sep 17 00:00:00 2001 From: Christopher Faulet <cfau...@haproxy.com> Date: Thu, 24 Nov 2016 14:53:22 +0100 Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4 For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not correctly updated when the 3rd argument was parsed. Are you sure your patch is correct ? I think it's bogus : diff --git a/src/flt_spoe.c b/src/flt_spoe.c index 0b722b6..8227140 100644 --- a/src/flt_spoe.c +++ b/src/flt_spoe.c @@ -2017,7 +2017,7 @@ process_spoe_actions(struct stream *s, struct spoe_context *ctx, goto skip; memset(, 0, sizeof(smp)); smp_set_owner(, s->be, s->sess, s, dir|SMP_OPT_FINAL); - if (decode_spoe_data(p+idx, p+size, ) == -1) + if ((idx += decode_spoe_data(p+idx, p+size, )) == -1) goto skip; The only case it will work is when idx = 0 before decoding, which doesn't really look like the only case you're interested in. I guess you wanted to do this instead : - if (decode_spoe_data(p+idx, p+size, ) == -1) + ret = decode_spoe_data(p+idx, p+size, ); + if (ret == -1) goto skip; + idx += ret; Am I wrong ? That's the reason why I hate assignments in "if" conditions, half of the time they are bogus, the other half they make the reader scratch his head wondering if it's bogus or intended :-) Ah of course you're right ! too tired... New patch attached. thanks. -- Christopher >From 1b46491bf93c76602122ea5814c42fdcfbf2d816 Mon Sep 17 00:00:00 2001 From: Christopher Faulet <cfau...@haproxy.com> Date: Thu, 24 Nov 2016 14:53:22 +0100 Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4 For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not correctly updated when the 3rd argument was parsed. --- src/flt_spoe.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/flt_spoe.c b/src/flt_spoe.c index 0b722b6..776848e 100644 --- a/src/flt_spoe.c +++ b/src/flt_spoe.c @@ -2017,8 +2017,10 @@ process_spoe_actions(struct stream *s, struct spoe_context *ctx, goto skip; memset(, 0, sizeof(smp)); smp_set_owner(, s->be, s->sess, s, dir|SMP_OPT_FINAL); -if (decode_spoe_data(p+idx, p+size, ) == -1) + +if ((i = decode_spoe_data(p+idx, p+size, )) == -1) goto skip; +idx += i; SPOE_PRINTF(stderr, "%d.%06d [SPOE/%-15s] %s: stream=%p" " - set-var '%s.%s.%.*s'\n", -- 2.7.4
Re: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
Hi Christopher, On Thu, Nov 24, 2016 at 03:06:13PM +0100, Christopher Faulet wrote: > >From 7ed3c2942d57ea2ddfc8973cce9cc1c94bca01da Mon Sep 17 00:00:00 2001 > From: Christopher Faulet <cfau...@haproxy.com> > Date: Thu, 24 Nov 2016 14:53:22 +0100 > Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames > X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4 > > For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not > correctly updated when the 3rd argument was parsed. Are you sure your patch is correct ? I think it's bogus : > diff --git a/src/flt_spoe.c b/src/flt_spoe.c > index 0b722b6..8227140 100644 > --- a/src/flt_spoe.c > +++ b/src/flt_spoe.c > @@ -2017,7 +2017,7 @@ process_spoe_actions(struct stream *s, struct > spoe_context *ctx, > goto skip; > memset(, 0, sizeof(smp)); > smp_set_owner(, s->be, s->sess, s, > dir|SMP_OPT_FINAL); > - if (decode_spoe_data(p+idx, p+size, ) == -1) > + if ((idx += decode_spoe_data(p+idx, p+size, > )) == -1) > goto skip; The only case it will work is when idx = 0 before decoding, which doesn't really look like the only case you're interested in. I guess you wanted to do this instead : - if (decode_spoe_data(p+idx, p+size, ) == -1) + ret = decode_spoe_data(p+idx, p+size, ); + if (ret == -1) goto skip; + idx += ret; Am I wrong ? That's the reason why I hate assignments in "if" conditions, half of the time they are bogus, the other half they make the reader scratch his head wondering if it's bogus or intended :-) Willy
[PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
Hi, Here is a small bug fix on SPOE filter. -- Christopher >From 7ed3c2942d57ea2ddfc8973cce9cc1c94bca01da Mon Sep 17 00:00:00 2001 From: Christopher Faulet <cfau...@haproxy.com> Date: Thu, 24 Nov 2016 14:53:22 +0100 Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4 For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not correctly updated when the 3rd argument was parsed. --- src/flt_spoe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/flt_spoe.c b/src/flt_spoe.c index 0b722b6..8227140 100644 --- a/src/flt_spoe.c +++ b/src/flt_spoe.c @@ -2017,7 +2017,7 @@ process_spoe_actions(struct stream *s, struct spoe_context *ctx, goto skip; memset(, 0, sizeof(smp)); smp_set_owner(, s->be, s->sess, s, dir|SMP_OPT_FINAL); -if (decode_spoe_data(p+idx, p+size, ) == -1) +if ((idx += decode_spoe_data(p+idx, p+size, )) == -1) goto skip; SPOE_PRINTF(stderr, "%d.%06d [SPOE/%-15s] %s: stream=%p" -- 2.7.4
