Edit report at https://bugs.php.net/bug.php?id=80460&edit=1
ID: 80460
Comment by: kadler at us dot ibm dot com
Reported by: mirish at ibm dot com
Summary: ODBC doesn't account for SQL_NO_TOTAL indicator,
causing segmentation fault
Status: Closed
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:
Looks like we will need to open a new bug to fix this properly, since
the current fix, while definitely better than crashing, isn't all that
useful to our users since any time SQL_NO_TOTAL is encountered false is
returned. Before I do, I figured I would address some of the comments
here to provide a conistent thread:
> 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.
But relying on the output indicator as the source of truth for how much
data was returned (even when not SQL_NO_TOTAL) can cause buffer
overflow, since the indicator contains how much data *could be
returned*, not how much actually was. Without truncation, these numbers
would be the same, but if the data was truncated, the indicator will be
larger than the buffer size and you'll overflow the buffer. The only
way to know how much character data was actually returned in that case
is using strlen/wcslen.
> 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.
I am pretty sure that SQL_ATTR_OUTPUT_NTS was added for source
compatibility with Open Group's Call-Level Interface. The attribute is
there for compatibility applications that try to call it, but it is not
supported/implemented.
Note that CLI compatibility was added in ODBC 3.0, so that's why the
attribute wouldn't have existed in ODBC 2:
https://docs.microsoft.com/en-us/sql/odbc/reference/odbc-and-the-standard-cli
For a more clear understanding of how character data is treated in
ODBC, this doc is more enlightening:
https://docs.microsoft.com/en-us/sql/odbc/reference/develop-app/character-data-and-c-strings
"When character data is returned from the driver to the application,
*the driver must always null-terminate it*. This gives the application
the choice of whether to handle the data as a string or a character
array. If the application buffer is not large enough to return all of
the character data, the driver truncates it to the byte length of the
buffer less the number of bytes required by the null-termination
character, null-terminates the truncated data, and stores it in the
buffer." (emphasis mine)
> 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.
I haven't looked at that issue in super fine detail (and I'm not an
expert with Oracle by any means), but it appears it's dealing with
binary data from a BLOB column. Binary data not being character data,
it isn't and shouldn't be null-terminated. Of course, in the one trace
SQLGetData is being called with SQL_CHAR, so maybe it's being converted
to character data (as a hex string). If it is being converted to
character data, then the driver has a bug in not terminating it
properly, otherwise PHP shouldn't expect null-termination in this case.
And certainly there is some benefit and utility in being defensive
against bugs in drivers, but in that case PHP should reserve space to
null-terminate the buffer itself. Indeed, the doc I referenced above
even explicitly mentions this:
"Therefore, applications must always allocate extra space for the
null-termination character in buffers used to retrieve character data.
For example, a 51-byte buffer is needed to retrieve 50 characters of
data."
Finally, for properly handling SQL_NO_TOTAL with ODBC, this is probably
the best doc to use since it comes straight from Microsoft (though it
is about SQL Server):
https://docs.microsoft.com/en-us/sql/relational-databases/native-client/features/odbc-driver-behavior-change-when-handling-character-conversions
Previous Comments:
------------------------------------------------------------------------
[2021-04-27 15:13:50] git@php.net
Automatic comment on behalf of cmb69
Revision: https://github.com/php/php-src/commit/7f8397620048ee3aea51d62bf9324102fff5c284
Log: Fix #80460: ODBC doesn't account for SQL_NO_TOTAL indicator
------------------------------------------------------------------------
[2021-04-13 16:27:01] cmb@php.net
> 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>
------------------------------------------------------------------------
[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>
------------------------------------------------------------------------
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