Fix integer overflow and NULL handle in invoke() - #167
Closed
SebZar wants to merge 16 commits into
Closed
Conversation
RfcCreateFunction returns NULL on failure; the error info is communicated via the error_info out-parameter, not via the RFC_RC return value. The previous check tested `rc`, which was still set to RFC_OK from its initialiser and therefore never triggered. A NULL function_handle would silently propagate through the rest of invoke(), causing undefined behaviour when the RFC SDK operated on it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…BLE case When RfcGetTable() failed, the error branch freed parameter_name_u and set value to NULL but did not return. Execution fell through to the call below, which passed the freed pointer and an uninitialised table_handle to rfc_get_table_value(). If the RFC SDK accepted the garbage handle without error, RfcGetRowCount() could return an arbitrarily large row count. The subsequent loop then tried to populate a PHP array with that many entries, triggering PHP's safe_emalloc() overflow guard: "Possible integer overflow in memory allocation (N * 32 + 32)" The equivalent code in rfc_get_field_value() already returned early in the same error case; this commit brings rfc_get_parameter_value() in line with that pattern. Fixes: gkralik#163 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sapuc_to_zval_len_ex returns IS_NULL when RfcSAPUCToUTF8 fails. Accessing Z_STRVAL on an IS_NULL zval is undefined behaviour. Return an empty zend_string in that case instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…error path - INT2 lower bound was -32767 instead of -32768, silently rejecting one valid value and showing a wrong error message - INT8 was cast to (int) before ZVAL_LONG, truncating 64-bit values on any platform where sizeof(int) < sizeof(RFC_INT8) - rfc_get_bcd_decfloat_value ignored RFC errors that were neither RFC_OK nor RFC_BUFFER_TOO_SMALL; the buffer was leaked and no exception was thrown Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…or paths rfc_set_table_row: the memcpy(field_desc.name, field_name_u, strlenU(...)) was both redundant (RfcGetFieldDescByName already populates field_desc.name) and wrong (strlenU returns SAP_UC units, not bytes, so only half the string was copied, corrupting the field name for any multi-character identifier). rfc_get_table_value / rfc_get_table_line: ZVAL_NULL was called after array_init without a preceding zval_ptr_dtor, leaking the array on every error path inside the row/field iteration loops. rfc_describe_type / rfc_wrap_field_description / rfc_wrap_parameter_description / rfc_describe_function_interface: same array-init-without-dtor pattern on SDK error paths, reachable via getFunctionDescription(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Pv6 copy-paste getAttributes(), ping(), getFunction(): throw ConnectionException instead of passing NULL to RfcGetConnectionAttributes/RfcPing/RfcGetFunctionDesc when the connection was already closed via close(). invoke(): call RfcIsConnectionHandleValid before RfcCreateFunction so a stale handle (connection dropped without close()) is detected early rather than crashing inside the SDK. sapnwrfc_open_connection: update rfc_login_params_len to the actual count of string keys after the loop (pre-allocation included numeric keys, so the SDK could have received garbage entries). Throw when i==0 (all keys were numeric) instead of passing an empty params array to RfcOpenConnection. getAttributes: fix copy-paste bug where partnerIPv6 was reading attributes.partnerIP instead of attributes.partnerIPv6. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getFunction: zend_update_property_str already increments the string's refcount when it stores the value; passing zend_string_copy(function_name) bumped it a second time, leaking one reference per getFunction() call. getName: RETURN_STR transfers ownership of intern->name to the return value, decrementing it when the caller drops the return value. Since the function object still holds intern->name and releases it in its free handler, this caused a double-free. RETURN_STR_COPY increments the refcount first so both sides own one reference. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d array_init RETURN_NULL() overwrites the return_value zval without releasing the previous value. After object_init_ex() or array_init() the return_value holds a live object/array, so every subsequent RETURN_NULL() leaked it. getFunction: two error paths after object_init_ex (RfcGetParameterCount failure and RfcGetParameterDescByIndex failure in the parameter-count loop) now call zval_ptr_dtor(return_value) before RETURN_NULL(). invoke: three error paths in the result-collection loop after array_init (RfcGetParameterDescByIndex, RfcIsParameterActive, rfc_get_parameter_value) now call zval_ptr_dtor(return_value) before RETURN_NULL(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… error message setTraceLevel: the "l" specifier in zend_parse_parameters writes a zend_long, not an unsigned int. On 64-bit builds the write overflows into adjacent stack space. Changed the local to zend_long and added an explicit (unsigned int) cast at the RfcSetTraceLevel call site where the SDK expects an unsigned value. isParameterActive: the connection-closed error message said "Failed to set status" — a copy-paste from setParameterActive. Changed to "get status" to match what the function actually does. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace blanket build/ ignore with specific subdirs (output/, php/, sap/, workspace/) so build-windows.ps1 and build-windows.md can be tracked. Add Windows nmake output dirs (x64/, x86/), generated batch files (configure.bat, config.nice.bat, configure.js), and *.log. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collect built DLLs into build/output/<variant>/ instead of leaving them in the nmake-generated x64/Release_TS/ subfolder inside the source tree. Add -OutputDir parameter (default: build/output/) to allow overriding the destination. Re-runs skip already-extracted workspace content. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The \$invalidateCache bool parameter was declared in arginfo and the stub file but was never parsed or used in the C implementation — passing true had no effect. Wire it up: parse the optional 'b' argument and clear the RFC SDK function desc cache when true, independent of the connection-level use_function_desc_cache flag. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lues rfc_set_float_value called convert_to_double(value) and rfc_set_bcd_decfloat_value called convert_to_string(value) directly on the zval pointer received from the caller's invoke() loop. That pointer lives inside the shared HashTable of the input array, so the in-place conversion silently changed the type of the original PHP array element (int -> double, or int/float -> string) visible to the caller after invoke() returned. Fix rfc_set_float_value: extract the numeric value with a direct cast instead of calling convert_to_double. Fix rfc_set_bcd_decfloat_value: copy the zval with ZVAL_COPY_VALUE before calling convert_to_string, so the caller's value is untouched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ructs rfc_get_int1_value and rfc_get_int2_value used (int) when assigning to ZVAL_LONG, which expects zend_long. On 64-bit platforms zend_long is 64 bits while int is 32 bits; the intermediate (int) cast was superfluous and inconsistent with the (zend_long) cast already used by INT8. Change both to (zend_long) for consistency. Remove three unused struct typedefs from exceptions.c (sapnwrfc_exception_object, sapnwrfc_connection_exception_object, sapnwrfc_functioncall_exception_object). The exception classes use standard PHP registration without custom allocators, so these structs were dead code. The first also had an incorrect pointer member (zend_object *zobj) instead of the embedded form used everywhere else. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n table read RfcMoveTo return value was silently ignored and both RfcMoveTo and RfcGetCurrentRow were called with NULL for error_info, losing any SDK error details. RfcGetCurrentRow result was also never checked for NULL before being passed to rfc_get_table_line, which would have dereferenced the NULL handle via RfcDescribeType. Now check RfcMoveTo rc, check RfcGetCurrentRow for NULL, and capture error_info in both calls so exceptions carry meaningful error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the root cause and proximate cause of the integer overflow reported in #163.
Bug 1 —
invoke()does not detectRfcCreateFunctionfailure (sapnwrfc.c)RfcCreateFunctionsignals failure by returningNULL; it does not set anRFC_RCreturn value. The previous guard checkedrc, which was initialised toRFC_OKat the top of the function and never updated — so aNULLfunction handle silently passed the check and propagated into all subsequent RFC SDK calls.Bug 2 — use-after-free and integer overflow in
rfc_get_parameter_value(rfc_parameters.c)When
RfcGetTable()fails in theRFCTYPE_TABLEbranch ofrfc_get_parameter_value, the error path freedparameter_name_uand setvaluetoNULL, but did not return. Execution fell through to the next line, which passed the freed pointer and an uninitialisedtable_handletorfc_get_table_value().If the RFC SDK accepted the garbage handle without error,
RfcGetRowCount()could return an arbitrarily large value. The subsequent loop then tried to populate a PHP array with that many entries, triggering PHP'ssafe_emalloc()overflow guard:The equivalent code in
rfc_get_field_value()already returns early in the same error case; this PR bringsrfc_get_parameter_value()in line with that pattern.How the two bugs chain
Under load,
RfcCreateFunctioncan fail (e.g. memory pressure, stale handle) → Bug 1 lets theNULLhandle through →RfcGetTable(NULL, ...)fails → Bug 2 continues with an uninitialisedtable_handle→ garbage row count → PHP integer overflow.Changes
sapnwrfc.c: checkfunction_handle == NULLinstead ofrc != RFC_OKafterRfcCreateFunctionrfc_parameters.c: add missingreturn valuein theRFCTYPE_TABLEerror branch ofrfc_get_parameter_valueTest plan
make test)