# Static Analysis of PHP_5_4 with CLANG - Thread: https://externals.io/message/57741 - Original archive: https://news-web.php.net/php.internals/57741 - Started by Sebastian Bergmann on 2012-02-05T19:53:04+00:00 - Messages: 8 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #57741 — Sebastian Bergmann — 2012-02-05T19:53:04+00:00 URL: https://externals.io/message/57741#57741 http://clang-php54.phpunit.de/ might be of interest to some. http://bit.ly/u06eCD has details of how to produce this report. -- Sebastian Bergmann Co-Founder and Principal Consultant http://sebastian-bergmann.de/ http://thePHP.cc/ --- ## #57748 — Pierre Joye — 2012-02-05T21:21:03+00:00 URL: https://externals.io/message/57741#57748 · In reply to: #57741 hi, Nice :) Btw, we have that using the VC++ static analyzer for 5.3/5.4/trunk for (almost) all revisions. See the builds logs here: http://windows.php.net/downloads/snaps/ Cheers, >  http://clang-php54.phpunit.de/ might be of interest to some. >  http://bit.ly/u06eCD has details of how to produce this report. > > -- > Sebastian Bergmann                    Co-Founder and Principal Consultant > http://sebastian-bergmann.de/                           http://thePHP.cc/ > > -- > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #57749 — Stas Malyshev — 2012-02-05T21:58:13+00:00 URL: https://externals.io/message/57741#57749 · In reply to: #57741 Hi! > http://clang-php54.phpunit.de/ might be of interest to some. > http://bit.ly/u06eCD has details of how to produce this report. Thanks, definitely is a useful thing. Though I'm not sure if this tool is always right. For example, I looked at http://clang-php54.phpunit.de/report-dqJzsw.html#EndPath, and it claims in line 1331 replacement is garbage. However, the only way to get there is to pass through (flags & ENT_HTML_SUBSTITUTE_DISALLOWED_CHARS), and if (flags & ENT_HTML_SUBSTITUTE_DISALLOWED_CHARS) is not 0 then replacement is initialized in lines 1245-1252 by one of the clauses. Looks like this tool does not remember the branches it took before. Am I missing something here or should we submit a bug report to CLANG devs? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57750 — Stas Malyshev — 2012-02-05T22:08:17+00:00 URL: https://externals.io/message/57741#57750 · In reply to: #57749 Hi! Another thing - is there any way to give CLANG some hints about some functions - such as the fact that zend_error(E_ERROR) does not return or just make some exceptions when we know some situation that it thinks can happen does not in fact happen - such as revtal_ptr_ptr = &retval_ptr and then passing retval_ptr_ptr would actually change retval_ptr somewhere on the way, etc. The idea is if we could weed out false positives and somehow mark them we could use this tool in CI for detecting real errors. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57751 — Rasmus Lerdorf — 2012-02-05T22:23:50+00:00 URL: https://externals.io/message/57741#57751 · In reply to: #57749 > Hi! > > > […] > > Thanks, definitely is a useful thing. Though I'm not sure if this tool > is always right. For example, I looked at > http://clang-php54.phpunit.de/report-dqJzsw.html#EndPath, and it claims > in line 1331 replacement is garbage. However, the only way to get there > is to pass through (flags & ENT_HTML_SUBSTITUTE_DISALLOWED_CHARS), and > if (flags & ENT_HTML_SUBSTITUTE_DISALLOWED_CHARS) is not 0 then > replacement is initialized in lines 1245-1252 by one of the clauses. > Looks like this tool does not remember the branches it took before. Am I > missing something here or should we submit a bug report to CLANG devs? I checked scan.coverity and it didn't find any problems in ext/standard/html.c The Coverity scan does find a bunch of possible null-dereferences. For example, in zend_compile.c: 3075 if (!strcasecmp(arg_info->class_name, "self") && fptr->common.scope ) { 3076 class_name = fptr->common.scope->name; 3077 class_name_len = fptr->common.scope->name_length; 3078 } else if (!strcasecmp(arg_info->class_name, "parent") && fptr->common.scope->parent) { The if on 3075 implies that fptr->common.scope could be null and then in the else if on 3078 fptr->common.scope->parent is checked. Now, it probably means that every time arg_info->class_name is set to "parent" fptr->common.scope will be defined, but a static analyzer isn't able to detect that. Note also that this code only appears in trunk. The 5.4 code is completely different. Was this a missed trunk commit? -Rasmus --- ## #57753 — Gustavo Lopes — 2012-02-05T23:04:27+00:00 URL: https://externals.io/message/57741#57753 · In reply to: #57750 > Another thing - is there any way to give CLANG some hints about some > functions - such as the fact that zend_error(E_ERROR) does not return > or just make some exceptions when we know some situation that it > thinks can happen does not in fact happen - such as revtal_ptr_ptr = > &retval_ptr and then passing retval_ptr_ptr would actually change > retval_ptr somewhere on the way, etc. > The idea is if we could weed out false positives and somehow mark > them we could use this tool in CI for detecting real errors. Perhaps those instances could be replaced by zend_error_noreturn. That variant has the "noreturn" attribute, which signals the function never returns. This useful for suppressing those kind of warnings (note C11 also added a standard __Noreturn). -- Gustavo Lopes --- ## #57756 — Gwynne Raskind — 2012-02-06T01:50:59+00:00 URL: https://externals.io/message/57741#57756 · In reply to: #57750 > Hi! > > Another thing - is there any way to give CLANG some hints about some > functions - such as the fact that zend_error(E_ERROR) does not return or > just make some exceptions when we know some situation that it thinks can > happen does not in fact happen - such as revtal_ptr_ptr = &retval_ptr and > then passing retval_ptr_ptr would actually change retval_ptr somewhere on > the way, etc. > The idea is if we could weed out false positives and somehow mark them we > could use this tool in CI for detecting real errors. Clang (and its static analyzer) has a huge number of annotative attributes for everything which can be easily ifdef'd to nothing on non-Clang. gives most of them, and Clang also has the (nearly) full range of GCC's attributes. You can also disable the analyzer for non-annotatable false positives with a #if block (I forget the name of the macro off the top of my head). The most recent versions of clang should be able to trace aliased pointer paths to some extent, so if you're getting false positives there, it's possible there's a subtle issue (I haven't looked at the actual output). -- Gwynne --- ## #57758 — Laruence — 2012-02-06T05:52:15+00:00 URL: https://externals.io/message/57741#57758 · In reply to: #57751 > > > […] > > I checked scan.coverity and it didn't find any problems in > ext/standard/html.c > > The Coverity scan does find a bunch of possible null-dereferences. > > For example, in zend_compile.c: > > 3075                                if > (!strcasecmp(arg_info->class_name, "self") && fptr->common.scope ) { > 3076                                        class_name = > fptr->common.scope->name; > 3077                                        class_name_len = > fptr->common.scope->name_length; > 3078                                } else if > (!strcasecmp(arg_info->class_name, "parent") && > fptr->common.scope->parent) { > > The if on 3075 implies that fptr->common.scope could be null and then in > the else if on 3078 fptr->common.scope->parent is checked. Now, it > probably means that every time arg_info->class_name is set to "parent" > fptr->common.scope will be defined, but a static analyzer isn't able to > detect that. Note also that this code only appears in trunk. The 5.4 > code is completely different. Was this a missed trunk commit? Rasmus: this is a fix for #60573. since 5.4 is in freeze period, so I hold from 5.4. and IMO if the classname is set to "self", then the scope could not be NULL, obviously the code is in-appropriate. I will fix it. Dmitry, could you give me some advise on this plz? thanks > > -Rasmus > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/