[jwrosewell]

Reading a weighted value through a chain of calls, such as $results->getValuesAsWeightedStringList('RegisteredCountry')->getValue()->get(0)->getValue(), stopped PHP with std::bad_alloc or std::length_error and exit code 134, where holding each step in a variable worked. This makes a weighted list's get return a copy of the weighted value that PHP owns, so the chained read is the held read.

Closes #92.

Why it stopped PHP

The C++ Value<T>::getValue() returns its value by value (Value.hpp line 131 at 7992af70, T getValue() const), so $w->getValue() is a new list that PHP holds only while the chain needs it. SWIG's PHP vector get returns a reference into the list (Lib/php/std_vector.i line 45 at SWIG v4.5.1, const_reference get(int i)), and the wrapper hands that reference to PHP without ownership. In the generated wrapper at rc 36b71fd, PHP_METHOD(FiftyOneDegreesIpiWeightedStringListSwig,get) takes the address of the element (line 14884) and passes it on with ownership 0 (line 14921, SWIG_SetPointerZval(return_value, (void *)result, SWIGTYPE_p_WeightedValueT_std__string_t, 0)). Once get returns, nothing holds the list, PHP frees it, and the weighted value read next points into freed memory. The same holds for the four other weighted list types.

Valgrind shows it. With the chained read added to a script and run on rc's wrapper under Valgrind, run 43588217293 reports "Invalid read of size 8" in FiftyOneDegreesIpiWeightedValueStringSwig::getValue at an address "8 bytes inside a block of size 40 free'd", the block freed by SWIG_Php_free_obj_WeightedStringListSwig, reached from the list value's getValue. That run belongs to the check proposed for #94, whose red is this read.

No specification covers this, because it is how the PHP binding holds memory, not how a Property behaves. The behaviour the test asks for is that a chained read gives what the same read with each step held in a variable gives.

What changed

Files are linked at the head, 4497a2e0.

1. ip-intelligence/on-premise/ipi-php.i gives each of the five weighted value types (std::string, std::vector<uint8_t>, bool, int and double) an output typemap for const WeightedValue<T> & that returns new WeightedValue<T>(*$1) with ownership 1, so PHP owns a copy and frees it with the object. Only the lists' get returns that type by reference, so nothing else in the wrapper changes.
2. ip-intelligence/on-premise/src/php8/ipi_wrap.cxx is regenerated with SWIG 4.5.1 by generate-wrapper.sh, the version the committed wrapper was made with. The diff is the five get methods, 15 lines.

The test, ip-intelligence/tests/ChainedCallTests.php, in the Integration suite, runs a script in its own PHP, because reading freed memory can stop PHP. On the free Lite file it reads RegisteredCountry, RegisteredName and RegisteredOwner 1,000 times each, holding each step, then chained, then with only the weighted value kept while other memory is used, and fails unless all three agree every time and PHP ends with status 0.

Runs

Each throwaway run uses a workflow with two parts. One job builds SWIG 4.5.1, regenerates the wrapper from ipi-php.i, and compares it with the committed wrapper. The test jobs build both extensions as CI does, put the free Lite IP Intelligence file where php.ini names it, and run ChainedCallTests and IpIntelligenceOnPremiseTests with PHP 8.0, 8.3 and 8.5.

• Red, run 43562978010 on dc6cb85, failure. Its parent is the test commit 7dfd201 on rc 2104ad7. With PHP 8.0, 8.3 and 8.5 the chained call test failed ("Tests: 20, Assertions: 123, Failures: 1"), PHP ending with std::bad_alloc or std::length_error and status 134, and the other 19 tests passed.
• The generator run, run 43562984521 on the typemap commit aba6ee3, made the wrapper that 04e8aa2 commits. Its test jobs ran the wrapper committed before, so they failed as the red did.
• rc 36b71fd was merged in at 976da64 with no conflict. It had changed ipi-php.i and the wrapper in another place (#100).
• Green, run 43583401562 on 51af459, success. Its parent is the head 976da64. The generator job regenerated the wrapper from ipi-php.i and found it equal to the committed one (git diff --exit-code), and with PHP 8.0, 8.3 and 8.5 the 20 tests passed, the chained call test included ("OK (20 tests, 127 assertions)").
• The first green, run 43582355562, passed on PHP 8.0, while PHP 8.3 and 8.5 stopped building the Device Detection extension on Ubuntu 26.04 (#107), so the test jobs were moved to ubuntu-24.04.
• rc 49ea15c, which holds #117, was merged in at 4497a2e with no conflict. Build with GCC 15 by taking the cxx rc that fixes it, and remove each on-premise extension's settings when it shuts down so dl() no longer corrupts the heap #117 moved the cxx submodule to 0312236c and changed ipi-php.i and the wrapper in another place (its shutdown now removes the extension's settings), and the merged ipi-php.i keeps both changes. The generator run, run 43661338232 on a97b7a7, whose parent is 4497a2e, regenerated the wrapper with SWIG 4.5.1 from the merged ipi-php.i and cxx 0312236c. It came out the same, byte for byte, as the wrapper the merge made, and both generate-wrapper.sh --check runs passed, so no wrapper commit was needed. The draft's own wrapper checks passed on 4497a2e (run 43662236020).
• Full build, run 43663082686 on the head 4497a2e, success. The run printed 9 of 9 Setting 'PullRequestSha' lines on 4497a2e, and Configure printed "Left out 5 macOS entries". It ran on rc 49ea15c, already merged into the head, which holds #117 and its move of cxx to 0312236c. All eight jobs passed, Ubuntu with PHP 7.4 and 8.0 to 8.5 and Windows with PHP 8.3. In the Ubuntu PHP 8.3 job, on ubuntu-26.04, the Unit suites passed 1,044 tests (pipeline/core 509 with 1 skipped, pipeline/engines 9, pipeline/cloudrequestengine 90, pipeline/did 192, pipeline/translation 22, pipeline/derivedproperty 52, device-detection/cloud 29, device-detection/onpremise 89 and ip-intelligence 52) and the Integration suites 133 (pipeline/cloudrequestengine 6, device-detection/cloud 36, location 16 and ip-intelligence 75).

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

What remains

• Device Detection's wrapper was read for the same fault. Its lists hold strings, which get copies into PHP strings, and its three results handed to PHP without ownership (getMetaData, getKeys and engine_get in hash_wrap.cxx) point into the engine, which lives as long as PHP.
• In the IP Intelligence wrapper, the same holds for getMetaData, getKeys and ipi_engine_get. The configuration's eleven collection getters (getStrings, getGraph and the others on FiftyOneDegreesIpiConfigIpiSwig) also hand PHP a pointer into the configuration without ownership. That is safe while the configuration is held, as the engine builder and every example hold it, and a chain from a configuration that nothing holds would meet the same fault. Nothing reads them that way, and this draft leaves them. Raised as #147.
• Both extensions still register SWIG's pointer classes under the same names, which #94 is about.