-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Segmentation fault on pdo_dblib::nextRowset (bug #69757) #1386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Could you also add a test for this issue? |
Maybe #1404 helps testing this patch. |
ext/pdo_dblib/dblib_stmt.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can dbcolname also return NULL? Maybe should be if(fname && *fname) ?
any news about pdo_dblib ? |
As far as I remember, pdo_dblib doesn't have an active maintainer, so yeah it is hard to say. @weltling what do you think about cherry picking this for RC6 as it have been standing here for a fair amount of time with a test etc or do you prefer to postpone it for 7.0.1? |
ext/pdo_dblib/dblib_stmt.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should ensure col->name
is \0 terminated, strlen() is used afterwards. Or better, snprintf() returns the number of bytes written, that can also help to spare the strlen() call. Please consult man.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weltling snprintf automatically adds the NUL termination. However it would be better to just use spprintf instead (no need to estrdup afterwards).
col->namelen = spprintf(&col->name, NULL, "computed%d", colno);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic yeah, overseen that the len was firm. Your variant is handier anyway.
@KalleZ as it's a crash fix, probably fine for RC6. But it first should go into 5.6 (originally was even targeting 5.5). I'll not be able to test it anytime soon, anyway. @MiRacLe-RPZ are you an active pdo_dblib ext user? Maybe you could go for maintaining it? Thanks. |
@weltling i'm active mssql ext user, but this extension removed from php7 source tree, now i try to migrate my codebase on pdo_dblib. At this moment pdo_dblib unusable for me - crashed on simple scenarios, missing some features available in ext-mssql, and other pdo-related extensions, etc, but this is "our future" :). About maintaining - i can't write good C-code, but i can test, rate from the user point of view other patches, to improve them as opportunities. |
@MiRacLe-RPZ well, whether it's the future ... pdo_dblib was suggested for removals just like mssql for PHP7. Now it is where it is :) But as you mention mssql - ext/odbc is used by most people i saw, maybe that were something for you to think about. With this patch - please address the @nikic idea, then we actually can go apply your patch based on your confirmation. Thanks. |
Patch from @nikic was applyed, thanks for review. @weltling, with ext/odbc (ext/pdo_odbc) i need setup environment(DSN-config, odbc.ini) before connect, this is hard for me (heterogeneous environment, dynamic addresses, etc), and impossible for most of my customers, dblib(freetds)/sqlsrv(ms sqlserver native client) not required system-wide configuration. Many other technical reasons not to use odbc in my projects, but comparative ease of installation played a decisive role in the selection of the driver work with databases many years ago...with ODBC I have to change not only the way to connect to the database, but all client code (sql queries). If it becomes necessary, it will be easier to drop support for non-windows system (can only use ext/sqlsrv) or even write a new product on a different platform. |
Comment on behalf of ab at php.net: Merged into 5.6 and up. Please check the current 7.0 as well. Thanks! |
I'm still experiencing this issue in PHP 5.6.23 on Centos 7 using FreeTDS v0.95.81 I have created a new bug: https://bugs.php.net/bug.php?id=72488 |
Workaround for handling "unnamed" fields in dblib's resultset. Code was copied from ext/mssql/php_mssql.c.