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.000000, 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(&smp, 0, sizeof(smp));
> smp_set_owner(&smp, s->be, s->sess, s,
> dir|SMP_OPT_FINAL);
> - if (decode_spoe_data(p+idx, p+size, &smp) == -1)
> + if ((idx += decode_spoe_data(p+idx, p+size,
> &smp)) == -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, &smp) == -1)
+ ret = decode_spoe_data(p+idx, p+size, &smp);
+ 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
