Re: clang static analysis of haproxy

[Willy Tarreau]

On Wed, Oct 26, 2016 at 01:33:24PM +0500,  ?? wrote:
> properly tuned travis-ci moves the pressure from maintainer to contributor,
> exactly like you told above :-)
> 
> https://travis-ci.org/openresty/lua-nginx-module/pull_requests
> 
> PR are being automatically tested (build matrix might be "gcc, clang" x
> "linux, osx", even more wide if you use docker here)

Most of the problems I have with contributions are actually not in the
code, as code can be fixed, but the scarcity of commit messages. People
send a patch and disappear. Then all of us have to deal with that patch
for 15 years, regularly scratching our head every time it is suspected
of participating to a bug, trying to find whether or not what was done
there was intended or not. And even for backports the details are very
important. In general when commit messages are of good quality I merge
them almost without reading the code, because I know that anyone will be
able to amend it later if needed. I'm not sure travis-ci will take care
of this.

Willy

Re: clang static analysis of haproxy

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

2016-10-26 13:26 GMT+05:00 Willy Tarreau :

> On Wed, Oct 26, 2016 at 01:15:43PM +0500,  ?? wrote:
> > as time goes on ... I see, haproxy moved to github ?
>
> No, it's another fork dedicated to travis that causes some confusion
> and that we're trying to smoothly move to fix this recurrent problem.
>
> > I like github, especially for PR and travis.
>
> Ah that's bad because PRs are exactly the reason I don't like github :
> they move the burden from contributors to maintainers in that it is
> really painful to request good quality patches and even more to fix
> the minor issues they have. And more importantly, it doesn't show
> people how to contribute good code nor does it incite people to start
> to participate. We all started by commenting a patch received in a
> mail where we spotted an obvious bug or wanted to give an opinion. It
> it important to keep this. We're in fact even setting up something to
> automatically forward the PRs to the list so that everyone can
> participate, and automatically close them since github doesn't offer
> the option to disable them.
>
> > should I open a PR on those (not very critial) issues ?
>
> Don't do it yet please, Lukas is currently trying to get rid of all
> of them in order to move the current repository.
>
> > and what do you think of travis-ci ? I have some experience in using it,
> > very nice.
>
> No opinion, I don't know it at all, but that's exactly the purpose of
> the current "haproxy" project on github, it contains some patches for
> travis maintained by Jeff. That's all I know about it. Maybe if we
> integrate this cleanly Jeff will not even need to maintain this repo
> anymore.
>


properly tuned travis-ci moves the pressure from maintainer to contributor,
exactly like you told above :-)

https://travis-ci.org/openresty/lua-nginx-module/pull_requests

PR are being automatically tested (build matrix might be "gcc, clang" x
"linux, osx", even more wide if you use docker here)


>
> Regards,
> Willt
>

Re: clang static analysis of haproxy

[Willy Tarreau]

On Wed, Oct 26, 2016 at 01:15:43PM +0500,  ?? wrote:
> as time goes on ... I see, haproxy moved to github ?

No, it's another fork dedicated to travis that causes some confusion
and that we're trying to smoothly move to fix this recurrent problem.

> I like github, especially for PR and travis.

Ah that's bad because PRs are exactly the reason I don't like github :
they move the burden from contributors to maintainers in that it is
really painful to request good quality patches and even more to fix
the minor issues they have. And more importantly, it doesn't show
people how to contribute good code nor does it incite people to start
to participate. We all started by commenting a patch received in a
mail where we spotted an obvious bug or wanted to give an opinion. It
it important to keep this. We're in fact even setting up something to
automatically forward the PRs to the list so that everyone can
participate, and automatically close them since github doesn't offer
the option to disable them.

> should I open a PR on those (not very critial) issues ?

Don't do it yet please, Lukas is currently trying to get rid of all
of them in order to move the current repository.

> and what do you think of travis-ci ? I have some experience in using it,
> very nice.

No opinion, I don't know it at all, but that's exactly the purpose of
the current "haproxy" project on github, it contains some patches for
travis maintained by Jeff. That's all I know about it. Maybe if we
integrate this cleanly Jeff will not even need to maintain this repo
anymore.

Regards,
Willt

Re: clang static analysis of haproxy

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

as time goes on ... I see, haproxy moved to github ?
I like github, especially for PR and travis.

should I open a PR on those (not very critial) issues ?

and what do you think of travis-ci ? I have some experience in using it,
very nice.

2016-06-07 15:20 GMT+05:00 Willy Tarreau :

> Hi,
>
> On Mon, Jun 06, 2016 at 09:41:48PM +0500,  ?? wrote:
> > Hello,
> >
> > I run scan-build (which is clang static analysis tool) on haproxy git
> source
> >
> > here's result
> >
> > http://chipitsine.github.io/haproxy-1.7-dev/
> >
> >
> > there some null pointer dereferences and memory leaks, I think worth
> > looking at them.
>
> Thanks for doing this. As usual with such tools, a lot are false alarms
> but when looking at the logic error ones, I'm seeing this one which is
> totally valid :
>
>http://chipitsine.github.io/haproxy-1.7-dev/report-bf7c44.html#EndPath
>
> (the parenthesis is misplaced, it should be removed).
>
> This one is wrong :
>http://chipitsine.github.io/haproxy-1.7-dev/report-562959.html#EndPath
>(s is never NULL)
>
> This one as well :
>http://chipitsine.github.io/haproxy-1.7-dev/report-562959.html#EndPath
>(LIST_NEXT is valid if LIST_ISEMPTY is false)
>
> This one is right (but split in multiple reports) :
>http://chipitsine.github.io/haproxy-1.7-dev/report-562959.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-38458e.html#EndPath
>(*err=*merr may exhibit the same issue). It will not happend from all
>the call places anyway, but I guess there's something wrong with the
>whole code path, as _merr seems to be sort of a fallback for err and
>is not used as such.
>
> This one is wrong :
>http://chipitsine.github.io/haproxy-1.7-dev/report-a9cda9.html#EndPath
>(stktable_data_cast() cannot return NULL for a valid ptr, which is known
> to be valid because the data type is stored in the table according to
> some earlier tests).
>
> This one is wrong :
>http://chipitsine.github.io/haproxy-1.7-dev/report-ece278.html#EndPath
>(the loop must have at least one iteration since the server list was
> checked to have at least one server).
>
> This one is wrong :
>http://chipitsine.github.io/haproxy-1.7-dev/report-ba43f3.html#EndPath
>(a server always belongs to a proxy).
>
> This one is wrong :
>http://chipitsine.github.io/haproxy-1.7-dev/report-aafd4e.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-859fe6.html#EndPath
>(curmailers is global and is not null here).
>
> Same here with curr_resolvers :
>http://chipitsine.github.io/haproxy-1.7-dev/report-7d9a8b.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-678199.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-3c1789.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-c929f2.html#EndPath
>
> And here with curpeers :
>http://chipitsine.github.io/haproxy-1.7-dev/report-de79db.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-403a2e.html#EndPath
>http://chipitsine.github.io/haproxy-1.7-dev/report-ffb215.html#EndPath
>
> This one is wrong because tmplog is always assigned non-null values :
>http://chipitsine.github.io/haproxy-1.7-dev/report-9872e8.html#EndPath
>
> This is the one for which you proposed a patch :
>http://chipitsine.github.io/haproxy-1.7-dev/report-0bb120.html#EndPath
>By the way an error message is missing in your patch, to mention that
>either "request" or "response" is expected here.
>
> This one is right :
>http://chipitsine.github.io/haproxy-1.7-dev/report-df4c7e.html#EndPath
>(like the previous one, it's not important since we're aborting the
> startup, but better fix it).
>
> This one is interesting :
>http://chipitsine.github.io/haproxy-1.7-dev/report-93fb8d.html#EndPath
>It is not possible because a pattern taking type SMP_T_BOOL will not
>have the pat_parse_dotted_ver() parser, but the analyser was quite good
>at finding its way through all these conditions and picking that
>combination. We can definitely initialize minor to 0 just for the sake
>of easier debugging (it may later cause an extra report of unused store
>but we don't care, it's annoying to read garbage in gdb, better stay on
>zero).
>
> Overall I find that the reports are quick to read, quick to eliminate or
> make one wonder the good questions. Thanks for reporting this. You're
> welcome to provide patches for the good ones above if you want :-)
>
> Willy
>

Re: clang static analysis of haproxy

[Willy Tarreau]

Hi,

On Mon, Jun 06, 2016 at 09:41:48PM +0500,  ?? wrote:
> Hello,
> 
> I run scan-build (which is clang static analysis tool) on haproxy git source
> 
> here's result
> 
> http://chipitsine.github.io/haproxy-1.7-dev/
> 
> 
> there some null pointer dereferences and memory leaks, I think worth
> looking at them.

Thanks for doing this. As usual with such tools, a lot are false alarms
but when looking at the logic error ones, I'm seeing this one which is
totally valid :

   http://chipitsine.github.io/haproxy-1.7-dev/report-bf7c44.html#EndPath

(the parenthesis is misplaced, it should be removed).

This one is wrong :
   http://chipitsine.github.io/haproxy-1.7-dev/report-562959.html#EndPath
   (s is never NULL)

This one as well :
   http://chipitsine.github.io/haproxy-1.7-dev/report-562959.html#EndPath
   (LIST_NEXT is valid if LIST_ISEMPTY is false)

This one is right (but split in multiple reports) :
   http://chipitsine.github.io/haproxy-1.7-dev/report-562959.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-38458e.html#EndPath
   (*err=*merr may exhibit the same issue). It will not happend from all
   the call places anyway, but I guess there's something wrong with the
   whole code path, as _merr seems to be sort of a fallback for err and
   is not used as such.

This one is wrong :
   http://chipitsine.github.io/haproxy-1.7-dev/report-a9cda9.html#EndPath
   (stktable_data_cast() cannot return NULL for a valid ptr, which is known
to be valid because the data type is stored in the table according to
some earlier tests).

This one is wrong :
   http://chipitsine.github.io/haproxy-1.7-dev/report-ece278.html#EndPath
   (the loop must have at least one iteration since the server list was
checked to have at least one server).

This one is wrong :
   http://chipitsine.github.io/haproxy-1.7-dev/report-ba43f3.html#EndPath
   (a server always belongs to a proxy).

This one is wrong :
   http://chipitsine.github.io/haproxy-1.7-dev/report-aafd4e.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-859fe6.html#EndPath
   (curmailers is global and is not null here).

Same here with curr_resolvers :
   http://chipitsine.github.io/haproxy-1.7-dev/report-7d9a8b.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-678199.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-3c1789.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-c929f2.html#EndPath

And here with curpeers :
   http://chipitsine.github.io/haproxy-1.7-dev/report-de79db.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-403a2e.html#EndPath
   http://chipitsine.github.io/haproxy-1.7-dev/report-ffb215.html#EndPath

This one is wrong because tmplog is always assigned non-null values :
   http://chipitsine.github.io/haproxy-1.7-dev/report-9872e8.html#EndPath

This is the one for which you proposed a patch :
   http://chipitsine.github.io/haproxy-1.7-dev/report-0bb120.html#EndPath
   By the way an error message is missing in your patch, to mention that
   either "request" or "response" is expected here.

This one is right :
   http://chipitsine.github.io/haproxy-1.7-dev/report-df4c7e.html#EndPath
   (like the previous one, it's not important since we're aborting the
startup, but better fix it).

This one is interesting :
   http://chipitsine.github.io/haproxy-1.7-dev/report-93fb8d.html#EndPath
   It is not possible because a pattern taking type SMP_T_BOOL will not
   have the pat_parse_dotted_ver() parser, but the analyser was quite good
   at finding its way through all these conditions and picking that
   combination. We can definitely initialize minor to 0 just for the sake
   of easier debugging (it may later cause an extra report of unused store
   but we don't care, it's annoying to read garbage in gdb, better stay on
   zero).

Overall I find that the reports are quick to read, quick to eliminate or
make one wonder the good questions. Thanks for reporting this. You're
welcome to provide patches for the good ones above if you want :-)

Willy

Re: clang static analysis of haproxy

[Willy Tarreau]

Hi,

On Tue, Jun 07, 2016 at 01:00:40AM +0500,  ?? wrote:
> like that ?

Looks good indeed, but I'd add something :

> --- src/proxy.c.orig2016-06-07 00:56:29.001214621 +0500
> +++ src/proxy.c2016-06-07 00:59:01.823568543 +0500
> @@ -426,11 +426,14 @@
>  hdr->index = curpx->nb_req_cap++;
>  curpx->req_cap = hdr;
>  }
> -if (strcmp(args[2], "response") == 0) {
> +else if (strcmp(args[2], "response") == 0) {
>  hdr->next = curpx->rsp_cap;
>  hdr->index = curpx->nb_rsp_cap++;
>  curpx->rsp_cap = hdr;
>  }
> +else {
> +free(hdr);

Here, please add this line :

   hdr = NULL;

It serves as a safety guard against a double free or a use-after-free if
someone later adds more code after this statement and the condition becomes
messed up. It's generally good hygiene to assign nulls after a free.

Please submit a complete patch with a commit message, I'll happily merge it.
(Just read CONTRIBUTING if you're unsure).

thanks,
Willy

Re: clang static analysis of haproxy

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

like that ?


--- src/proxy.c.orig2016-06-07 00:56:29.001214621 +0500
+++ src/proxy.c2016-06-07 00:59:01.823568543 +0500
@@ -426,11 +426,14 @@
 hdr->index = curpx->nb_req_cap++;
 curpx->req_cap = hdr;
 }
-if (strcmp(args[2], "response") == 0) {
+else if (strcmp(args[2], "response") == 0) {
 hdr->next = curpx->rsp_cap;
 hdr->index = curpx->nb_rsp_cap++;
 curpx->rsp_cap = hdr;
 }
+else {
+free(hdr);
+}
 return 0;
 }
 else {


2016-06-07 0:34 GMT+05:00 Sasha Pachev :

> I took a quick look. The first one I looked at was a minor issue, but
> still legitimate.
>
> http://chipitsine.github.io/haproxy-1.7-dev/report-0bb120.html#EndPath
>
> line 419
>
> hdr should be freed if args[2] is neither "request" nor "response",
> otherwise the allocated address is permanently forgotten.
>
>
> On Mon, Jun 6, 2016 at 12:52 PM, Maciej Katafiasz
>  wrote:
> > On 6 June 2016 at 09:41, Илья Шипицин  wrote:
> >> also, is there some bug tracker ? CI (like travis-ci or jenkins) ?
> >
> > No, this list is the place to report things.
> >
> > Cheers,
> > Maciej
> >
>
>
>
> --
> Sasha Pachev
>
> Fast Running Blog.
> http://fastrunningblog.com
> Run. Blog. Improve. Repeat.
>

Re: clang static analysis of haproxy

[Sasha Pachev]

I took a quick look. The first one I looked at was a minor issue, but
still legitimate.

http://chipitsine.github.io/haproxy-1.7-dev/report-0bb120.html#EndPath

line 419

hdr should be freed if args[2] is neither "request" nor "response",
otherwise the allocated address is permanently forgotten.


On Mon, Jun 6, 2016 at 12:52 PM, Maciej Katafiasz
 wrote:
> On 6 June 2016 at 09:41, Илья Шипицин  wrote:
>> also, is there some bug tracker ? CI (like travis-ci or jenkins) ?
>
> No, this list is the place to report things.
>
> Cheers,
> Maciej
>



-- 
Sasha Pachev

Fast Running Blog.
http://fastrunningblog.com
Run. Blog. Improve. Repeat.

Re: clang static analysis of haproxy

[Maciej Katafiasz]

On 6 June 2016 at 09:41, Илья Шипицин  wrote:
> also, is there some bug tracker ? CI (like travis-ci or jenkins) ?

No, this list is the place to report things.

Cheers,
Maciej