php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #80460 ODBC doesn't account for SQL_NO_TOTAL indicator, causing segmentation fault
Submitted: 2020-12-02 02:46 UTC Modified: 2021-04-13 16:27 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:0 (0.0%)
From: mirish at ibm dot com Assigned: cmb (profile)
Status: Closed Package: ODBC related
PHP Version: Irrelevant OS: RHEL 7.9
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: mirish at ibm dot com
New email:
PHP Version: OS:

 

 [2020-12-02 02:46 UTC] mirish at ibm dot com
Description:
------------
The ODBC extension (ext/odbc/php_odbc.c) causes a segmentation fault when SQL_NO_TOTAL indicator is returned to a bound column from a call to SQLFetch and similar functions.

There are two major issues:

1. There is an assumption that the number of bytes returned from a call to SQLColAttribute with a type of SQL_DESC_OCTET_LENGTH will return the number of bytes required to store the value. There are cases when data is stored in an encoding which stores character data in a single byte, which then needs to be converted to variable multi-byte when SQLFetch is actually called (eg. EBCDIC 278 to UTF8 conversion). This might be a broken driver implementation on our part, but the ODBC docs are rather thin and at times seem contradictory on this point.

This will cause a buffer to be allocated of size X, where up to X * 4 may be needed to store a string. There is a variable in the code (charextraalloc) to allocate buffers of this size, but none of the checks work for ODBC v 3.0+ drivers. When the buffer is too small, the driver may return SQL_NO_TOTAL when in the StrLen_or_IndPtr, as it didn't have enough space for the column, and might not know how many bytes are left.

2. The bigger issue is that calls to SQLFetch can return SQL_NO_TOTAL to the StrLen_or_IndPtr stored on SQLBindCol when the buffer bound isn't large enough for the data, but the code isn't checking for this. Instead, the code checks if the value of StrLen_or_IndPtr is SQL_NULL_DATA and assigns a value to null. If it isn't SQL_NULL_DATA, it assumes that it holds the length of the string, then calls functions like ZVAL_STRINGL and RETURN_STRINGL with SQL_NO_TOTAL (which evaluates to -4) as the length. Trying to create a string with length of -4 causes a segmentation fault, which causes the program to crash.

As mentioned, the first issue might be a problem in driver implementation, but the second definitely needs to be fixed. SQL_NO_TOTAL is definitely a valid StrLen_or_IndPtr value, and there are no checks for it in the code, causing a segfault.


Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-12-02 10:55 UTC] cmb@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: cmb
 [2020-12-02 10:55 UTC] cmb@php.net
Isn't the second part basically a duplicate of bug #44618, which
is supposed to be fixed as of PHP 7.3.25 and 7.4.13?
 [2020-12-02 17:07 UTC] mirish at ibm dot com
-Status: Feedback +Status: Assigned
 [2020-12-02 17:07 UTC] mirish at ibm dot com
It doesn't look like they are related. Bug #44618 seems to note that it should handle SQL_NO_DATA, but that is a cursor indicator different from SQL_NO_TOTAL, which indicates that there is data left to retrieve, but the driver doesn't know (or doesn't want to calculate) how much data is left. As written, SQL_NO_TOTAL indicator (-4) is passed as a length to string functions.

I can confirm that this segmentation fault occurs on 7.4.13.
 [2020-12-04 15:57 UTC] cmb@php.net
-Status: Assigned +Status: Feedback
 [2020-12-04 15:57 UTC] cmb@php.net
While the other bug report was indeed about SQL_NO_DATA, the fix
is supposed to cater to any result other than SQL_SUCCESS or
SQL_SUCCESS_WITH_INFO, and to return early in those cases.

Anyway, could you please generate a stack backtrace[1] and post it
here (or provide it somewhere else if it is very large)?

[1] <https://bugs.php.net/bugs-generating-backtrace.php>
 [2020-12-13 04:22 UTC] php-bugs at lists dot php dot net
No feedback was provided. The bug is being suspended because
we assume that you are no longer experiencing the problem.
If this is not the case and you are able to provide the
information that was requested earlier, please do so and
change the status of the bug back to "Re-Opened". Thank you.
 [2021-02-08 13:12 UTC] ml at menten dot com
Hello,

according to IBM support we have a related problem reading special characters from VARGRAPHIC fields in PHP 7.3.26 with the following driver: 

IBM i Access Client Solutions - PASE Application Package for IBM i.
Connectivity to Db2® for i using ODBC with unixODBC
ibm-iaccess.ppc64 1.1.0.14-0 @/ibm-iaccess-1.1.0.14-0.ibmi7.2.ppc64

If we write to a VARGRAPHIC field of length 70, for example, we can write the full 70 characters, but we can only read them if the 70 characters do not consist of special characters or if there are at most 35 special characters (e.g. ä).
For example, if we have i special characters and 69 non-special characters in the field, it cannot be read out either. Analogously, this problem also occurs with CHAR and VARCHAR fields. 

Test Script:


<?php
ini_set('display_errors', true);
header('Content-Type: text/html; charset=utf-8');
ini_set("default_mimetype", "text/html");
ini_set("default_charset", "UTF-8");




// setup
$user = "test";
$pw = "test";


// full 70 length / no special characters -> OK
#	$test = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";

// 35 length / all special characters -> OK
#	$test = "äääääääääääääääääääääääääääääääääää";

// 36 length / 35 special characters, 1 normal character -> no result
$test = "äääääääääääääääääääääääääääääääääää1";

// 52 length / 35 normal characters, 17 special character -> OK
#	$test = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaäääääääääääääääää";

// 53 length / 35 normal characters, 18 special character -> no result
#	$test = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaääääääääääääääääää";

// full 70 length / 70 special characters -> no result
#	$test = "ääääääääääääääääääääääääääääääääääääääääääääääääääääääääääääääääääääää";



try
{
	$connection = new PDO("odbc:DRIVER={IBM i Access ODBC Driver};SYSTEM=localhost;DATABASE=*LOCAL;NAM=1;DBQ=IEFFECTDB,IEFFECT28;CCSID=1208;DEBUG=524288", $user, $pw, array(PDO::ATTR_ERRMODE => PDO::ERRMODE_SILENT));
}
catch (Exception $e)
{
	echo($e->getMessage());
	echo "<br>";
	echo($e->getCode());
	
	die();
}


// create table
$query = "CREATE TABLE IEFFECTDB/TEST (TEST1 VARGRAPHIC(70) ccsid 1200 NOT null DEFAULT '')";
$params = array();
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);


// insert
$query = "INSERT INTO IEFFECTDB/TEST (TEST1) VALUES (?)";
$params = array($test);
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);


// select
$query = "SELECT TEST1 FROM IEFFECTDB/TEST";
$params = array();
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);
echo "<pre>";
var_dump($stmt->fetch(PDO::FETCH_ASSOC));
echo "<pre>";


// drop table
$query = "DROP TABLE IEFFECTDB/TEST";
$params = array();
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);


$connection = null;

echo "------------------------------------------------------------------------------------------------------------";
echo "<br><br>";


try
{
	$connection = new PDO("odbc:DRIVER={IBM i Access ODBC Driver};SYSTEM=localhost;DATABASE=*LOCAL;NAM=1;DBQ=IEFFECTDB,IEFFECT28;CCSID=1208", $user, $pw, array(PDO::ATTR_ERRMODE => PDO::ERRMODE_SILENT));
}
catch (Exception $e)
{
	echo($e->getMessage());
	echo "<br>";
	echo($e->getCode());
	
	die();
}


// create table
$query = "CREATE TABLE IEFFECTDB/TEST (TEST1 VARGRAPHIC(70) ccsid 1200 NOT null DEFAULT '')";
$params = array();
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);


// insert
$query = "INSERT INTO IEFFECTDB/TEST (TEST1) VALUES (?)";
$params = array($test);
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);


// select
$query = "SELECT TEST1 FROM IEFFECTDB/TEST";
$params = array();
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);
echo "<pre>";
var_dump($stmt->fetch(PDO::FETCH_ASSOC));
echo "<pre>";


// drop table
$query = "DROP TABLE IEFFECTDB/TEST";
$params = array();
$stmt = $connection->prepare($query);
$result = $stmt->execute($params);
 [2021-02-08 13:30 UTC] cmb@php.net
> according to IBM support we have a related problem […]

This ticket is about a segmentation fault in the ODBC extension;
your issue is about truncation of strings in the PDO_ODBC
extension.  Please report that as separate ticket.
 [2021-03-25 17:44 UTC] mirish at ibm dot com
cmb@php.net noted:

"While the other bug report was indeed about SQL_NO_DATA, the fix
is supposed to cater to any result other than SQL_SUCCESS or
SQL_SUCCESS_WITH_INFO, and to return early in those cases."

I think I see the confusion now. When SQLGetData returns SQL_NO_TOTAL, it is doing so in the StrLen_or_IndPtr parameter, not as the return code: https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgetdata-function?view=sql-server-ver15

The issue you linked does indeed check the return code, but the issue here is that the valid values returned from the StrLen_or_IndPtr are:

* The length of the data available to return
* SQL_NO_TOTAL
* SQL_NULL_DATA

The current code checks for SQL_NULL_DATA, and assigns a value of null if so. Otherwise, it just assumes the value stored there is the length in bytes returned and tries to allocate a buffer of that size. But that isn't the case when it returns SQL_NO_TOTAL (-4), which will try to allocated a buffer of -4 and cause a segfault. There needs to be another check for:

result->values[i].vallen == SQL_NO_TOTAL

Similar to how it checks for SQL_NO_DATA.

I will work to generate a backtrace, might take a moment of relearning how to build PHP on my system.
 [2021-03-25 17:46 UTC] mirish at ibm dot com
Last comment should read:

result->values[i].vallen == SQL_NO_TOTAL

Similar to how it checks for SQL_NULL_DATA.
 [2021-03-25 20:04 UTC] mirish at ibm dot com
Ok, a little clarification: It looks like my particular error is generated from the StrLen_or_IndPtr bound from SQLBindCol: https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlbindcol-function?view=sql-server-ver15

The valid values are still:
* The length of the data available to return
* SQL_NO_TOTAL
* SQL_NULL_DATA

Then, when odbc_fetch_row is called, SQLExtendedFetch gets called (in my particular test, but SQLFetch and SQLGetData can equally return the same thing), populates that indicator bound in SQLBindCol, and SQL_NO_TOTAL is returned to that StrLen_or_IndPtr, which is mapped in the php_odbc.c code to result->values[field_ind].vallen.

Then, the code tries to generate a string from that length (-4), causing the seg fault.

Backtrace:

#0  0x00007f0d54ed9474 in __memcpy_ssse3_back () from /usr/lib64/libc.so.6
#1  0x0000000000591919 in zend_string_init (str=0x7f0d54491028 "", len=18446744073709551612, persistent=0)
    at /home/mirish/php-src/Zend/zend_string.h:157
#2  0x0000000000596dc1 in zif_odbc_result (execute_data=0x7f0d54414150, return_value=0x7f0d54414120)
    at /home/mirish/php-src/ext/odbc/php_odbc.c:2224
#3  0x000000000085cbb1 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER () at /home/mirish/php-src/Zend/zend_vm_execute.h:1313
#4  0x00000000008bc92c in execute_ex (ex=0x7f0d54414020) at /home/mirish/php-src/Zend/zend_vm_execute.h:53564
#5  0x00000000008c09fa in zend_execute (op_array=0x7f0d5447e400, return_value=0x0)
    at /home/mirish/php-src/Zend/zend_vm_execute.h:57664
#6  0x00000000007ef9fc in zend_execute_scripts (type=8, retval=0x0, file_count=3)
    at /home/mirish/php-src/Zend/zend.c:1663
#7  0x000000000075bba0 in php_execute_script (primary_file=0x7ffd3adb2620) at /home/mirish/php-src/main/main.c:2619
#8  0x00000000008c31c6 in do_cli (argc=2, argv=0x2c11700) at /home/mirish/php-src/sapi/cli/php_cli.c:961
#9  0x00000000008c4105 in main (argc=2, argv=0x2c11700) at /home/mirish/php-src/sapi/cli/php_cli.c:1352


And for good measure, here is the ODBC trace:

...
[ODBC][88259][1616702310.287337][SQLExecDirect.c][240]
		Entry:
			Statement = 0x1f66a90
			SQL = [SELECT * FROM MIRISH.UTF8TEST][length = 29 (SQL_NTS)]
[ODBC][88259][1616702310.468968][SQLExecDirect.c][521]
		Exit:[SQL_SUCCESS]
[ODBC][88259][1616702310.469037][SQLNumResultCols.c][156]
		Entry:
			Statement = 0x1f66a90
			Column Count = 0x7effad458730
[ODBC][88259][1616702310.469087][SQLNumResultCols.c][251]
		Exit:[SQL_SUCCESS]
			Count = 0x7effad458730 -> 1
[ODBC][88259][1616702310.469128][SQLColAttribute.c][294]
		Entry:
			Statement = 0x1f66a90
			Column Number = 1
			Field Identifier = SQL_DESC_NAME
			Character Attr = 0x7effad45c140
			Buffer Length = 256
			String Length = 0x7fff4dbce140
			Numeric Attribute = (nil)
[ODBC][88259][1616702310.469188][SQLColAttribute.c][709]
		Exit:[SQL_SUCCESS]
[ODBC][88259][1616702310.469217][SQLColAttribute.c][294]
		Entry:
			Statement = 0x1f66a90
			Column Number = 1
			Field Identifier = SQL_DESC_CONCISE_TYPE
			Character Attr = (nil)
			Buffer Length = 0
			String Length = (nil)
			Numeric Attribute = 0x7effad45c250
[ODBC][88259][1616702310.469252][SQLColAttribute.c][709]
		Exit:[SQL_SUCCESS]
[ODBC][88259][1616702310.469280][SQLColAttribute.c][294]
		Entry:
			Statement = 0x1f66a90
			Column Number = 1
			Field Identifier = SQL_DESC_OCTET_LENGTH
			Character Attr = (nil)
			Buffer Length = 0
			String Length = (nil)
			Numeric Attribute = 0x7fff4dbce138
[ODBC][88259][1616702310.469309][SQLColAttribute.c][709]
		Exit:[SQL_SUCCESS]
[ODBC][88259][1616702310.469338][SQLBindCol.c][236]
		Entry:
			Statement = 0x1f66a90
			Column Number = 1
			Target Type = 1 SQL_CHAR
			Target Value = 0x7effad491028
			Buffer Length = 2
			StrLen Or Ind = 0x7effad45c248
[ODBC][88259][1616702310.469382][SQLBindCol.c][344]
		Exit:[SQL_SUCCESS]
[ODBC][88259][1616702310.469452][SQLExtendedFetch.c][166]
		Entry:
			Statement = 0x1f66a90
			Fetch Type = 1
			Row = 1
			PcRow = 0x7fff4dbce1a0
			Row Status = 0x7fff4dbce190
[ODBC][88259][1616702310.560382][SQLExtendedFetch.c][339]
		Exit:[SQL_SUCCESS]
 [2021-03-25 21:16 UTC] cmb@php.net
-Status: No Feedback +Status: Assigned
 [2021-03-26 10:16 UTC] cmb@php.net
-Status: Assigned +Status: Analyzed
 [2021-03-26 10:16 UTC] cmb@php.net
Thanks for the clarification!  Indeed, these SQL_NO_TOTAL checks
are necessary.
 [2021-03-26 14:20 UTC] 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 14:25 UTC] 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 17:37 UTC] 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-30 14:51 UTC] 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-04-13 16:27 UTC] 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-04-27 15:13 UTC] 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-27 15:13 UTC] git@php.net
-Status: Analyzed +Status: Closed
 [2021-05-26 17:55 UTC] kadler at us dot ibm dot com
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
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Mar 28 14:01:29 2024 UTC