Edit report at https://bugs.php.net/bug.php?id=80460&edit=1
ID: 80460
Updated by: cmb@php.net
Reported by: mirish at ibm dot com
Summary: ODBC doesn't account for SQL_NO_TOTAL indicator,
causing segmentation fault
Status: Analyzed
Type: Bug
Package: ODBC related
Operating System: RHEL 7.9
PHP Version: Irrelevant
Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
> I have tested several ODBC 3.0-compliant drivers and they don't
> seem to treat SQL_DESC_OCTET_LENGTH in a standardized way when the
> schema/table/field character encoding differs from the client
> character encoding.
The ODBC specification states[1]:
| The transfer octet length of a column is the maximum number of
| bytes returned to the application when data is transferred to its
| default C data type.
And elsewhere[2]:
| Similarly, the values for transfer octet length do not come from
| SQL_DESC_LENGTH. They come from the SQL_DESC_OCTET_LENGTH of a
| field of a descriptor for all character and binary types.
So this looks like a bug in some of these drivers. Of course,
being liberal on the PHP side isn't bad per se, but â¦
> The safest solution, even if it wastes a tiny amount of space in
> the buffer for a short amount of time, is to multiply by 4.
Would that amount of space be tiny? For instance, MySQL allows
VARCHAR up to 64 KiB. There may be multiple such columns in a
query, say eight, so we would need to overallocate 1.5 MiB. So I
think we would need to restrict this somehow.
PDO_ODBC basically does this by "going long". The threshold is
very low (256 bytes), but whenever the threshold is exceeded, it
doesn't bind further columns but rather reads these with
SQLGetData(). That might not be the best solution, but something
to consider for ext/odbc as well.
However, I don't think any of this should be changed in a stable
release, and I don't see the behavior as a bug in PHP, but rather
as something that might be improved. Thus, in my opinion PR 6809
should be merged as is to fix the segmentation faults reported in
this ticket. Feel free to open a new ticket about further
improvements.
[1] <https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/transfer-octet-length>
[2] <https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/column-size-decimal-digits-transfer-octet-length-and-display-size>
Previous Comments:
------------------------------------------------------------------------
[2021-03-30 14:51:43] cmb@php.net
Thank you for checking, and the further input!
Regarding returning the "good" part of the buffer in case of
SQL_NO_TOTAL: I don't think that relying on the driver having
properly NUL terminated the string is a good idea; if the string is
not NUL terminated, that would result in a buffer overflow. And,
yes, the ODBC 3.8 specification states:
| This attribute defaults to SQL_TRUE. A call to SQLSetEnvAttr to
| set it to SQL_TRUE returns SQL_SUCCESS. A call to SQLSetEnvAttr to
| set it to SQL_FALSE returns SQL_ERROR and SQLSTATE HYC00 (Optional
| feature not implemented).
But doesn't that hint that maybe an earlier version of the spec
allowed SQL_FALSE for that attribute? (Otherwise having this
attribute would be completely useless.) Besides that this is an
ODBC 3 attribute; I have no idea how that was handled by ODBC 2.
And this is all about the spec; what about the practise (and it is
known that ODBC drivers have all kinds of quirks)? Bug #80783 has
an ODBC backtrace where SQLGetData() which requests 256 bytes,
has those 256 bytes in the buffer without any trailing NUL.
IMHO, far too risky for any stable branch, and maybe even too
risky to be introduced without explicit opt-in from the user for
the "master" branch.
Regarding charextraalloc: while I regard this generally as hack,
it might make sense to always overallocate character and binary
buffers. OTOH, I wonder whether SQL_DESC_OCTET_LENGTH is even the
proper column attribute to check for, since[1]:
| For variable-length character or binary types, this is the
| maximum length in bytes.
What maximum length?
I think I need to investigate closer.
Regarding the tests: you may need to temporarily customize
ext/odbc/tests/config.inc. Apparently, the environment variables
ODBCINI and ODBCSYSINI are hard-coded there (although I don't see
why this is).
[1] <https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlcolattribute-function>
------------------------------------------------------------------------
[2021-03-26 17:37:38] mirish at ibm dot com
I have tested the patch and it does work (no more segfaults!), but I have a few thoughts:
1.
I'm not sure just throwing an error is the best solution. If the driver returns SQL_NO_TOTAL,
there is still good data in the buffer, it just isn't ALL of the data (and the driver
doesn't want to calculate how much is left, probably because it would have to translate to a
different character encoding). The string data in the buffer should be null-terminated[1], so using
ZVAL_STRING instead of ZVAL_STRINGL should retrieve all of the good data from the buffer. Its
possible this would be a good change for ALL ZVAL_STRINGLs in the code, but to minimize the amount
of regressions added to the code I think it would be fine adding to the if-else blocks you've
already created. I think users might still want to know that not all data was returned, so keeping
the warning is probably a good idea:
...
} else if (result->values[i].vallen == SQL_NO_TOTAL) {
ZVAL_STRING(&tmp, buf);
php_error_docref(NULL, E_WARNING, "Cannot get all data of column #%d (driver
cannot determine length)", i + 1, rc);
} else {
...
2.
Although the case of SQL_NO_TOTAL should still be handled, I think most of it can be avoided if the
variable charextraalloc is always set to true, multiplying whatever is returned from
SQL_DESC_OCTET_LENGTH by 4. Currently it only sets this variable to true in a few cases, but it
should probably be broadened.
I have tested several ODBC 3.0-compliant drivers and they don't seem to treat
SQL_DESC_OCTET_LENGTH in a standardized way when the schema/table/field character encoding differs
from the client character encoding. For instance, a field with a character encoding of Windows-1251
could have a CHAR(1) field that holds the â character in a single byte (0x86). If the client
wants data returned in UTF-8, this expands to become 0xE2 0x80 0xA0, requiring 3 bytes. Like I said,
drivers seem to handle this case differently: FreeTDS and the IBM i Access driver would return 1,
while the MySQL driver actually checks to see the maximum UTF-8 expansion and returns 3. The safest
solution, even if it wastes a tiny amount of space in the buffer for a short amount of time, is to
multiply by 4.
Always multiplying by 4 SHOULD ensure enough buffer space so that all of the data is returned and so
SQL_NO_TOTAL isn't encountered. In my test case, when I set charextraalloc to always be true,
my queries return data perfectly.
3.
I am not having much luck running the ODBC tests, I pass in the environment variables (or even
export them) and try to run it, but 20 of the 21 tests are skipped as "could not connect".
Will continue to try. I don't think there are any tests that check for SQL_NO_TOTAL, and since
it happens in a very particular circumstance it would be hard to force, but I can write some tests
that at least change the StrLen_or_IndPtr value after it is returned to force the code to take the
SQL_NO_TOTAL path.
[1]: There is an option in SQLSetEnvAttr called SQL_ATTR_OUTPUT_NTS that controls whether character
data returned is null-terminated. It looks like you CAN set it to SQL_FALSE, but the docs note:
"A call to SQLSetEnvAttr to set it to SQL_FALSE returns SQL_ERROR and SQLSTATE HYC00 (Optional
feature not implemented)." Not sure if that is MSSQL-specific, but I have never known any one
to try to use this feature.
------------------------------------------------------------------------
[2021-03-26 14:25:59] cmb@php.net
@mirish, could you please test the provided patch[1]? It would
also be great if you could run the test suite before and after
applying the patch, to see whether we already have tests covering
this scenario. If you build from source, you can run the test
suite by running
make test TESTS=ext/odbc/tests
You need to set the environment variables ODBC_TEST_DSN,
ODBC_TEST_USER and ODBC_TEST_PASS for that to work.
If the existing tests do not (sufficently) cover this scenario,
it would be great if you could provide (an) additional test(s).
[1] <https://github.com/php/php-src/pull/6809>
------------------------------------------------------------------------
[2021-03-26 14:20:29] cmb@php.net
The following pull request has been associated:
Patch Name: Fix #80460: ODBC doesn't account for SQL_NO_TOTAL indicator
On GitHub: https://github.com/php/php-src/pull/6809
Patch: https://github.com/php/php-src/pull/6809.patch
------------------------------------------------------------------------
[2021-03-26 10:16:36] cmb@php.net
Thanks for the clarification! Indeed, these SQL_NO_TOTAL checks
are necessary.
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=80460
--
Edit this bug report at https://bugs.php.net/bug.php?id=80460&edit=1