Welcome! Log In Create A New Profile

Advanced

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

Posted by Felipe Guerreiro Barbosa Ruiz 
Felipe Guerreiro Barbosa Ruiz
Possible regression on HAProxy 1.6, related to ACLs and dynamic payload buffers
March 14, 2017 08:10PM
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) <<more stuff>>

I did some digging and found that the ability to handle dynamic buffers was
added in 00f0084752eab236af80e61291d672e835790cff
http://git.haproxy.org/?p=haproxy-1.5.git;a=commit;h=00f0084752eab236af80e61291d672e835790cff
but it seems to have been removed in
d7bdcb874bcbd82737e51f080b9b0092863399f9
http://git.haproxy.org/?p=haproxy-1.6.git;a=commit;h=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
<https://cbonte.github.io/haproxy-dconv/1.6/configuration.html#7.3.5-req.payload>;,
which say that "As a special case, if the <length> argument is zero, the
the whole buffer from <offset> to the end is extracted."

Was that on purpose or is it an actual bug?

Cheers,
Felipe
Hi Felipe,

On Tue, Mar 14, 2017 at 04:03:22PM -0300, Felipe Guerreiro Barbosa Ruiz wrote:
> 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) <<more stuff>>
>
> I did some digging and found that the ability to handle dynamic buffers was
> added in 00f0084752eab236af80e61291d672e835790cff
> http://git.haproxy.org/?p=haproxy-1.5.git;a=commit;h=00f0084752eab236af80e61291d672e835790cff
> but it seems to have been removed in
> d7bdcb874bcbd82737e51f080b9b0092863399f9
> http://git.haproxy.org/?p=haproxy-1.6.git;a=commit;h=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
> <https://cbonte.github.io/haproxy-dconv/1.6/configuration.html#7.3.5-req.payload>;,
> which say that "As a special case, if the <length> argument is zero, the
> the whole buffer from <offset> 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.

Thanks,
Willy
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
> > http://git.haproxy.org/?p=haproxy-1.5.git;a=commit;h=00f0084752eab236af80e61291d672e835790cff
> > but it seems to have been removed in
> > d7bdcb874bcbd82737e51f080b9b0092863399f9
> > http://git.haproxy.org/?p=haproxy-1.6.git;a=commit;h=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
> > <https://cbonte.github.io/haproxy-dconv/1.6/configuration.html#7.3.5-req.payload>;,
> > which say that "As a special case, if the <length> argument is zero, the
> > the whole buffer from <offset> 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
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 <[email protected]> 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
> > > <http://git.haproxy.org/?p=haproxy-1.5.git;a=commit;h=
> 00f0084752eab236af80e61291d672e835790cff>
> > > but it seems to have been removed in
> > > d7bdcb874bcbd82737e51f080b9b0092863399f9
> > > <http://git.haproxy.org/?p=haproxy-1.6.git;a=commit;h=
> 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
> > > <https://cbonte.github.io/haproxy-dconv/1.6/
> configuration.html#7.3.5-req.payload>,
> > > which say that "As a special case, if the <length> argument is zero,
> the
> > > the whole buffer from <offset> 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
>
Sorry, only registered users may post in this forum.

Click here to login