Re: strange cppcheck finding

[Willy Tarreau]

On Tue, Mar 20, 2018 at 06:45:08PM +0500,  ??? wrote:
> "UB" stands for undefined behaviour. that's the reason why cppcheck is
> unhappy.
> how do that properly - that's the question :)

The thing is that I'm not aware of any other way to safely detect integer
overflows, it's always done like this. In fact this undefined behaviour
on unsigned ints is defined per-architecture. I think you can safely turn
this one off as we do use integer wrapping at other places on purpose, and
we even build with -fwrapv to make it defined :-)

Cheers,
Willy

Re: strange cppcheck finding

[Илья Шипицин]

"UB" stands for undefined behaviour. that's the reason why cppcheck is
unhappy.
how do that properly - that's the question :)

2018-03-20 10:48 GMT+05:00 Willy Tarreau :

> On Mon, Mar 19, 2018 at 06:55:46PM +0500,  ??? wrote:
> > (it's master)
> >
> > is it in purpose ?
> >
> > [src/ssl_sock.c:1553]: (warning) Invalid test for overflow
> > 'msg+rec_len and
> > overflow is UB.
>
> The code is :
>
> rec_len = (msg[0] << 8) + msg[1];
> msg += 2;
> if (msg + rec_len > end || msg + rec_len < msg)
> return;
>
> It's indeed an overflow check which was placed on purpose. What does
> your tool propose as a better way to check for an overflow ? rec_len
> being a size_t, it's unsigned so the overflow check is fine and
> necessary in my opinion.
>
> Regards,
> Willy
>

Re: strange cppcheck finding

[Willy Tarreau]

On Mon, Mar 19, 2018 at 06:55:46PM +0500,  ??? wrote:
> (it's master)
> 
> is it in purpose ?
> 
> [src/ssl_sock.c:1553]: (warning) Invalid test for overflow
> 'msg+rec_len overflow is UB.

The code is :

rec_len = (msg[0] << 8) + msg[1];
msg += 2;
if (msg + rec_len > end || msg + rec_len < msg)
return;

It's indeed an overflow check which was placed on purpose. What does
your tool propose as a better way to check for an overflow ? rec_len
being a size_t, it's unsigned so the overflow check is fine and
necessary in my opinion.

Regards,
Willy

strange cppcheck finding

[Илья Шипицин]

(it's master)

is it in purpose ?

[src/ssl_sock.c:1553]: (warning) Invalid test for overflow
'msg+rec_len

Re: cppcheck finding

[Willy Tarreau]

Hi,

On Thu, Mar 15, 2018 at 04:27:43PM +0500,  ??? wrote:
> [src/51d.c:373]: (error) Invalid number of character '{' when no macros are
> defined.

Just a small hint, please always mention which version (or ideally commit)
you report issues like this.

>From what I'm seeing, the program is complaining that the code will
not build if neither FIFTYONEDEGREES_H_PATTERN_INCLUDED nor
FIFTYONEDEGREES_H_TRIE_INCLUDED is set. I think we necessarily have one
or the other so it might be a non-issue. I tend to think that this code
could be slightly refactored to limit the impact of these #ifdef, but
to be honest, I'm not sure we'd get any benefit from this.

Thanks,
Willy

cppcheck finding

[Илья Шипицин]

Hi,

[src/51d.c:373]: (error) Invalid number of character '{' when no macros are
defined.

?

Re: cppcheck finding

[Willy Tarreau]

Hi,

On Wed, Mar 14, 2018 at 06:09:26PM +0500,  ??? wrote:
> any action on that ?

It was merged :
  - ec9516a6 in mainline
  - 60238357 in 1.8 branch

Thanks,
Willy

Re: cppcheck finding

[Илья Шипицин]

any action on that ?

2018-03-08 22:29 GMT+05:00 Olivier Houchard :

> Hi,
>
> On Thu, Mar 08, 2018 at 05:44:31PM +0100, Willy Tarreau wrote:
> > Hi,
> >
> > On Wed, Mar 07, 2018 at 03:26:25PM +0500,  ??? wrote:
> > > Hello,
> > >
> > > [src/proto_uxst.c:160]: (warning) Redundant assignment of
> > > 'xfer_sock->next->prev' to itself.
> > >
> > > is it in purpose ?
> >
> > I suspect it's a mistake and that it was meant to be xfer_sock->prev
> instead.
> > CCing Olivier to double-check.
> >
>
> Oops, you're right, good catch !
> The attached patch should fix it.
>
> Regards,
>
> Olivier
>

Re: cppcheck finding

[Olivier Houchard]

Hi,

On Thu, Mar 08, 2018 at 05:44:31PM +0100, Willy Tarreau wrote:
> Hi,
> 
> On Wed, Mar 07, 2018 at 03:26:25PM +0500,  ??? wrote:
> > Hello,
> > 
> > [src/proto_uxst.c:160]: (warning) Redundant assignment of
> > 'xfer_sock->next->prev' to itself.
> > 
> > is it in purpose ?
> 
> I suspect it's a mistake and that it was meant to be xfer_sock->prev instead.
> CCing Olivier to double-check.
> 

Oops, you're right, good catch !
The attached patch should fix it.

Regards,

Olivier
>From 32b505d6093bad96eb4a65272bd3e7b3aad4767b Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Thu, 8 Mar 2018 18:25:49 +0100
Subject: [PATCH] MINOR: unix: Don't mess up when removing the socket from the
 xfer_sock_list.

When removing the socket from the xfer_sock_list, we want to set
next->prev to prev, not to next->prev, which is useless.

This should be backported to 1.8.
---
 src/proto_uxst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/proto_uxst.c b/src/proto_uxst.c
index 3ab637f20..0f717385e 100644
--- a/src/proto_uxst.c
+++ b/src/proto_uxst.c
@@ -157,7 +157,7 @@ static int uxst_find_compatible_fd(struct listener *l)
if (xfer_sock->prev)
xfer_sock->prev->next = xfer_sock->next;
if (xfer_sock->next)
-   xfer_sock->next->prev = xfer_sock->next->prev;
+   xfer_sock->next->prev = xfer_sock->prev;
free(xfer_sock);
}
return ret;
-- 
2.14.3

Re: cppcheck finding

[Willy Tarreau]

Hi,

On Wed, Mar 07, 2018 at 03:26:25PM +0500,  ??? wrote:
> Hello,
> 
> [src/proto_uxst.c:160]: (warning) Redundant assignment of
> 'xfer_sock->next->prev' to itself.
> 
> is it in purpose ?

I suspect it's a mistake and that it was meant to be xfer_sock->prev instead.
CCing Olivier to double-check.

Thanks!
Willy

cppcheck finding

[Илья Шипицин]

Hello,

[src/proto_uxst.c:160]: (warning) Redundant assignment of
'xfer_sock->next->prev' to itself.

is it in purpose ?

Re: another cppcheck finding

[Baptiste]

I also fixed it in a patch set to make the resolution pool dynamic :)

Baptiste

Re: another cppcheck finding

[Christopher Faulet]

Le 04/10/2017 à 11:54, Илья Шипицин a écrit :

Hi,

I'm working on the DNS part to make it thread-safe. In my patch set,
among other things, I fixed this one. I will send everything to
Willy in few days. So don't bother with it.


... using ThreadSanitizer from google ?



No. We planned to add the support of threads in HAProxy 1.8. It requires 
many changes to make all parts thread-safe. DNS is one of them. Note 
that currently HAProxy is not multithreaded, there is no thread-safety 
issue.


--
Christopher Faulet

Re: another cppcheck finding

[Илья Шипицин]

2017-10-04 14:00 GMT+05:00 Christopher Faulet :

> Le 04/10/2017 à 07:49, Илья Шипицин a écrit :
>
>>
>>
>> 2017-10-04 9:15 GMT+05:00 Willy Tarreau >:
>>
>> Hi Ilya,
>>
>> [also CCing Baptiste]
>>
>> On Tue, Oct 03, 2017 at 05:25:17PM +0500,  ??? wrote:
>> > [src/dns.c:2502]: (error) Memory leak: buffer
>> >
>> >
>> > I do not see any "buffer" usage except conditional free.
>> > should we just remove "buffer" from there ?
>>
>> I think you're referring to this part :
>>
>> struct dns_resolution *dns_alloc_resolution(void)
>> {
>>  struct dns_resolution *resolution = NULL;
>>  char *buffer = NULL;
>>
>>  resolution = calloc(1, sizeof(*resolution));
>>  buffer = calloc(1, global.tune.bufsize);
>>
>>  if (!resolution || !buffer) {
>>  free(buffer);
>>  free(resolution);
>>  return NULL;
>>  }
>>
>>  LIST_INIT(>requester.wait);
>>  LIST_INIT(>requester.curr);
>>
>>  return resolution;
>> }
>>
>> And there's definitely a memory leak on the allocated buffer. Buffers
>> used to be needed for resolution in the past but I think that's no
>> longer the case, so I think that indeed the buffer can be completely
>> removed (unless it should be assigned somewhere of course). It's
>>
>>
>> great. I'll send a patch today
>>
>>
> Hi,
>
> I'm working on the DNS part to make it thread-safe. In my patch set, among
> other things, I fixed this one. I will send everything to Willy in few
> days. So don't bother with it.
>

... using ThreadSanitizer from google ?

I plan to fix cppcheck/clang issues first, and have a look at sanitizers
after that


>
> Thanks
> --
> Christopher Faulet
>
>

Re: another cppcheck finding

[Christopher Faulet]

Le 04/10/2017 à 07:49, Илья Шипицин a écrit :



2017-10-04 9:15 GMT+05:00 Willy Tarreau >:

Hi Ilya,

[also CCing Baptiste]

On Tue, Oct 03, 2017 at 05:25:17PM +0500,  ??? wrote:
> [src/dns.c:2502]: (error) Memory leak: buffer
>
>
> I do not see any "buffer" usage except conditional free.
> should we just remove "buffer" from there ?

I think you're referring to this part :

struct dns_resolution *dns_alloc_resolution(void)
{
         struct dns_resolution *resolution = NULL;
         char *buffer = NULL;

         resolution = calloc(1, sizeof(*resolution));
         buffer = calloc(1, global.tune.bufsize);

         if (!resolution || !buffer) {
                 free(buffer);
                 free(resolution);
                 return NULL;
         }

         LIST_INIT(>requester.wait);
         LIST_INIT(>requester.curr);

         return resolution;
}

And there's definitely a memory leak on the allocated buffer. Buffers
used to be needed for resolution in the past but I think that's no
longer the case, so I think that indeed the buffer can be completely
removed (unless it should be assigned somewhere of course). It's


great. I'll send a patch today



Hi,

I'm working on the DNS part to make it thread-safe. In my patch set, 
among other things, I fixed this one. I will send everything to Willy in 
few days. So don't bother with it.


Thanks
--
Christopher Faulet

Re: another cppcheck finding

[Илья Шипицин]

2017-10-04 9:15 GMT+05:00 Willy Tarreau :

> Hi Ilya,
>
> [also CCing Baptiste]
>
> On Tue, Oct 03, 2017 at 05:25:17PM +0500,  ??? wrote:
> > [src/dns.c:2502]: (error) Memory leak: buffer
> >
> >
> > I do not see any "buffer" usage except conditional free.
> > should we just remove "buffer" from there ?
>
> I think you're referring to this part :
>
> struct dns_resolution *dns_alloc_resolution(void)
> {
> struct dns_resolution *resolution = NULL;
> char *buffer = NULL;
>
> resolution = calloc(1, sizeof(*resolution));
> buffer = calloc(1, global.tune.bufsize);
>
> if (!resolution || !buffer) {
> free(buffer);
> free(resolution);
> return NULL;
> }
>
> LIST_INIT(>requester.wait);
> LIST_INIT(>requester.curr);
>
> return resolution;
> }
>
> And there's definitely a memory leak on the allocated buffer. Buffers
> used to be needed for resolution in the past but I think that's no
> longer the case, so I think that indeed the buffer can be completely
> removed (unless it should be assigned somewhere of course). It's
>

great. I'll send a patch today



> possible that there are other such places after the DNS processing
> was refactored some time ago.
>
> Thanks,
> Willy
>
> >
> > Cheers,
> > Ilya Shipitsin
>

Re: another cppcheck finding

[Willy Tarreau]

Hi Ilya,

[also CCing Baptiste]

On Tue, Oct 03, 2017 at 05:25:17PM +0500,  ??? wrote:
> [src/dns.c:2502]: (error) Memory leak: buffer
> 
> 
> I do not see any "buffer" usage except conditional free.
> should we just remove "buffer" from there ?

I think you're referring to this part :

struct dns_resolution *dns_alloc_resolution(void)
{
struct dns_resolution *resolution = NULL;
char *buffer = NULL;

resolution = calloc(1, sizeof(*resolution));
buffer = calloc(1, global.tune.bufsize);

if (!resolution || !buffer) {
free(buffer);
free(resolution);
return NULL;
}

LIST_INIT(>requester.wait);
LIST_INIT(>requester.curr);

return resolution;
}

And there's definitely a memory leak on the allocated buffer. Buffers
used to be needed for resolution in the past but I think that's no
longer the case, so I think that indeed the buffer can be completely
removed (unless it should be assigned somewhere of course). It's
possible that there are other such places after the DNS processing
was refactored some time ago.

Thanks,
Willy

> 
> Cheers,
> Ilya Shipitsin

another cppcheck finding

[Илья Шипицин]

hello!



[src/dns.c:2502]: (error) Memory leak: buffer


I do not see any "buffer" usage except conditional free.
should we just remove "buffer" from there ?

Cheers,
Ilya Shipitsin

Re: cppcheck finding

[Cyril Bonté]

Hi all,

Le 15/09/2017 à 18:40, Willy Tarreau a écrit :

I once had an interesting discussion with PHK who proposed to extend
the varnish test program to also cover haproxy so that we could write
various test cases, as he wrote this tool to address exactly the same
issue. It could be an option, but I didn't have time (nor do I now) to
look into that.


I also wanted to explore that possibility, but unfortunately I'm not 
able to find time yet, too :-/


I had some ideas to automatically generate test configurations, 
randomizing keyword orders for example, to validate the config parser, 
the expected behaviour and to ensure there is no regression between 
versions. The only issue is that it requires some time :)



--
Cyril Bonté

Re: cppcheck finding

[Willy Tarreau]

On Fri, Sep 15, 2017 at 03:04:26PM +0200, Christopher Faulet wrote:
> You're right, there are bugs there. The worst is on the compression filter.
> I attached patches to fix them.
> 
> Willy, could you merge it please ? Some of them must be backported in 1.7.

Now applied, thanks Christopher.

Willy

Re: cppcheck finding

[Willy Tarreau]

Hi Aleks,

On Fri, Sep 15, 2017 at 06:29:42PM +0200, Aleksandar Lazic wrote:
> Hi.
> 
> Willy Tarreau wrote on 15.09.2017:
> 
> > On Fri, Sep 15, 2017 at 06:36:20PM +0500,  ??? wrote:
> >> I'd say, it's chicken and egg situation. Whichever comes first, tests or 
> >> CI.
> >> if we start a CI with "just build", it will evolve, people will start
> >> writing tests (I beleive so)
> 
> > I tend to believe it as well. However what I'm less convinced about is
> > the long term maintenance of such an infrastructure. People tend to
> > volunteer to set up stuff initially because it improves a situation but
> > after 3 or 4 years they have other things to do and nobody maintains
> > what they built anymore. We've seen this in the past with other stuff
> > like a translated version of the web site for example. It's more
> > important to spot who is willing to step up on this and to be sure to
> > maintain it for a while and at least long enough to pass it to someone
> > else once they start to get fed up.
> 
> I have created a build ci on gitlab.
> 
> https://gitlab.com/aleks001/haproxy17-centos/tree/master
> 
> The test would be very difficult due to the fact that you should have at 
> least a backend <-> haproxy <-> client and what Test should be run?

Hehe welcome to the real world guys ;-)

Yes testing an intermediary is a real pain as it requires acting on both
sides at once. When you look at tests/test-fsm.cfg you start to realize
the type of pain it is, and this file doesn't even test 0.1% of the
internals, it just validates that we didn't break too much something
very obvious.

> When you take a look into the features of haproxy ;-) it's a huge task.
> 
> I suggest to use caddy as simple webserver or can we use the error_file 
> function to deliver some content from haproxy?

I tend to use netcat, openssl, socat, curl, httpterm, inject, haproxy
(yes, itself). Also writing config files involving multiple features at
once by chaining instances works pretty well (ie ssl, proxy protocol
etc). But even then it's a pain and it tests very very little of it.

I once had an interesting discussion with PHK who proposed to extend
the varnish test program to also cover haproxy so that we could write
various test cases, as he wrote this tool to address exactly the same
issue. It could be an option, but I didn't have time (nor do I now) to
look into that.

Cheers,
Willy

Re: cppcheck finding

[Илья Шипицин]

2017-09-15 21:29 GMT+05:00 Aleksandar Lazic :

> Hi.
>
> Willy Tarreau wrote on 15.09.2017:
>
> > On Fri, Sep 15, 2017 at 06:36:20PM +0500,  ??? wrote:
> >> I'd say, it's chicken and egg situation. Whichever comes first, tests
> or CI.
> >> if we start a CI with "just build", it will evolve, people will start
> >> writing tests (I beleive so)
>
> > I tend to believe it as well. However what I'm less convinced about is
> > the long term maintenance of such an infrastructure. People tend to
> > volunteer to set up stuff initially because it improves a situation but
> > after 3 or 4 years they have other things to do and nobody maintains
> > what they built anymore. We've seen this in the past with other stuff
> > like a translated version of the web site for example. It's more
> > important to spot who is willing to step up on this and to be sure to
> > maintain it for a while and at least long enough to pass it to someone
> > else once they start to get fed up.
>
> I have created a build ci on gitlab.
>
> https://gitlab.com/aleks001/haproxy17-centos/tree/master



there's more easy way of using docker images with gitlab-ci,
for example:

https://gitlab.com/chipitsine/openvpn/blob/master/.gitlab-ci.yml


also, gitlab is very flexible, in the example above I use "shared" runners.
and, if we want to maintain, we can attach own runners as well


>
>
> The test would be very difficult due to the fact that you should have at
> least a backend <-> haproxy <-> client and what Test should be run?
>
> When you take a look into the features of haproxy ;-) it's a huge task.
>

yep


>
> I suggest to use caddy as simple webserver or can we use the error_file
> function to deliver some content from haproxy?
>
> > Cheers,
> > Willy
>
>
> --
> Best Regards
> Aleks
>
>

Re: cppcheck finding

[Aleksandar Lazic]

Hi.

Willy Tarreau wrote on 15.09.2017:

> On Fri, Sep 15, 2017 at 06:36:20PM +0500,  ??? wrote:
>> I'd say, it's chicken and egg situation. Whichever comes first, tests or CI.
>> if we start a CI with "just build", it will evolve, people will start
>> writing tests (I beleive so)

> I tend to believe it as well. However what I'm less convinced about is
> the long term maintenance of such an infrastructure. People tend to
> volunteer to set up stuff initially because it improves a situation but
> after 3 or 4 years they have other things to do and nobody maintains
> what they built anymore. We've seen this in the past with other stuff
> like a translated version of the web site for example. It's more
> important to spot who is willing to step up on this and to be sure to
> maintain it for a while and at least long enough to pass it to someone
> else once they start to get fed up.

I have created a build ci on gitlab.

https://gitlab.com/aleks001/haproxy17-centos/tree/master

The test would be very difficult due to the fact that you should have at 
least a backend <-> haproxy <-> client and what Test should be run?

When you take a look into the features of haproxy ;-) it's a huge task.

I suggest to use caddy as simple webserver or can we use the error_file 
function to deliver some content from haproxy?

> Cheers,
> Willy


-- 
Best Regards
Aleks

Re: cppcheck finding

[Willy Tarreau]

On Fri, Sep 15, 2017 at 06:36:20PM +0500,  ??? wrote:
> I'd say, it's chicken and egg situation. Whichever comes first, tests or CI.
> if we start a CI with "just build", it will evolve, people will start
> writing tests (I beleive so)

I tend to believe it as well. However what I'm less convinced about is
the long term maintenance of such an infrastructure. People tend to
volunteer to set up stuff initially because it improves a situation but
after 3 or 4 years they have other things to do and nobody maintains
what they built anymore. We've seen this in the past with other stuff
like a translated version of the web site for example. It's more
important to spot who is willing to step up on this and to be sure to
maintain it for a while and at least long enough to pass it to someone
else once they start to get fed up.

Cheers,
Willy

Re: cppcheck finding

[Илья Шипицин]

2017-09-15 18:22 GMT+05:00 Christopher Faulet :

> Le 15/09/2017 à 15:07, Илья Шипицин a écrit :
>
>> and what about CI ?
>>
>> something like gitlab-ci, travis, jenkins ? I'll invest some efforts in
>> that
>>
>>
> No CI. This would be useful to have one but we have no time to work on it
> for now. Having a CI is not a big deal. The harder is to write tests and
> set up and maintain the build/test matrix.
>


I'd say, it's chicken and egg situation. Whichever comes first, tests or CI.
if we start a CI with "just build", it will evolve, people will start
writing tests (I beleive so)


>
> --
> Christopher Faulet
>

Re: cppcheck finding

[Christopher Faulet]

Le 15/09/2017 à 15:07, Илья Шипицин a écrit :

and what about CI ?

something like gitlab-ci, travis, jenkins ? I'll invest some efforts in that



No CI. This would be useful to have one but we have no time to work on 
it for now. Having a CI is not a big deal. The harder is to write tests 
and set up and maintain the build/test matrix.


--
Christopher Faulet

Re: cppcheck finding

[Илья Шипицин]

and what about CI ?

something like gitlab-ci, travis, jenkins ? I'll invest some efforts in that

2017-09-15 18:04 GMT+05:00 Christopher Faulet :

> Le 15/09/2017 à 08:36, Илья Шипицин a écrit :
>
>> great, thank for the feedback.
>>
>> there're few things like that
>>
>> [src/flt_http_comp.c:926] -> [src/flt_http_comp.c:926]: (warning) Either
>> the condition 'txn' is redundant or there is possible null pointer
>> dereference: txn.
>> [src/flt_spoe.c:2765] -> [src/flt_spoe.c:2766]: (warning) Either the
>> condition 'ctx==NULL' is redundant or there is possible null pointer
>> dereference: ctx.
>> [src/haproxy.c:568]: (error) Common realloc mistake: 'next_argv' nulled
>> but not freed upon failure
>> [src/server.c:3993] -> [src/server.c:3995]: (warning) Either the
>> condition 'nameserver' is redundant or there is possible null pointer
>> dereference: nameserver.
>> [src/ssl_sock.c:4403] -> [src/ssl_sock.c:4397]: (warning) Either the
>> condition '!bind_conf' is redundant or there is possible null pointer
>> dereference: bind_conf.
>>
>>
> Hi,
>
> You're right, there are bugs there. The worst is on the compression
> filter. I attached patches to fix them.
>
> Willy, could you merge it please ? Some of them must be backported in 1.7.
>
> Thanks,
> --
> Christopher Faulet
>

Re: cppcheck finding

[Christopher Faulet]

Le 15/09/2017 à 08:36, Илья Шипицин a écrit :

great, thank for the feedback.

there're few things like that

[src/flt_http_comp.c:926] -> [src/flt_http_comp.c:926]: (warning) Either 
the condition 'txn' is redundant or there is possible null pointer 
dereference: txn.
[src/flt_spoe.c:2765] -> [src/flt_spoe.c:2766]: (warning) Either the 
condition 'ctx==NULL' is redundant or there is possible null pointer 
dereference: ctx.
[src/haproxy.c:568]: (error) Common realloc mistake: 'next_argv' nulled 
but not freed upon failure
[src/server.c:3993] -> [src/server.c:3995]: (warning) Either the 
condition 'nameserver' is redundant or there is possible null pointer 
dereference: nameserver.
[src/ssl_sock.c:4403] -> [src/ssl_sock.c:4397]: (warning) Either the 
condition '!bind_conf' is redundant or there is possible null pointer 
dereference: bind_conf.




Hi,

You're right, there are bugs there. The worst is on the compression 
filter. I attached patches to fix them.


Willy, could you merge it please ? Some of them must be backported in 1.7.

Thanks,
--
Christopher Faulet
>From 362bf07d61b06469bff839886d52db24daa2aa5e Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Fri, 15 Sep 2017 10:14:43 +0200
Subject: [PATCH 1/5] BUG/MEDIUM: compression: Fix check on txn in
 smp_fetch_res_comp_algo

The check was totally messed up. In the worse case, it led to a crash, when
res.comp_algo sample fetch was retrieved on uncompressed response (with the
compression enabled).

This patch must be backported in 1.7.
---
 src/flt_http_comp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/flt_http_comp.c b/src/flt_http_comp.c
index 64c669d54..4d5332832 100644
--- a/src/flt_http_comp.c
+++ b/src/flt_http_comp.c
@@ -923,7 +923,7 @@ smp_fetch_res_comp_algo(const struct arg *args, struct sample *smp,
 	struct filter *filter;
 	struct comp_state *st;
 
-	if (!(txn || !(txn->rsp.flags & HTTP_MSGF_COMPRESSING)))
+	if (!txn || !(txn->rsp.flags & HTTP_MSGF_COMPRESSING))
 		return 0;
 
 	list_for_each_entry(filter, _flt(smp->strm)->filters, list) {
-- 
2.13.5

>From 30bb79ce1a47666d5d9cd20636e97b7589e34dd7 Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Fri, 15 Sep 2017 11:39:36 +0200
Subject: [PATCH 2/5] BUG/MINOR: compression: Check response headers before
 http-response rules eval

This is required if we want to use res.comp or res.comp_algo sample fetches in
http-response rules.

This patch must be backported in 1.7.
---
 src/flt_http_comp.c | 30 +-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/flt_http_comp.c b/src/flt_http_comp.c
index 4d5332832..0ed252895 100644
--- a/src/flt_http_comp.c
+++ b/src/flt_http_comp.c
@@ -103,6 +103,12 @@ comp_start_analyze(struct stream *s, struct filter *filter, struct channel *chn)
 		st->initialized = 0;
 		st->finished= 0;
 		filter->ctx = st;
+
+		/* Register post-analyzer on AN_RES_WAIT_HTTP because we need to
+		 * analyze response headers before http-response rules execution
+		 * to be sure we can use res.comp and res.comp_algo sample
+		 * fetches */
+		filter->post_analyzers |= AN_RES_WAIT_HTTP;
 	}
 	return 1;
 }
@@ -135,7 +141,8 @@ comp_http_headers(struct stream *s, struct filter *filter, struct http_msg *msg)
 	if (!(msg->chn->flags & CF_ISRESP))
 		select_compression_request_header(st, s, msg);
 	else {
-		select_compression_response_header(st, s, msg);
+		/* Response headers have already been checked in
+		 * comp_http_post_analyze callback. */
 		if (st->comp_algo) {
 			register_data_filter(s, msg->chn, filter);
 			st->hdrs_len = s->txn->rsp.sov;
@@ -147,6 +154,26 @@ comp_http_headers(struct stream *s, struct filter *filter, struct http_msg *msg)
 }
 
 static int
+comp_http_post_analyze(struct stream *s, struct filter *filter,
+		   struct channel *chn, unsigned an_bit)
+{
+	struct http_txn   *txn = s->txn;
+	struct http_msg   *msg = >rsp;
+	struct comp_state *st  = filter->ctx;
+
+	if (an_bit != AN_RES_WAIT_HTTP)
+		goto end;
+
+	if (!strm_fe(s)->comp && !s->be->comp)
+		goto end;
+
+	select_compression_response_header(st, s, msg);
+
+  end:
+	return 1;
+}
+
+static int
 comp_http_data(struct stream *s, struct filter *filter, struct http_msg *msg)
 {
 	struct comp_state *st = filter->ctx;
@@ -768,6 +795,7 @@ struct flt_ops comp_ops = {
 
 	.channel_start_analyze = comp_start_analyze,
 	.channel_end_analyze   = comp_end_analyze,
+	.channel_post_analyze  = comp_http_post_analyze,
 
 	.http_headers  = comp_http_headers,
 	.http_data = comp_http_data,
-- 
2.13.5

>From eef0b960798c3db93923f2c920f27bab6e53286e Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Fri, 15 Sep 2017 11:51:18 +0200
Subject: [PATCH 3/5] BUG/MINOR: spoe: Don't rely on SPOE ctx in debug message
 when its creation failed

If the SPOE context creation failed, we must not try to use it in the debug
message used to notice the 

Re: cppcheck finding

[Илья Шипицин]

great, thank for the feedback.

there're few things like that

[src/flt_http_comp.c:926] -> [src/flt_http_comp.c:926]: (warning) Either
the condition 'txn' is redundant or there is possible null pointer
dereference: txn.
[src/flt_spoe.c:2765] -> [src/flt_spoe.c:2766]: (warning) Either the
condition 'ctx==NULL' is redundant or there is possible null pointer
dereference: ctx.
[src/haproxy.c:568]: (error) Common realloc mistake: 'next_argv' nulled but
not freed upon failure
[src/server.c:3993] -> [src/server.c:3995]: (warning) Either the condition
'nameserver' is redundant or there is possible null pointer dereference:
nameserver.
[src/ssl_sock.c:4403] -> [src/ssl_sock.c:4397]: (warning) Either the
condition '!bind_conf' is redundant or there is possible null pointer
dereference: bind_conf.


I had a quick look to them, seem to be logical errors.
are there any CI process for haproxy ? I didn't find any


2017-09-15 11:26 GMT+05:00 Willy Tarreau :

> Hello Ilya,
>
> On Thu, Sep 14, 2017 at 11:11:36PM +0500,  ??? wrote:
> > hello,
> >
> > [src/flt_http_comp.c:926] -> [src/flt_http_comp.c:926]: (warning) Either
> > the condition 'txn' is redundant or there is possible null pointer
> > dereference: txn.
> >
> > should there be && instead of || ?
>
> You're speaking about this :
>
> if (!(txn || !(txn->rsp.flags & HTTP_MSGF_COMPRESSING)))
> return 0;
>
> and you're right, it should be this :
>
> if (!txn || !(txn->rsp.flags & HTTP_MSGF_COMPRESSING))
> return 0;
>
> CCing Christopher. We've had this since 1.7 apparently.
>
> Thanks for the report!
> Willy
>

Re: cppcheck finding

[Willy Tarreau]

Hello Ilya,

On Thu, Sep 14, 2017 at 11:11:36PM +0500,  ??? wrote:
> hello,
> 
> [src/flt_http_comp.c:926] -> [src/flt_http_comp.c:926]: (warning) Either
> the condition 'txn' is redundant or there is possible null pointer
> dereference: txn.
> 
> should there be && instead of || ?

You're speaking about this :

if (!(txn || !(txn->rsp.flags & HTTP_MSGF_COMPRESSING)))
return 0;

and you're right, it should be this :

if (!txn || !(txn->rsp.flags & HTTP_MSGF_COMPRESSING))
return 0;

CCing Christopher. We've had this since 1.7 apparently.

Thanks for the report!
Willy

cppcheck finding

[Илья Шипицин]

hello,

[src/flt_http_comp.c:926] -> [src/flt_http_comp.c:926]: (warning) Either
the condition 'txn' is redundant or there is possible null pointer
dereference: txn.

should there be && instead of || ?

Cheers,
Ilya Shipitsin