fix(webserver): php_core_lib various fixes - #150
Conversation
…ative_gettext, php_native_gettext_noop, and php_native_ngettext functions php_core_lib
…unction of php_core_lib
…pFilter function of php_core_lib
RealGreenDragon
changed the title
got3nks
left a comment
got3nks
left a comment
There was a problem hiding this comment.
Thanks. Verified the four fixes:
PRId64matches PHP's signed-integer semantics — fine. Worth noting though:int_valis declared asuint64_tinphp_syntree.h:67, and there's a secondPRIu64print of the same field atphp_syntree.cpp:965(thecast_value_strpath) that this PR doesn't touch. After this lands, the codebase will display the field as signed in one place and unsigned in the other. Easiest tidy-up: flip that line toPRId64too (same justification asphp_var_dump), either in this PR or a small follow-up. The "right" long-term fix is to changeint_valtoint64_tin the struct so the storage type matches the semantics, but that's a wider change.php_native_substrremoval — confirmed unused (grep -rn "php_native_substr\|\"substr\"" src/webserverreturns only the definition itself).- NULL-before-deref fixes in the four functions are textbook UB fixes.
while ( *scan_ptr )overstrlen(scan_ptr)— net behaviour identical, O(N) → O(1) per iteration.
On the three doubts:
-
php_native_issetcast_value_strcalls without checkingstr_val— leave it. Thecast_value_strcalls normalise the value type; the function's intent is to mimic PHP'sisset()which checks whether the variable is set + non-null. The current behaviour matches enough of that semantics; removing the calls risks subtle behaviour changes on edge cases (PHP_VAL_NONE, PHP_VAL_INT_DATA). I'd skip this one in this PR. -
php_native_htmlspecialcharscast_value_dnumon result — dead but harmless. The function later overwritesresultwith a string. Removing thecast_value_dnumis safe; keeping it is wasted cycles. Author's call — fine either way. Not in this PR's scope. -
CWriteStrBuffer::Writeoff-by-one — real bug.strncpy(dst, src, n)does NOT null-terminate whenstrlen(src) >= n. The(len + 1) <= m_curr_buf_leftcheck reserves a byte for the terminator, but nothing actually writes it. Two fixes either is acceptable:- Append
dst[len] = '\0';afterstrncpy. - Switch to
memcpy(dst, src, len); dst[len] = '\0';(clearer intent, no strncpy padding behaviour).
Worth doing as a small follow-up rather than dragging it into this PR — different file, different concern.
- Append
Approving as-is — the line 965 PRIu64 is the only thing worth picking up in this PR if you want symmetry, otherwise a follow-up is fine.
fix(webserver): PR #150 follow-up
2 participants
Fixes:
php_native_substras it contains several bugs and above all is never usedphp_native_substr,php_native_gettext,php_native_gettext_noop, andphp_native_ngettextPRIu64withPRId64inphp_var_dump(PHP_VAL_INTcan hold a signed integer)while ( strlen(scan_ptr) )withwhile ( *scan_ptr )inCPhpFilter::CPhpFilter, making the check O(1) instead of O(N)Doubts and suggestions:
php_native_issetcallscast_value_stron params without checkingparam->str_val(so it seems to do nothing useful); the only apparent reason is to triggerassert(0)whentype=PHP_VAL_INT_DATA, but I don't see the advantage → two options: remove params and thecast_value_strcall entirely, or also check whetherparam->str_valis not an empty string (but this seems unnecessary as the type check alone should be sufficient)php_native_htmlspecialcharscallscast_value_dnumon result without an apparent reason (result->int_valis never used); the only apparent reason seems to be triggeringassert(0)whentype=PHP_VAL_INT_DATA, but I don't see the advantage → remove thecast_value_dnumcallCWriteStrBuffer::Write: inif ( (len + 1) <= m_curr_buf_left ),lenis incremented by 1 to account for the string terminator, butstrncpydoes not actually add it → two options: explicitly add the null terminator (it is unclear whether the current behavior is intentional), or replace(len + 1)withlenin the condition (also replacingstrncpywithmemcpyto make the intent clearer)If you can help clarify these doubts, I will apply the suggested changes in this PR.