Fix pdo_odbc output-buffer leak and stale-binding out-of-bounds read#22735
Fix pdo_odbc output-buffer leak and stale-binding out-of-bounds read#22735iliaal wants to merge 1 commit into
Conversation
SQLBindParameter recorded deferred pointers (P->outbuf, &P->len, and the bound-parameter struct) into the ODBC statement at PDO_PARAM_EVT_ALLOC, and P->outbuf was never freed. Both outlive the bound-parameter struct, which PDO destroys mid-statement on execute() with an array or a rebind: the buffer leaked and the stale binding made the next SQLExecute read freed memory. Track output buffers on the statement and free them in the destructor, and defer binding to execute time, resetting and rebinding the current parameters when the set changed. Closes phpGH-22735
fa0df82 to
5406151
Compare
NattyNarwhal
left a comment
There was a problem hiding this comment.
Initial pass, looks good other than a few nitpicky things related to clarity.
| P->outbuflen = P->len; | ||
| if (!S->out_buffers) { | ||
| ALLOC_HASHTABLE(S->out_buffers); | ||
| zend_hash_init(S->out_buffers, 8, NULL, odbc_free_out_buffer, 0); |
There was a problem hiding this comment.
If 8 is supposed to be pointer size, it should be using sizeof to make that clear
There was a problem hiding this comment.
That's zend_hash_init's nSize (initial table size), not a pointer size, so sizeof doesn't apply.
There was a problem hiding this comment.
My misread there. That said, I'm still not a huge fan of the magic number here, if it's a guess on the typical high end of param count, we should be clear here; a lot of instances of zend_hash_init are not. (Oh, and persistent param at end should also probably be false since it's a bool param, but that's not consistent through most of the codebase... opportunity for cleanup?)
There was a problem hiding this comment.
Changed the persistent flag to false and replaced the 8 with HT_MIN_SIZE to make the minimum-table intent explicit.
SQLBindParameter recorded deferred pointers (P->outbuf, &P->len, and the bound-parameter struct) into the ODBC statement at PDO_PARAM_EVT_ALLOC, and P->outbuf was never freed. Both outlive the bound-parameter struct, which PDO destroys mid-statement on execute() with an array or a rebind: the buffer leaked and the stale binding made the next SQLExecute read freed memory. Track output buffers on the statement and free them in the destructor, and defer binding to execute time, resetting and rebinding the current parameters when the set changed. Closes phpGH-22735
5406151 to
f804cbe
Compare
SQLBindParameter recorded deferred pointers (P->outbuf, &P->len, and the bound-parameter struct) into the ODBC statement at PDO_PARAM_EVT_ALLOC, and P->outbuf was never freed. Both outlive the bound-parameter struct, which PDO destroys mid-statement on execute() with an array or a rebind: the buffer leaked and the stale binding made the next SQLExecute read freed memory. Track output buffers on the statement and free them in the destructor, and defer binding to execute time, resetting and rebinding the current parameters when the set changed. Closes phpGH-22735
f804cbe to
3a1a7d4
Compare
pdo_odbc bound each parameter with
SQLBindParameteratPDO_PARAM_EVT_ALLOC, recording deferred pointers (P->outbuffor output,&P->len, and the bound-parameter struct itself for input) into the ODBC statement, and never freedP->outbuf. Both outlive the bound-parameter struct, which PDO destroys mid-statement whenexecute()is given an array or a position is rebound, so the output buffer leaked and the stale binding made the nextSQLExecuteread freed memory. This tracks the output buffers on the statement and frees them in the destructor afterSQLFreeHandle, and defersSQLBindParameterto execute time, rebinding the current parameters when the parameter set has changed.Reproducers, both under a debug or ASAN build: