Fix GH-22667: pdo_odbc heap over-read on oversized column value#22672
Fix GH-22667: pdo_odbc heap over-read on oversized column value#22672iliaal wants to merge 1 commit into
Conversation
NattyNarwhal
left a comment
NattyNarwhal
left a comment
There was a problem hiding this comment.
Haven't got breakfast yet so may have missed something, but initial review seems mostly fine. Just a few questions.
| <?php | ||
| $file = tempnam(sys_get_temp_dir(), "gh22667"); | ||
| try { | ||
| $pdo = new PDO("odbc:Driver=SQLite3;Database=$file"); |
There was a problem hiding this comment.
Does this repro with the SQL Server driver, since that's what most of our tests are in? If not, we should install the SQLite ODBC driver in CI, and also see if the ODBC driver has an equivalent to :memory:.
There was a problem hiding this comment.
Switched to :memory: (the SQLite ODBC driver supports it). I've no SQL Server to confirm there, but it's the standard ODBC truncation contract (SQL_SUCCESS_WITH_INFO reports the full fetched_len while writing only buffer-worth), so it's driver-agnostic. Can add the driver to CI as a follow-up.
There was a problem hiding this comment.
You should be able to stand up SQL Server from the Docker container (see .github/actions/setup-mssql/action.yml), let me know if you need any help with that.
There was a problem hiding this comment.
Stood up the 2022 container and checked: it doesn't reproduce there. SQL Server's driver reports the declared column size (>= the value), or INT_MAX for LOB types which take the long/streaming path, so pdo_odbc never binds the short buffer against an over-long value. SQLite3 is the vehicle precisely because its loose typing lets a computed column describe as 255 while returning 4096, which is the SQL_SUCCESS_WITH_INFO truncation the clamp guards. So the test can't move to SQL Server; if you'd rather it run than skip, I'll add the SQLite ODBC driver to CI.
There was a problem hiding this comment.
Yeah, it's probably a SQLite quirk that we'd have to work around (and who knows what other drivers do similar). If so, then the driver should be added to CI so the test actually gets run. That should probably be done in a separate PR. (There's also some possibilities to do more ODBC tests in CI with platforms that don't support the SQL Server driver too (i.e. Alpine) by adding/converting tests and avoid overfitting to SQL Server, but that's a different discussion.)
There was a problem hiding this comment.
Ok, let me see about getting that going, SQLite has a + of being nice & light to makes for quick tests.
odbc_stmt_describe() binds a colsize+1 buffer for short columns and stores that capacity in datalen, but odbc_stmt_get_col() built the result string from the driver-reported fetched_len. A conforming driver truncates an over-long value into the buffer yet reports the full length, so ZVAL_STRINGL_FAST over-read past the allocation and returned adjacent heap to userland. Clamp fetched_len to the bound capacity, guarded by !is_long so the long-column SQLGetData path (whose fixed buffer is unrelated to datalen) is left untouched. Fixes phpGH-22667
iliaal
force-pushed
the
fix/gh-22667-pdo-odbc-clamp
branch
from
aae797d to
119dc33
Compare
2 participants
ext/pdo_odbc builds the result string from the driver-reported fetched_len, which on truncation exceeds the bound colsize+1 buffer (SQL_SUCCESS_WITH_INFO), over-reading heap into the returned value. Clamps to the bound capacity for short-bound columns only, so long columns are unaffected. Reproduces with the SQLite3 ODBC driver; the test skips without it. Sibling of GH-22668.
Fixes #22667