Re: [PATCH] BUG: payload: fix payload not retrieving arbitrary lengths

[Felipe Guerreiro Barbosa Ruiz]

Ok, that's brilliant! Thanks a lot!

Cheers,
Felipe

On 21 March 2017 at 11:48, Willy Tarreau  wrote:

> On Tue, Mar 21, 2017 at 11:26:15AM -0300, Felipe Guerreiro Barbosa Ruiz
> wrote:
> > Hi Willy,
> >
> > Thanks for fixing it. I guess pasting from the terminal straight to gmail
> > might have caused that. Next time I'll attach it, for sure!
> >
> > By the way, I noticed the patch was only applied to the latest
> development
> > version, but not to 1.6 or 1.7. I'm not sure if it will apply cleanly,
> but
> > I can create a patch for each version, if necessary.
>
> Don't worry, I'll backport all pending fixes to stable versions. For now
> I'm
> focusing on finishing to merge the remaining pending fixes in order to
> prepare
> a new version.
>
> Willy
>

Re: [PATCH] BUG: payload: fix payload not retrieving arbitrary lengths

[Felipe Guerreiro Barbosa Ruiz]

Hi Willy,

Thanks for fixing it. I guess pasting from the terminal straight to gmail
might have caused that. Next time I'll attach it, for sure!

By the way, I noticed the patch was only applied to the latest development
version, but not to 1.6 or 1.7. I'm not sure if it will apply cleanly, but
I can create a patch for each version, if necessary.

Cheers,
Felipe

On 20 March 2017 at 03:30, Willy Tarreau  wrote:

> Hi Felipe,
>
> On Thu, Mar 16, 2017 at 05:01:41PM -0300, Felipe Guerreiro Barbosa Ruiz
> wrote:
> > This fixes a regression introduced in d7bdcb874bcb, that removed the
> > ability to use req.payload(0,0) to read the whole buffer content. The
> > offending commit is present starting in version 1.6, so the patch
> > should be backported to versions 1.6 and 1.7.
>
> All useful indications are provided, that's perfect, I've applied it now.
> Thank you very much Felipe. BTW, just for your information, the patch was
> mangled by your mailer (lines were wrapped) but since it was easy I could
> fix it by hand :
>
> > diff --git a/src/payload.c b/src/payload.c
> > index a02a8696..b80a19c9 100644
> > --- a/src/payload.c
> > +++ b/src/payload.c
> > @@ -838,7 +838,7 @@ smp_fetch_payload(const struct arg *arg_p, struct
> > sample *smp, const char *kw, v
> > return 0;
> >
> > chn = ((smp->opt & SMP_OPT_DIR) == SMP_OPT_DIR_RES) ?
> > &smp->strm->res : &smp->strm->req;
> > -   if (!buf_size || buf_size > global.tune.bufsize || buf_offset +
> > buf_size > global.tune.bufsize) {
> > +   if (buf_size > global.tune.bufsize || buf_offset + buf_size >
> > global.tune.bufsize) {
> > /* will never match */
> > smp->flags = 0;
> > return 0;
>
> Next time, in order to avoid this you can attach the patch to your mail,
> mail agents generally don't touch attachments.
>
> Thanks,
> Willy
>

[PATCH] BUG: payload: fix payload not retrieving arbitrary lengths

[Felipe Guerreiro Barbosa Ruiz]

This fixes a regression introduced in d7bdcb874bcb, that removed the
ability to use req.payload(0,0) to read the whole buffer content. The
offending commit is present starting in version 1.6, so the patch
should be backported to versions 1.6 and 1.7.
---
 src/payload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/payload.c b/src/payload.c
index a02a8696..b80a19c9 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -838,7 +838,7 @@ smp_fetch_payload(const struct arg *arg_p, struct
sample *smp, const char *kw, v
return 0;

chn = ((smp->opt & SMP_OPT_DIR) == SMP_OPT_DIR_RES) ?
&smp->strm->res : &smp->strm->req;
-   if (!buf_size || buf_size > global.tune.bufsize || buf_offset +
buf_size > global.tune.bufsize) {
+   if (buf_size > global.tune.bufsize || buf_offset + buf_size >
global.tune.bufsize) {
/* will never match */
smp->flags = 0;
return 0;

Re: Possible regression on HAProxy 1.6, related to ACLs and dynamic payload buffers

[Felipe Guerreiro Barbosa Ruiz]

Sure thing, I'll get it tested and submit the patch. Thanks for the swift
response.

Cheers,
Felipe

On 15 March 2017 at 07:06, Willy Tarreau  wrote:

> On Wed, Mar 15, 2017 at 10:56:10AM +0100, Willy Tarreau wrote:
> > > I did some digging and found that the ability to handle dynamic
> buffers was
> > > added in 00f0084752eab236af80e61291d672e835790cff
> > >  00f0084752eab236af80e61291d672e835790cff>
> > > but it seems to have been removed in
> > > d7bdcb874bcbd82737e51f080b9b0092863399f9
> > >  d7bdcb874bcbd82737e51f080b9b0092863399f9>,
> > > but I can't tell if the removal was done on purpose or if it was an
> > > accident. Either way, d7bdcb874bcb was not backported to 1.5, making
> their
> > > behaviour wildly different. Also, due to d7bdcb874bcb, 1.6 does not
> work as
> > > stated in the docs
> > >  configuration.html#7.3.5-req.payload>,
> > > which say that "As a special case, if the  argument is zero,
> the
> > > the whole buffer from  to the end is extracted."
> >
> > That's a very accute observation.
> >
> > > Was that on purpose or is it an actual bug?
> >
> > Both. It was made on purpose to address a specific case without thinking
> about
> > this one in my opinion. I think we should return "don't know yet" when
> the
> > current buffer size is zero (not allocated yet) and use the buffer's size
> > when the argument is zero. I'm going to have a look at it.
>
> Looking closer, I was wrong, it's only a bug. In fact the same condition
> was used to address both payload_lv() and payload() while only the latter
> makes a special case of length zero. So I think this should be changed
> like this :
>
> -   if (!buf_size || buf_size > global.tune.bufsize || buf_offset +
> buf_size > global.tune.bufsize) {
> +   if (buf_size > global.tune.bufsize || buf_offset + buf_size >
> global.tune.bufsize) {
>
> Looking at the rest of the function it should work. Would you care to test
> and to send us this patch with a suitable commit message ? Please look at
> CONTRIBUTING to get all the info and/or just "git log src/payload.c".
>
> Thanks,
> Willy
>

Possible regression on HAProxy 1.6, related to ACLs and dynamic payload buffers

[Felipe Guerreiro Barbosa Ruiz]

Hi all,

After upgrading from 1.5 to 1.6 I noticed some ACLs stopped working. All of
them looked like: acl some_name req.payload(0,0) <>

I did some digging and found that the ability to handle dynamic buffers was
added in 00f0084752eab236af80e61291d672e835790cff

but it seems to have been removed in
d7bdcb874bcbd82737e51f080b9b0092863399f9
,
but I can't tell if the removal was done on purpose or if it was an
accident. Either way, d7bdcb874bcb was not backported to 1.5, making their
behaviour wildly different. Also, due to d7bdcb874bcb, 1.6 does not work as
stated in the docs
,
which say that "As a special case, if the  argument is zero, the
the whole buffer from  to the end is extracted."

Was that on purpose or is it an actual bug?

Cheers,
Felipe