-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix #44618: Fetching may rely on uninitialized data #6281
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
Unless `SQLGetData()` returns `SQL_SUCCESS` or `SQL_SUCCESS_WITH_INFO`, the `StrLen_or_IndPtr` output argument is not guaranteed to be properly set. Thus we handle retrieval failure other than `SQL_ERROR` by silently yielding `false` for those column values.
What is the actual return value we end up handling here? |
In this case, it is |
Independently of the problem you're trying to fix here ... why are we getting SQL_NO_DATA? The PHP code looks to be doing something sensible, why do we fail to fetch the text1 column? |
|
This connection requests use of the (deprecated) ODBC cursor library, which implements |
|
Any objections to merge this? |
|
My 2c here is that we should treat these cases as close to errors as we can. I understand we can't use odbc_sql_error(), so we should explicitly throw a warning and return false. Otherwise we will return corrupted data, and I doubt that anyone who uses these APIs will have appropriate checks in place to recognize |
|
Good point. Thanks! Maybe the error message can be improved (cc @kocsismate). I've chosen the column numbers to start at 1, because that is the convention for |
nikic
left a comment
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.
Looks reasonable to me!
Unless
SQLGetData()returnsSQL_SUCCESSorSQL_SUCCESS_WITH_INFO,the
StrLen_or_IndPtroutput argument is not guaranteed to be properlyset. Thus we handle retrieval failure other than
SQL_ERRORbysilently yielding
falsefor those column values.Alternative handling of retrieval failure would be to raise
E_WARNINGas well, and/or let the wholeodbc_fetch_into()/odbc_fetch_array()fail.I don't really mind how exactly we solve this, but that bug needs to be fixed.