Hi,
we have the plan to change types we use for zval data. A common place we
use this in is the family of zend_parse_[method_]parameters[_ex]
functions. The issue there is that those a variadic so the compiler
won't notice mistakes. As I was looking into clang internals a bit I
decided to create a plugin to clang's static analyzer to help with this.
The result can be found on
https://github.com/johannes/clang-php-checker/
If you get it compiled and working it can detect such things:
$ cat foo.c
int zend_parse_parameters(int ht, char *, ...);
#define FORMAT "lsd"
char *get_format(int a) {
if (!a) {
return 0;
}
return FORMAT;
}
int **get_location_for_int();
void foo() {
char *c;
int i;
extern int x;
char *format = get_format(x);
zend_parse_parameters(0, format, get_location_for_int(), &c,
&i);
}
$ clang -cc1 -analyze -analyzer-checker=php.ZPPChecker \
-load ./PHPChecker.so foo.c
foo.c:20:3: warning: Type of passed argument &SymRegion{conj_$3{int **}} is of type int ** which did not match expected long * (aka. long *) for modifier 'l' at offset 3
zend_parse_parameters(0, format, get_location_for_int(), &c, &i);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
foo.c:20:3: warning: Too few arguments for format "lsd" while checking for modifier 'd'
zend_parse_parameters(0, format, get_location_for_int(), &c, &i);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
The part of "&SymRegion{conj_$3{int **}}" isn't really nice, yet,
usually you will have variable names there which will be printed
instead, this example was supposed to show a more complex case. (this
example is held simple and missing i.e. TSRM parameters, they are
supported in ZTS and non-ZTS-mode)
Running this over one of my PHP builds I got those error reports:
http://schlueters.de/zppcheck/ maybe somebody wants to go through them
and fix at least the trivial ones.
The checker plugin currently only knows about PHP 5.5's zpp modifiers,
support for the size_t branch will be added (it's trivial to add, I'm
happy about pull requests, check for PHPSample in the code!)
If there is interest we can add such checks for other PHP-specific
things.
johannes
On Mon, Feb 17, 2014 at 9:42 PM, Johannes Schlüter
johannes@schlueters.de wrote:
Hi,
we have the plan to change types we use for zval data. A common place we
use this in is the family of zend_parse_[method_]parameters[_ex]
functions. The issue there is that those a variadic so the compiler
won't notice mistakes. As I was looking into clang internals a bit I
decided to create a plugin to clang's static analyzer to help with this.
The result can be found on
https://github.com/johannes/clang-php-checker/If you get it compiled and working it can detect such things:
$ cat foo.c int zend_parse_parameters(int ht, char *, ...); #define FORMAT "lsd" char *get_format(int a) { if (!a) { return 0; } return FORMAT; } int **get_location_for_int(); void foo() { char *c; int i; extern int x; char *format = get_format(x); zend_parse_parameters(0, format, get_location_for_int(), &c, &i); } $ clang -cc1 -analyze -analyzer-checker=php.ZPPChecker \ -load ./PHPChecker.so foo.c foo.c:20:3: warning: Type of passed argument &SymRegion{conj_$3{int **}} is of type int ** which did not match expected long * (aka. long *) for modifier 'l' at offset 3 zend_parse_parameters(0, format, get_location_for_int(), &c, &i); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ foo.c:20:3: warning: Too few arguments for format "lsd" while checking for modifier 'd' zend_parse_parameters(0, format, get_location_for_int(), &c, &i); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 warnings generated.The part of "&SymRegion{conj_$3{int **}}" isn't really nice, yet,
usually you will have variable names there which will be printed
instead, this example was supposed to show a more complex case. (this
example is held simple and missing i.e. TSRM parameters, they are
supported in ZTS and non-ZTS-mode)Running this over one of my PHP builds I got those error reports:
http://schlueters.de/zppcheck/ maybe somebody wants to go through them
and fix at least the trivial ones.The checker plugin currently only knows about PHP 5.5's zpp modifiers,
support for the size_t branch will be added (it's trivial to add, I'm
happy about pull requests, check for PHPSample in the code!)If there is interest we can add such checks for other PHP-specific
things.johannes
--
Not sure about some :-p
https://github.com/jpauli/php-src/compare/zpp_type_fixed
Julien
Julien,
<snip> Running this over one of my PHP builds I got those error reports:
http://schlueters.de/zppcheck/ maybe somebody wants to go through them
and fix at least the trivial ones.
By the way, your report seems to be listing entries twice -- at least as
the four PHPdbg SAPI ones. In this case these are all a case of the "s"
parameter taking a char**, int* but phpdbg.c is passing in a char**,
uint*. I'll issue a patch for Joe or Bob to review and push into PHP-5.6.
Note that http://www.php.net/manual/en/internals2.funcs.php states that
the "s" parameter takes the addresses of char*, uint that is char**
uint* which is wrong. Though why the length field should be signed
seems odd to me as a negative length makes no sense at all.
Regards Terry
<snip> Running this over one of my PHP builds I got those error reports:
http://schlueters.de/zppcheck/ maybe somebody wants to go through them
and fix at least the trivial ones.
By the way, your report seems to be listing entries twice -- at least
as the four PHPdbg SAPI ones.
Interesting catch, have to investigate.
In this case these are all a case of the "s" parameter taking a
char**, int* but phpdbg.c is passing in a char**, uint*. I'll issue a
patch for Joe or Bob to review and push into PHP-5.6.
Thanks.
Note that http://www.php.net/manual/en/internals2.funcs.php states
that the "s" parameter takes the addresses of char*, uint that is
char** uint* which is wrong.
Indeed a bug, will fix that right after sending this mail. Thanks for
noticing.
Though why the length field should be signed seems odd to me as a
negative length makes no sense at all.
- since it was defined that way
- since int is a simple choice
- since signed int has enough space for all expected strings
- since it can quite certainly passed to any library, it is
unlikely a library will choose int4_t or such as length
for strings and maxlength handling can now be in one place - since it is supposed to be fixed (see recent looong discussion)
;-)
johannes
Julien,
<snip> Running this over one of my PHP builds I got those error reports:
http://schlueters.de/zppcheck/ maybe somebody wants to go through them
and fix at least the trivial ones.By the way, your report seems to be listing entries twice -- at least as
the four PHPdbg SAPI ones. In this case these are all a case of the "s"
parameter taking a char**, int* but phpdbg.c is passing in a char**, uint*.
I'll issue a patch for Joe or Bob to review and push into PHP-5.6.Note that http://www.php.net/manual/en/internals2.funcs.php states that
the "s" parameter takes the addresses of char*, uint that is char** uint*
which is wrong. Though why the length field should be signed seems odd to
me as a negative length makes no sense at all.
Right, that is something we have fixes in the int64 and sizet branch.
Cheers,
Pierre