Fixed bug #68638 pg_update() fails to store infinite values. #1046
Conversation
Regex to check float values changed to accept "infinity" values and ignore case. Quotes are added to prevent syntax error on PostgreSQL.
|
A test script (phpt) included is better |
|
@laruence to test this patch i need connection with database, table and data. There is a way to mock this on test script (phpt) ? |
|
@wfelipew please check out other test scripts under ext/pgsql/tests |
|
Also, this does not look like security issue, so it's probably not getting into 5.4. |
|
@smalyshev right, only for 5.5+ |
|
@laruence Test script included |
|
@laruence @smalyshev |
|
@wfelipew I will look tomorrow.. probably no .. we can cherry-pick it |
ext/pgsql/pgsql.c
Outdated
There was a problem hiding this comment.
Is for single quote values, because if you send "infinity" values without quotes PostgreSQL throws it syntax error.
Since PostgreSQL accept single quotes for all kind of values, i put this without check if is a "inifinity" or not.
There was a problem hiding this comment.
then this will slow down all pathes
There was a problem hiding this comment.
Ok, i will change it to only add quotes on "infinity" values.
Add quotes only if "infinity" values are received.
|
@laruence look if now is good |
ext/pgsql/pgsql.c
Outdated
There was a problem hiding this comment.
you still introduce a new convert_match for all paths..
why not do:
if (php_pgsql_convert_match() == FAILURE){
if (php_pgsql_convert_match("infinite") == SUCCESS) {
add_quotes
} else {
err = 1;
}
} else {
//keep fast path un-touched
}is that clear?
There was a problem hiding this comment.
Yes, its clear.. but now doing some tests i see that using this approachs (check with php_pgsql_convert_match to only add quotes on 'infinity' values) generates notices when the regex not match.
subs = (regmatch_t *)ecalloc(sizeof(regmatch_t), re.re_nsub+1);
regerr = regexec(&re, str, re.re_nsub+1, subs, 0);
if (regerr == REG_NOMATCH) {
#ifdef PHP_DEBUG
php_error_docref(NULL TSRMLS_CC, E_NOTICE, "'%s' does not match with '%s'", str, regex);
#endif
ret = FAILURE;
}Test result:
Notice: pg_update(): 'inf' does not match with '^([+-]{0,1}[0-9]+)|([+-]{0,1}[0-9]*[\.][0-9]+)|([+-]{0,1}[0-9]+[\.][0-9]*)$' in /opt/php/test_pgsql.php on line 19
string(52) "UPDATE "test_68638" SET "value"=E'inf' WHERE "id"=1;"
Notice: pg_update(): 'inf' does not match with '^([+-]{0,1}[0-9]+)|([+-]{0,1}[0-9]*[\.][0-9]+)|([+-]{0,1}[0-9]+[\.][0-9]*)$' in /opt/php/test_pgsql.php on line 21
Notice: pg_update(): '-inf' does not match with '^([+-]{0,1}[0-9]+)|([+-]{0,1}[0-9]*[\.][0-9]+)|([+-]{0,1}[0-9]+[\.][0-9]*)$' in /opt/php/test_pgsql.php on line 22
Notice: pg_update(): '+inf' does not match with '^([+-]{0,1}[0-9]+)|([+-]{0,1}[0-9]*[\.][0-9]+)|([+-]{0,1}[0-9]+[\.][0-9]*)$' in /opt/php/test_pgsql.php on line 23
I think one possible solutions to avoid this notice is to fork the "php_pgsql_convert_match" to "php_pgsql_match" that not generate this notice and use this function on the first "if" to not generate notice.
Like this:
if (php_pgsql_match() == FAILURE){
if (php_pgsql_convert_match("infinite") == SUCCESS) {
add_quotes
} else {
err = 1;
}
} else {
//keep fast path un-touched
}There was a problem hiding this comment.
Other solution is that:
else {
/* FIXME: better regex must be used */
if (php_pgsql_convert_match(Z_STRVAL_PP(val), Z_STRLEN_PP(val), "^([+-]{0,1}[0-9]+)|([+-]{0,1}[0-9]*[\\.][0-9]+)|([+-]{0,1}[0-9]+[\\.][0-9]*)|([+-]{0,1}(inf)(inity){0,1})$", 1 TSRMLS_CC) == FAILURE) {
err = 1;
}
else {
ZVAL_STRING(new_val, Z_STRVAL_PP(val), 1);
if(strcasestr(Z_STRVAL_PP(val),"inf")!=0){
php_pgsql_add_quotes(new_val, 1 TSRMLS_CC);
}
}
}Do you think this strcasestr in all paths can significantly slow execution ?
There was a problem hiding this comment.
hmm, looks a little ugly, but if you don't have better fix.. then I think we have to do in this way?
Check if value is "infinity" with strcasestr() to add quotes
|
@laruence check the last commit.. i think this is the better solution |
ext/pgsql/pgsql.c
Outdated
|
@wfelipew okey, I am going to merge it... thanks |
|
@laruence Indent fixed |
|
sorry for delay, maybe do final check in weekend. thanks |
|
merged 5.5+ , 5.4 is security fix only.. but strcasestr is _GNU_SOURCE only... thus at last I still prefer use the first idea I talked before..I remove the E_NOTICE if match fails. thanks |
|
Close as it's applied already. Thanks. |
Regex to check float values changed to accept "infinity" values and ignore case.
Quotes are added to prevent syntax error on PostgreSQL.