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
