OT: Seeking help: F5 in front of haproxy

[Aleksandar Lazic]

Dear List members.

Please accept my apologize for the question here, but as here are a lot persons 
with huge knowledge I hope someone can help me.

I have the following easy requirements for the F5.

* Route and Balance Clients ( browser ) based on SNI header to backend pool
 * Make health checks with SNI header set to check availability of back ends

HAProxy handle the requests properly, when I make a `openssl s_client 
-servername ` from the F5 shell but the browsers are unable to make TLS 
handshakes .

Any help is very welcome also of the list.
 Again sorry for disturbing you, and thanks for your patience.

Very best regards
 Aleks.

Re: Bugfix version 2.0.1/2.1?

[Aleksandar Lazic]

Hi Christopher.

Thanks for answer.

Regards
 Aleks

Mon Jun 24 15:29:58 GMT+02:00 2019 Christopher Faulet :

> Le 24/06/2019 à 14:46, Aleksandar Lazic a écrit :
> > Hi,
> >
> > as this cookie bug prevent some user to use the shiny new haproxy 2 is 
> > there any
> > plan to release a bugfix version soon?
> >
>
> Hi Aleks,
>
> Yes, we planned to do a new 2.0 release this week. Wednesday or Thursday. 
> There
> are still annoying bugs we want to try to fix before. But if we don't have any
> fix by Wednesday, we will do the 2.0.1 as is.
>
>
> --
> Christopher Faulet
>

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Mildis]

Le 24 juin 2019 à 19:32, Vincent Bernat  a écrit :
> 
> ❦ 24 juin 2019 19:08 +02, Tim Düsterhus :
> 
>>> You can workaround this by not chrooting HAProxy. The problem is that
>>> once chrooted, it cannot load the library. We should force libpthread to
>>> preload this lib.
>> 
>> This mailing list thread might be relevant / helpful here:
>> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
>> (Message-ID: 8e3aa43f-d6ad-4128-bb10-feeb5f315...@gandi.net).
> 
> Thanks. Adding ADDLIB="-Wl,--no-as-needed -lgcc_s -Wl,--as-needed" fixes
> the issue.
> 
Thanks Vincent for your prompt action.
As Tim and I point it, 
https://www.mail-archive.com/haproxy@formilux.org/msg33189.html 
 shows this 
behavior and solution.
I was not aware of haproxy 2.0 autodetection of nbthreads highlighted by 
William and the side-effect on packaging.

> 1.9 was already calling pthread_exit() but it doesn't seem to happen
> with 1.9.8. Do you know what could have changed?
> -- 
> The lunatic, the lover, and the poet,
> Are of imagination all compact...
>   -- Wm. Shakespeare, "A Midsummer Night's Dream"

Re: haproxy 2.0 - stretch - libgcc_s.so.1

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

пн, 24 июн. 2019 г. в 22:08, Vincent Bernat :

>  ❦ 24 juin 2019 18:43 +02, Mildis :
>
> > I'm hitting
> > https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
> > with haproxy 2.0 on Stretch, when doing a hot-reload
> >
> > Jun 24 18:34:05 haproxy[32347]: libgcc_s.so.1 must be installed for
> pthread_cancel to work
> > Jun 24 18:34:05 haproxy[32347]: [WARNING] 174/183405 (32347) : Former
> worker #1 (32363) exited with code 134 (Aborted)
> >
> > Is it something to be fixed by Vincent during the build for the
> > packaging ?
>
> You can workaround this by not chrooting HAProxy. The problem is that
> once chrooted, it cannot load the library. We should force libpthread to
> preload this lib.
>

ok, seems it might be covered with some automatic testing.
either debian (when packaging) or mainstream.

is anybody volunteering to set up tests )) ?


> --
> The better part of valor is discretion.
> -- William Shakespeare, "Henry IV"
>
>

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[William Lallemand]

On Mon, Jun 24, 2019 at 07:42:44PM +0200, William Lallemand wrote:
> On Mon, Jun 24, 2019 at 07:38:03PM +0200, Tim Düsterhus wrote:
> > Vincent,
> > 
> > Am 24.06.19 um 19:32 schrieb Vincent Bernat:
> > > with 1.9.8. Do you know what could have changed?
> > > 
> > 
> > I can't help there. I just remembered I read something about libgcc_s.so
> > on the list before and searched my INBOX.
> > 
> > Best regards
> > Tim Düsterhus
> > 
> 
> Hi,
> 
> It happens to me with the latest commit in 1.9 branch.
> 
> You need a nbthread > 1 to reproduce.
> 
> Regards,
> 

I forget to explain: 2.0 calculates nbthread automatically, that's probably why
you don't have the problem in 1.9 if you don't have an explicit nbthread.


-- 
William Lallemand

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[William Lallemand]

On Mon, Jun 24, 2019 at 07:38:03PM +0200, Tim Düsterhus wrote:
> Vincent,
> 
> Am 24.06.19 um 19:32 schrieb Vincent Bernat:
> > with 1.9.8. Do you know what could have changed?
> > 
> 
> I can't help there. I just remembered I read something about libgcc_s.so
> on the list before and searched my INBOX.
> 
> Best regards
> Tim Düsterhus
> 

Hi,

It happens to me with the latest commit in 1.9 branch.

You need a nbthread > 1 to reproduce.

Regards,

-- 
William Lallemand

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Tim Düsterhus]

Vincent,

Am 24.06.19 um 19:32 schrieb Vincent Bernat:
> with 1.9.8. Do you know what could have changed?
> 

I can't help there. I just remembered I read something about libgcc_s.so
on the list before and searched my INBOX.

Best regards
Tim Düsterhus

Re: [PATCH 0/9] Fix issues detected by clang analyzer.

[Tim Düsterhus]

Willy,

Am 24.06.19 um 11:07 schrieb Willy Tarreau:
> On Mon, Jun 24, 2019 at 10:03:29AM +0200, Tim Düsterhus wrote:
>>> Well, some there seem to be real bugs, but others are more a matter of
>>> style than correctness and I disagree with a bogus compiler dictating
>>> what coding style rules we must respect. Because that compiler is bogus.
>>
>> To be clear: This is not a compiler. It's a static analyzer. clang !=
>> clang analyzer.
> 
> OK maybe but the point remains, we're relying on a tool complaining about
> *style* and not about code correctness. History has proven multiple times
> that whenever we fall into that trap, we end up with :
>   - confusing code which is less and less maintainable over time,
> due to the extra checks that lead to nothing and that become very
> difficult to adjust.
> 
>   - new bugs due to the extra complexity involved in certain checks
> or ways to write code.
> 
>   - regressions in stable branch by failure to adapt the mangled code
> to work properly there.
> 
> Yes I'm in favor of fixing bogus code or to adapt fragile constructs,
> but not to work around compiler (or analyzer) bugs or stupidities if
> they add wrong assumptions, confusion, or suspicion in the code.

I agree here, that's why I only created patches for a subset of the
reported issues.

>>> Please note that I *do* prefer the sizeof(*foo) for readability and long
>>> term correctness, it's just that we must not claim a bug in the code when
>>> the bug is in the compiler itself (or whatever claims there is a bug).
>>
>> Feel free to re-tag that one as MINOR :-)
> 
> In my opinion it's a cleanup : it doesn't change the emitted code at all,
> and makes the code more robust against changes, and saves one round trip
> to the include file for the casual reader trying to figure what caused a
> bug to happen.

Agreed.

>>> Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
>>> of BUG_ON() is to help the *developer*. It should be seen as proven
>>> documentation that helps figuring branches when analysing a bug. While
>>> the assertion in a comment may have become wrong over time, the assertion
>>> in a BUG_ON() is provably true.
>>
>> I agree with any *large* changes to make the analyzer happy. In the two
>> patches I made it was a minimal change to make. The proxy_parse_declare
>> probably could also be changed to:
>>
>>> --- a/src/proxy.c
>>> +++ b/src/proxy.c
>>> @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int 
>>> section, struct proxy *curpx,
>>> hdr->index = curpx->nb_req_cap++;
>>> curpx->req_cap = hdr;
>>> }
>>> -   if (strcmp(args[2], "response") == 0) {
>>> +   else {
>>> +   BUG_ON(strcmp(args[2], "response") != 0);
>>> hdr->next = curpx->rsp_cap;
>>> hdr->index = curpx->nb_rsp_cap++;
>>> curpx->rsp_cap = hdr;
>>
>> which might be acceptable?
> 
> No because that BUG_ON() above is redundant, just put an "else if (strcmp...)"
> if you want but I really fail to see the problem there and we're not going
> to add confusion in the code to please a shitty compiler or analyzer which
> finds bugs where they are not. What you're doing above is mangling the code
> to *temporarily* shut up the analyzer. It has nothing to do with addressing
> any real issue. Either the analyzer is right and the bug is somewhere else
> and must be fixed, or it's wrong and we'd rather ignore it. Otherwise we

IMO silencing is better than ignoring, because then a the report does
not need to be evaluated again when checking again. Of course I
recognize that silencing potentially comes at a cost to the human and
that one should not make any huge changes, just to please the machine.

For the changes I made I'd argue that they actually might be helpful to
the human as well to make sure that the locations in question do not
diverge. Less so for the `proxy_parse_declare` one, more for
`h2_make_htx_(request|response)` one. I find the latter incredibly
non-obvious, but that might be just my view.

May I suggest this one for the latter then (I believe it captures the
intent better than the current one):

--- a/src/h2.c
+++ b/src/h2.c
@@ -689,7 +689,7 @@ int h2_make_htx_request(struct http_hdr *list,
struct htx *htx, unsigned int *ms
goto fail;

/* Let's dump the request now if not yet emitted. */
-   if (!(fields & H2_PHDR_FND_NONE)) {
+   if (!sl) {
sl = h2_prepare_htx_reqline(fields, phdr_val, htx, msgf);
if (!sl)
goto fail;
@@ -913,7 +913,7 @@ int h2_make_htx_response(struct http_hdr *list,
struct htx *htx, unsigned int *m
goto fail;

/* Let's dump the request now if not yet emitted. */
-   if (!(fields & H2_PHDR_FND_NONE)) {
+   if (!sl) {
sl = h2_prepare_htx_stsline(fields, phdr_val, htx, msgf);
 

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Vincent Bernat]

 ❦ 24 juin 2019 19:08 +02, Tim Düsterhus :

>> You can workaround this by not chrooting HAProxy. The problem is that
>> once chrooted, it cannot load the library. We should force libpthread to
>> preload this lib.
>
> This mailing list thread might be relevant / helpful here:
> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
> (Message-ID: 8e3aa43f-d6ad-4128-bb10-feeb5f315...@gandi.net).

Thanks. Adding ADDLIB="-Wl,--no-as-needed -lgcc_s -Wl,--as-needed" fixes
the issue.

1.9 was already calling pthread_exit() but it doesn't seem to happen
with 1.9.8. Do you know what could have changed?
-- 
The lunatic, the lover, and the poet,
Are of imagination all compact...
-- Wm. Shakespeare, "A Midsummer Night's Dream"

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Tim Düsterhus]

Vincent,
Mildis,

Am 24.06.19 um 19:05 schrieb Vincent Bernat:
> You can workaround this by not chrooting HAProxy. The problem is that
> once chrooted, it cannot load the library. We should force libpthread to
> preload this lib.
> 

This mailing list thread might be relevant / helpful here:
https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
(Message-ID: 8e3aa43f-d6ad-4128-bb10-feeb5f315...@gandi.net).

Best regards
Tim Düsterhus

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Tim Düsterhus]

Ilya,

Am 24.06.19 um 19:02 schrieb Илья Шипицин:
> who is Vincent ? and why are you asking the list about Vincent activities ?

Vincent Bernat. One of the Debian maintainers of HAProxy. Mildis is
asking whether their issie is caused by HAProxy or by Debian's packaging.

Best regards
Tim Düsterhus

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Vincent Bernat]

 ❦ 24 juin 2019 18:43 +02, Mildis :

> I'm hitting
> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html 
> with haproxy 2.0 on Stretch, when doing a hot-reload
>
> Jun 24 18:34:05 haproxy[32347]: libgcc_s.so.1 must be installed for 
> pthread_cancel to work
> Jun 24 18:34:05 haproxy[32347]: [WARNING] 174/183405 (32347) : Former worker 
> #1 (32363) exited with code 134 (Aborted)
>
> Is it something to be fixed by Vincent during the build for the
> packaging ?

You can workaround this by not chrooting HAProxy. The problem is that
once chrooted, it cannot load the library. We should force libpthread to
preload this lib.
-- 
The better part of valor is discretion.
-- William Shakespeare, "Henry IV"

Re: haproxy 2.0 - stretch - libgcc_s.so.1

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

Hello,

who is Vincent ? and why are you asking the list about Vincent activities ?

пн, 24 июн. 2019 г. в 21:47, Mildis :

> Hi,
>
> I'm hitting
> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
> with haproxy 2.0 on Stretch, when doing a hot-reload
>
> Jun 24 18:34:05 haproxy[32347]: libgcc_s.so.1 must be installed for
> pthread_cancel to work
> Jun 24 18:34:05 haproxy[32347]: [WARNING] 174/183405 (32347) : Former
> worker #1 (32363) exited with code 134 (Aborted)
>
> Is it something to be fixed by Vincent during the build for the packaging ?
>
> Thanks,
> Mildis
>

haproxy 2.0 - stretch - libgcc_s.so.1

[Mildis]

Hi,

I'm hitting
https://www.mail-archive.com/haproxy@formilux.org/msg33189.html 
with haproxy 2.0 on Stretch, when doing a hot-reload

Jun 24 18:34:05 haproxy[32347]: libgcc_s.so.1 must be installed for 
pthread_cancel to work
Jun 24 18:34:05 haproxy[32347]: [WARNING] 174/183405 (32347) : Former worker #1 
(32363) exited with code 134 (Aborted)

Is it something to be fixed by Vincent during the build for the packaging ?

Thanks,
Mildis

Re: Bugfix version 2.0.1/2.1?

[Christopher Faulet]

Le 24/06/2019 à 14:46, Aleksandar Lazic a écrit :

Hi,

as this cookie bug prevent some user to use the shiny new haproxy 2 is there any 
plan to release a bugfix version soon?




Hi Aleks,

Yes, we planned to do a new 2.0 release this week. Wednesday or Thursday. There 
are still annoying bugs we want to try to fix before. But if we don't have any 
fix by Wednesday, we will do the 2.0.1 as is.



--
Christopher Faulet

Bugfix version 2.0.1/2.1?

[Aleksandar Lazic]

Hi,

as this cookie bug prevent some user to use the shiny new haproxy 2 is there 
any plan to release a bugfix version soon?

Regards
 Aleks

Re: [PATCH 5/9] BUG/MINOR: spoe: Fix memory leak if failing to allocate memory

[Christopher Faulet]

Le 23/06/2019 à 22:10, Tim Duesterhus a écrit :

Technically harmless, but it annoys clang analyzer.

This bug was introduced in 336d3ef0e77192582c98b3c578927a529ceadd9b.
This fix should be backported to HAProxy 1.9+.
---
  src/flt_spoe.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index f20b2640b..4e48aa95c 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -3751,6 +3751,7 @@ cfg_parse_spoe_agent(const char *file, int linenum, char 
**args, int kwm)
goto out;
}
if ((vph->name  = strdup(args[cur_arg])) == NULL) {
+   free(vph);
ha_alert("parsing [%s:%d] : out of memory.\n", 
file, linenum);
err_code |= ERR_ALERT | ERR_ABORT;
goto out;



Thanks, merged now.

--
Christopher Faulet

Re: Zero RTT in backend server side

[Igor Pav]

Thanks Olivier, it worked now. If I don't make a serious wrong, can
haproxy to do multiplexing connections from FE to a single connection
to BE by using H2+TLS, then keep the connection to BE alive as long as
possible, so we could omit handshakes?

On Mon, Jun 24, 2019 at 5:56 PM Olivier Houchard  wrote:
>
> Hi Igor,
>
> On Sun, Jun 23, 2019 at 08:42:46PM +0800, Igor Pav wrote:
> > Hi Olivier,
> >
> > The `retry-on 0rtt-rejected` will only work in tcp mode, is that
> > possible to let it work in http mode too?
> >
>
> It should work with HTTP too. What may happen is you're using "alpn" on
> the server line, and thus we have to wait until the handshake is done to
> know if we're using H1 or H2, so we can't send early data, because we won't
> know its format.
> If you only want yo use H2, you can add "proto h2" on your server line, and
> it should work.
>
> Regards,
>
> Olivier
>

Re: [PATCH 0/9] Fix issues detected by clang analyzer.

[Frederic Lecaille]

On 6/24/19 10:03 AM, Tim Düsterhus wrote:

Willy,

Am 24.06.19 um 07:33 schrieb Willy Tarreau:

Hi Tim,


[snipped]


If it emits an error when using sizeof(int) instead of sizeof(*foo) where
foo is an pointer to unsigned int, it is bogus. I challenge you to present
me a relevant architecture where unsigned changes the allocated size!r


For C11 it is guaranteed that there is none according to this:
https://stackoverflow.com/a/13169473/782822


This is guaranteed since the beginning of C. Not only for C11. :)

Fred.

Re: Zero RTT in backend server side

[Olivier Houchard]

Hi Igor,

On Sun, Jun 23, 2019 at 08:42:46PM +0800, Igor Pav wrote:
> Hi Olivier,
> 
> The `retry-on 0rtt-rejected` will only work in tcp mode, is that
> possible to let it work in http mode too?
> 

It should work with HTTP too. What may happen is you're using "alpn" on
the server line, and thus we have to wait until the handshake is done to
know if we're using H1 or H2, so we can't send early data, because we won't
know its format.
If you only want yo use H2, you can add "proto h2" on your server line, and
it should work.

Regards,

Olivier

Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

[Frederic Lecaille]

Hi Ilya,

On 6/24/19 8:25 AM, Илья Шипицин wrote:

I had to be mroe detailed :)

I meant "simple reg test" + address sanitizer which we run in travis

address sanitizer is capable of catching those things



No, we do not write reg test anymore for bugs which have very few chance 
to come back.


Fred.

Re: [PATCH 2/9] BUG/MINOR: log: Detect missing sampling ranges in config

[Frederic Lecaille]

On 6/24/19 11:01 AM, Frederic Lecaille wrote:

Hi Tim,

On 6/23/19 10:10 PM, Tim Duesterhus wrote:

Consider a config like:

 global
 log 127.0.0.1:10001 sample :10 local0

No sampling ranges are given here, leading to NULL being passed
as the first argument to qsort.

This configuration does not make sense anyway, a log without ranges
would never log. Thus output an error if no ranges are given.


Calling qsort() with such a  value is not a bug as far as the 
second argument (the size of the array with  as address) is 0.


See 7.20.5 http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf


Well after having read this paragraph carefully:

"Pointer arguments on such a call shall still have valid values, as 
described in 7.1.4."


So, my understanding of this paragraph is that even if the size of the 
array is 0, the pointer to the array must be valid. If not, it is UB.



Fred.

Re: [PATCH 0/9] Fix issues detected by clang analyzer.

[Willy Tarreau]

On Mon, Jun 24, 2019 at 10:03:29AM +0200, Tim Düsterhus wrote:
> > Well, some there seem to be real bugs, but others are more a matter of
> > style than correctness and I disagree with a bogus compiler dictating
> > what coding style rules we must respect. Because that compiler is bogus.
> 
> To be clear: This is not a compiler. It's a static analyzer. clang !=
> clang analyzer.

OK maybe but the point remains, we're relying on a tool complaining about
*style* and not about code correctness. History has proven multiple times
that whenever we fall into that trap, we end up with :
  - confusing code which is less and less maintainable over time,
due to the extra checks that lead to nothing and that become very
difficult to adjust.

  - new bugs due to the extra complexity involved in certain checks
or ways to write code.

  - regressions in stable branch by failure to adapt the mangled code
to work properly there.

Yes I'm in favor of fixing bogus code or to adapt fragile constructs,
but not to work around compiler (or analyzer) bugs or stupidities if
they add wrong assumptions, confusion, or suspicion in the code.

> There were a *much* larger number of mistakes reported. I picked the
> ones that either were valid (BUG/MINOR) or that were easily silenced
> (MINOR) and I carefully read the code to make sure that they are valid.

OK but one more reason then to drop any other tendencious ones if it
doesn't even achieve a 100% cleaning!

> > If it emits an error when using sizeof(int) instead of sizeof(*foo) where
> > foo is an pointer to unsigned int, it is bogus. I challenge you to present
> > me a relevant architecture where unsigned changes the allocated size!r
> 
> For C11 it is guaranteed that there is none according to this:
> https://stackoverflow.com/a/13169473/782822

Not surprised at all :-)

> > Please note that I *do* prefer the sizeof(*foo) for readability and long
> > term correctness, it's just that we must not claim a bug in the code when
> > the bug is in the compiler itself (or whatever claims there is a bug).
> 
> Feel free to re-tag that one as MINOR :-)

In my opinion it's a cleanup : it doesn't change the emitted code at all,
and makes the code more robust against changes, and saves one round trip
to the include file for the casual reader trying to figure what caused a
bug to happen.

> > Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
> > of BUG_ON() is to help the *developer*. It should be seen as proven
> > documentation that helps figuring branches when analysing a bug. While
> > the assertion in a comment may have become wrong over time, the assertion
> > in a BUG_ON() is provably true.
> 
> I agree with any *large* changes to make the analyzer happy. In the two
> patches I made it was a minimal change to make. The proxy_parse_declare
> probably could also be changed to:
> 
> > --- a/src/proxy.c
> > +++ b/src/proxy.c
> > @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int 
> > section, struct proxy *curpx,
> > hdr->index = curpx->nb_req_cap++;
> > curpx->req_cap = hdr;
> > }
> > -   if (strcmp(args[2], "response") == 0) {
> > +   else {
> > +   BUG_ON(strcmp(args[2], "response") != 0);
> > hdr->next = curpx->rsp_cap;
> > hdr->index = curpx->nb_rsp_cap++;
> > curpx->rsp_cap = hdr;
> 
> which might be acceptable?

No because that BUG_ON() above is redundant, just put an "else if (strcmp...)"
if you want but I really fail to see the problem there and we're not going
to add confusion in the code to please a shitty compiler or analyzer which
finds bugs where they are not. What you're doing above is mangling the code
to *temporarily* shut up the analyzer. It has nothing to do with addressing
any real issue. Either the analyzer is right and the bug is somewhere else
and must be fixed, or it's wrong and we'd rather ignore it. Otherwise we
can as well use this awesome analyzer and try to fix its reports :

   $ for i in src/*.c; do for j in $(seq 1 $((RANDOM%5+1))); do echo 
"$i:$((RANDOM%2000+1)): variable is used uninitialized in this 
function";done;done

Don't get me wrong, I'm not dismissing everything such analysers can find,
I'm saying that these tools should be used as a help an not as a goal. Each
report needs to be carefully evaluated. In the strcmp() example above,
nothing in your patch addresses a real issue since the code is just a
repetition of something done 5 lines above. If the analyzer is wrong there,
be it!

> > For the crash that clang doesn't detect that the code will stop there,
> 
> Just to make sure: clang *analyzer*. It's nothing an off-the-shelf clang
> will report.

One more reason for not sticking too hard to such wrong reports then!

Thanks,
Willy

Re: [PATCH 2/9] BUG/MINOR: log: Detect missing sampling ranges in config

[Frederic Lecaille]

Hi Tim,

On 6/23/19 10:10 PM, Tim Duesterhus wrote:

Consider a config like:

 global
log 127.0.0.1:10001 sample :10 local0

No sampling ranges are given here, leading to NULL being passed
as the first argument to qsort.

This configuration does not make sense anyway, a log without ranges
would never log. Thus output an error if no ranges are given.


Calling qsort() with such a  value is not a bug as far as the 
second argument (the size of the array with  as address) is 0.


See 7.20.5 http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf


This bug was introduced in d95ea2897eb951c72fd169f36b6a79905f2ed999.
This fix must be backported to HAProxy 2.0.
---
  src/log.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/src/log.c b/src/log.c
index ecbc7ab55..ef999d13f 100644
--- a/src/log.c
+++ b/src/log.c
@@ -929,6 +929,11 @@ int parse_logsrv(char **args, struct list *logsrvs, int 
do_del, char **err)
smp_rgs_sz++;
}
  
+		if (smp_rgs == NULL) {

+   memprintf(err, "no sampling ranges given");
+   goto error;
+   }
+
beg = smp_sz_str;
end = beg + strlen(beg);
new_smp_sz = read_uint((const char **), end);



I think this patch is correct but does not fix a bug. It helps the user 
in debugging its wrong configuration file.



Fred.

Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

[William Lallemand]

On Sun, Jun 23, 2019 at 10:10:12PM +0200, Tim Duesterhus wrote:
> Consider this configuration:
> 
> frontend fe_http
>   mode http
>   bind *:8080
> 
>   default_backend be_http
> 
> backend be_http
>   mode http
>   server example example.com:80
> 
> program foo bar
> 
> Running with valgrind results in:
> 
> ==16252== Invalid read of size 8
> ==16252==at 0x52AE3F: cfg_parse_program (mworker-prog.c:233)
> ==16252==by 0x4823B3: readcfgfile (cfgparse.c:2180)
> ==16252==by 0x47BCED: init (haproxy.c:1649)
> ==16252==by 0x404E22: main (haproxy.c:2714)
> ==16252==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
> 
> Check whether `ext_child` is valid before attempting to free it and its
> contents.
> 
> This bug was introduced in 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f.
> This fix must be backported to HAProxy 2.0.

Thanks, merged.

Also, I renamed the tag from "mworker" to "mworker-prog".

-- 
William Lallemand

Re: [PATCH 0/9] Fix issues detected by clang analyzer.

[Tim Düsterhus]

Willy,

Am 24.06.19 um 07:33 schrieb Willy Tarreau:
> Hi Tim,
> 
> On Sun, Jun 23, 2019 at 10:10:08PM +0200, Tim Duesterhus wrote:
>> Willy,
>>
>> these patches are the result of running
>>
>> scan-build make -j4 all TARGET=linux-glibc DEBUG=-DDEBUG_STRICT=1
>>
>> While it spits out a bunch of false negatives that are quite convoluted these
>> patches either:
>>
>> 1. Make it easier for clang analyzer to understand the code by making a few 
>> *small*
>>adjustments. The one in `proxy_parse_declare` might be questionable, 
>> though.
>> 2. Actually fix an issue I could reproduce with a carefully crafted example
>>configuration.
> 
> Well, some there seem to be real bugs, but others are more a matter of
> style than correctness and I disagree with a bogus compiler dictating
> what coding style rules we must respect. Because that compiler is bogus.

To be clear: This is not a compiler. It's a static analyzer. clang !=
clang analyzer.

There were a *much* larger number of mistakes reported. I picked the
ones that either were valid (BUG/MINOR) or that were easily silenced
(MINOR) and I carefully read the code to make sure that they are valid.

> If it emits an error when using sizeof(int) instead of sizeof(*foo) where
> foo is an pointer to unsigned int, it is bogus. I challenge you to present
> me a relevant architecture where unsigned changes the allocated size!r

For C11 it is guaranteed that there is none according to this:
https://stackoverflow.com/a/13169473/782822

> Please note that I *do* prefer the sizeof(*foo) for readability and long
> term correctness, it's just that we must not claim a bug in the code when
> the bug is in the compiler itself (or whatever claims there is a bug).

Feel free to re-tag that one as MINOR :-)

> Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
> of BUG_ON() is to help the *developer*. It should be seen as proven
> documentation that helps figuring branches when analysing a bug. While
> the assertion in a comment may have become wrong over time, the assertion
> in a BUG_ON() is provably true.

I agree with any *large* changes to make the analyzer happy. In the two
patches I made it was a minimal change to make. The proxy_parse_declare
probably could also be changed to:

> --- a/src/proxy.c
> +++ b/src/proxy.c
> @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int section, 
> struct proxy *curpx,
>   hdr->index = curpx->nb_req_cap++;
>   curpx->req_cap = hdr;
>   }
> - if (strcmp(args[2], "response") == 0) {
> + else {
> + BUG_ON(strcmp(args[2], "response") != 0);
>   hdr->next = curpx->rsp_cap;
>   hdr->index = curpx->nb_rsp_cap++;
>   curpx->rsp_cap = hdr;

which might be acceptable?

> For the crash that clang doesn't detect that the code will stop there,

Just to make sure: clang *analyzer*. It's nothing an off-the-shelf clang
will report.

> I understand the problem but I think that fixing it with an ifdef to
> detect this specific analyzer isn't the right way to do it because
> we'll get it in the face again soon for any other code analyzer. I
> think we should move this to an inline function marked with
> __attribute__((noreturn)) instead.
> 
> I'll have a deeper look, because I think we need some updated tools. If
> compilers get smarter at purposely annoying us, we'll need to more
> ostensibly shut them up when relevant by not telling them what we're
> doing since they appear to have difficulties reading the code they were
> made to compile in the first place...
> 
> I'm having a few ideas that could actually all derive from the
> ALREADY_CHECKED() hack. For example we could use something like this
> to block funny checks and disable certain absurd warnings that made
> us introduce bugs in the past :
> 
> #define FORCE(x) ({ typeof(x) __x__ = (x); asm("" : "=rm"(__x__) : 
> "0"(__x__)); __x__; })
> 
> We could also use __builtin_trap() when defined, or __builtin_unreachable()
> after an asm statement, optionally replaced by a loop when not defined.
> 
> I may propose you a few things to try later today.

Best regards
Tim Düsterhus

Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

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

I had to be mroe detailed :)

I meant "simple reg test" + address sanitizer which we run in travis

address sanitizer is capable of catching those things

пн, 24 июн. 2019 г. в 11:18, Илья Шипицин :

> Does it make sense to also add regtest on that regression?
>
> On Mon, Jun 24, 2019, 1:14 AM Tim Duesterhus  wrote:
>
>> Consider this configuration:
>>
>> frontend fe_http
>> mode http
>> bind *:8080
>>
>> default_backend be_http
>>
>> backend be_http
>> mode http
>> server example example.com:80
>>
>> program foo bar
>>
>> Running with valgrind results in:
>>
>> ==16252== Invalid read of size 8
>> ==16252==at 0x52AE3F: cfg_parse_program (mworker-prog.c:233)
>> ==16252==by 0x4823B3: readcfgfile (cfgparse.c:2180)
>> ==16252==by 0x47BCED: init (haproxy.c:1649)
>> ==16252==by 0x404E22: main (haproxy.c:2714)
>> ==16252==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
>>
>> Check whether `ext_child` is valid before attempting to free it and its
>> contents.
>>
>> This bug was introduced in 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f.
>> This fix must be backported to HAProxy 2.0.
>> ---
>>  src/mworker-prog.c | 30 --
>>  1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/mworker-prog.c b/src/mworker-prog.c
>> index 467ce9b24..ba52406e9 100644
>> --- a/src/mworker-prog.c
>> +++ b/src/mworker-prog.c
>> @@ -230,22 +230,24 @@ int cfg_parse_program(const char *file, int
>> linenum, char **args, int kwm)
>> return err_code;
>>
>>  error:
>> -   LIST_DEL(_child->list);
>> -   if (ext_child->command) {
>> -   int i;
>> -
>> -   for (i = 0; ext_child->command[i]; i++) {
>> -   if (ext_child->command[i]) {
>> -   free(ext_child->command[i]);
>> -   ext_child->command[i] = NULL;
>> +   if (ext_child) {
>> +   LIST_DEL(_child->list);
>> +   if (ext_child->command) {
>> +   int i;
>> +
>> +   for (i = 0; ext_child->command[i]; i++) {
>> +   if (ext_child->command[i]) {
>> +   free(ext_child->command[i]);
>> +   ext_child->command[i] = NULL;
>> +   }
>> }
>> +   free(ext_child->command);
>> +   ext_child->command = NULL;
>> +   }
>> +   if (ext_child->id) {
>> +   free(ext_child->id);
>> +   ext_child->id = NULL;
>> }
>> -   free(ext_child->command);
>> -   ext_child->command = NULL;
>> -   }
>> -   if (ext_child->id) {
>> -   free(ext_child->id);
>> -   ext_child->id = NULL;
>> }
>>
>> free(ext_child);
>> --
>> 2.21.0
>>
>>
>>

Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

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

Does it make sense to also add regtest on that regression?

On Mon, Jun 24, 2019, 1:14 AM Tim Duesterhus  wrote:

> Consider this configuration:
>
> frontend fe_http
> mode http
> bind *:8080
>
> default_backend be_http
>
> backend be_http
> mode http
> server example example.com:80
>
> program foo bar
>
> Running with valgrind results in:
>
> ==16252== Invalid read of size 8
> ==16252==at 0x52AE3F: cfg_parse_program (mworker-prog.c:233)
> ==16252==by 0x4823B3: readcfgfile (cfgparse.c:2180)
> ==16252==by 0x47BCED: init (haproxy.c:1649)
> ==16252==by 0x404E22: main (haproxy.c:2714)
> ==16252==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
>
> Check whether `ext_child` is valid before attempting to free it and its
> contents.
>
> This bug was introduced in 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f.
> This fix must be backported to HAProxy 2.0.
> ---
>  src/mworker-prog.c | 30 --
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/src/mworker-prog.c b/src/mworker-prog.c
> index 467ce9b24..ba52406e9 100644
> --- a/src/mworker-prog.c
> +++ b/src/mworker-prog.c
> @@ -230,22 +230,24 @@ int cfg_parse_program(const char *file, int linenum,
> char **args, int kwm)
> return err_code;
>
>  error:
> -   LIST_DEL(_child->list);
> -   if (ext_child->command) {
> -   int i;
> -
> -   for (i = 0; ext_child->command[i]; i++) {
> -   if (ext_child->command[i]) {
> -   free(ext_child->command[i]);
> -   ext_child->command[i] = NULL;
> +   if (ext_child) {
> +   LIST_DEL(_child->list);
> +   if (ext_child->command) {
> +   int i;
> +
> +   for (i = 0; ext_child->command[i]; i++) {
> +   if (ext_child->command[i]) {
> +   free(ext_child->command[i]);
> +   ext_child->command[i] = NULL;
> +   }
> }
> +   free(ext_child->command);
> +   ext_child->command = NULL;
> +   }
> +   if (ext_child->id) {
> +   free(ext_child->id);
> +   ext_child->id = NULL;
> }
> -   free(ext_child->command);
> -   ext_child->command = NULL;
> -   }
> -   if (ext_child->id) {
> -   free(ext_child->id);
> -   ext_child->id = NULL;
> }
>
> free(ext_child);
> --
> 2.21.0
>
>
>