php_odbc_fetch_hash() Fixed a segfault when fetching certain SQL NULLs

[brandonkirsch]

Fix for https://bugs.php.net/bug.php?id=61387

Changed conditional @ php_odbc.c:1774 to include the check:

     && Z_TYPE_P(tmp) != IS_NULL

This prevents a potential segfault on 1775 when zend_hash_update is called with Z_STR*(tmp) args.

Example script ("fetch_array.php"):

Backtrace:

Program received signal SIGSEGV, Segmentation fault.
zend_inline_hash_func (nKeyLength=, arKey=0x0) at /usr/src/php_fix/php-src/Zend/zend_hash.h:283
283 case 1: hash = ((hash << 5) + hash) + *arKey++; break;
#0 zend_inline_hash_func (nKeyLength=, arKey=0x0) at /usr/src/php_fix/php-src/Zend/zend_hash.h:283
#1 _zend_hash_add_or_update (ht=0xfce7d0, arKey=0x0, nKeyLength=1, pData=0x7fffffffab80, nDataSize=8, pDest=0x0, flag=1) at /usr/src/php_fix/php-src/Zend/zend_hash.c:218
#2 0x00000000005752af in php_odbc_fetch_hash (ht=, return_value=0xfcd038, result_type=2, return_value_ptr=, this_ptr=,

return_value_used=) at /usr/src/php_fix/php-src/ext/odbc/php_odbc.c:1775

[ircmaxell]

Looks good to me. The only thing I'd suggest is targeting the PR against 5.3, so that it can be merged up easier.

Other than that, the fix seems good (fixes a logic error where the type of the zval isn't the same)...

[LawnGnome]

Should this be Z_TYPE_P(tmp) == IS_STRING, rather than not IS_NULL? From context, it's obvious that the only two possible zval types for tmp are IS_STRING and IS_NULL, but this strikes me as a potential gotcha if the function is changed down the track to return more native PHP types for numeric values.

[brandonkirsch]

Adam I think you are right about that. Changing to Z_TYPE_P(tmp) ==
IS_STRING seems like a better way to avoid the problem.

I will ask for help on IRC for how to submit a better fix.

Thanks,
Brandon Kirsch

On Wed, Sep 12, 2012 at 11:13 PM, Adam Harvey notifications@github.comwrote:

In ext/odbc/php_odbc.c:

@@ -1765,7 +1765,7 @@ static void php_odbc_fetch_hash(INTERNAL_FUNCTION_PARAMETERS, int result_type)
if (result_type & ODBC_NUM) {
zend_hash_index_update(Z_ARRVAL_P(return_value), i, &tmp, sizeof(zval *), NULL);
} else {

•       if (!*(result->values[i].name)) {
  

•       if (!*(result->values[i].name) && Z_TYPE_P(tmp) != IS_NULL) {
  

Should this be Z_TYPE_P(tmp) == IS_STRING, rather than not IS_NULL? From
context, it's obvious that the only two possible zval types for tmp are
IS_STRING and IS_NULL, but this strikes me as a potential gotcha if the
function is changed down the track to return more native PHP types for
numeric values.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/193/files#r1594144.

[dsp]

Please don't submit merges. Pull requests should only contain the actual changes. The PR should be for the lowest branch to which the changes apply (PHP-5.3 in this case).

So if you don't squash d0d978b and 383fcb3 together and don't include the merges (we will do the merging for you :)).

Anyway, thanks a lot for the patches. At a first glance they look okay and I am looking into merging them.

[brandonkirsch]

Hi David,
Thanks for trying to work with my screwed up submission. I've never
contributed anything before so I appreciate everyone's help and feedback.

Regarding the branch used for pull request, I was simply following the
directions in the PHP GITFAQ Wiki, here:
https://wiki.php.net/vcs/gitfaq#committing_and_merging

Should I adjust that Wiki section to tell patch submitters NOT to merge
upwards?

Thanks,
Brandon

On Mon, Sep 17, 2012 at 4:25 AM, David Soria Parra <notifications@github.com

wrote:

Please don't submit merges. Pull requests should only contain the actual
changes. The PR should be for the lowest branch to which the changes apply
(PHP-5.3 in this case).

So if you don't squash d0d978bhttps://github.com/php/php-src/commit/d0d978band
383fcb3 383fcb3 together and
don't include the merges (we will do the merging for you :)).

Anyway, thanks a lot for the patches. At a first glance they look okay and
I am looking into merging them.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/193#issuecomment-8607257.

[felipensp]

I've pushed your changes manually, thanks!

[php-pulls]

Comment on behalf of felipe at php.net:

Done.