fix(base): an Option<const char*> owns the string it is given - #15
Merged
Merged
Conversation
SetFromString stored the caller's pointer, so a string option was only as alive as whatever buffer it was parsed out of. InitOptionsFromEnv reads each variable into a local StringU8 and hands over its c_str(), which dies at the end of the loop iteration: every const char* option set from the environment has pointed at freed memory since base stopped calling getenv (#14). It does not read as a crash. rx takes its capture path from RX_UI_SHOT=<path>, and with the dangling value the engine wrote a 51 KB png to a filename made of whatever the freed bytes happened to spell. Whoever calls SetFromString next gets a different kind of wrong. The other two callers show the shape of the trap. option_file.cc knew, and interns every value into a pool that outlives the options (InternValue). recreation's platform-profile loader did not, and passes the c_str() of a std::string that goes out of scope one line later. Three call sites, two of them holding it wrong, is an API telling you where the ownership belongs: with the option. ParseFromString now copies into a buffer the option keeps, so a value survives its source, Reset() drops it again, and callers can pass anything they have at hand. Only const char* options carry the buffer -- detail::OwnedText is empty for every other T -- so an Option<int> is the size it always was. base/option_test.cc covers what nothing covered: parsing per type, a value outliving the buffer it came from, the same through the environment, Reset, and a rejected parse leaving the old value in place. Three of the six fail without this change.
Force67
added a commit
to Force67/recreation
that referenced
this pull request
Sep 16, 2026
Retarget the build onto rx main (2198552), which moved its ultragui integration out of apps/editor into an rx::ui module and put an engine splash on it. rx::app links that module, so the plate needs nothing from the game -- but libultragui stops being optional, and the engine now owns three files and six shaders recreation carried its own copies of. libultragui is configured ahead of rx: rx::ui adds the checkout itself when no ultragui target exists yet, and that copy would carry ultragui's Lua scripting instead of recreation's C# host (adding the same source directory twice is a hard error anyway). The platform and RHI shims now come from the engine, as do the ugui pipeline shaders -- both repos embedded those six blobs to the same generated header path, which ninja refuses outright. RX_LIBULTRAGUI_DIR is forwarded for the builds that compile no HUD of their own; android checks the library out for that. The HUD's own Vulkan backend stays, renamed HudRenderBackend so it no longer collides with rx::ui::GuiRenderBackend. It keeps the growing descriptor pool chain a vanilla Scaleform screen's art needs, which the engine's single fixed block does not hold; it can go when that lands in rx::ui. The flake stops reading the siblings out of one developer's home directory. All six (rx, zetanet, nanobuf, libultragui, kinema, equilibrium) are fetched from github at the revisions build.yml pins, with submodules wherever CI checks them out recursively, so the sandbox and CI compile the same engine on any machine. What git does not carry does not arrive: rx's FidelityFX/DLSS/NRD/Jolt SDKs are downloads, so that build configures with those features off and a working copy stays the way to build with them. equilibrium moves to the head of devel5, the branch .gitmodules names and the pin had drifted off: a commit on no branch at all, five behind devel5 and one ahead, reachable by sha alone. devel5 brings the STL out of base's production surface, the security pass behind it, and a string option that owns its value instead of pointing at the buffer it was parsed from (Force67/equilibrium#15) -- the bug that had RX_UI_SHOT writing captures to a filename made of freed memory. equilibrium also stops being welded to the submodule. It is where base:: comes from, and it was reachable one way only: third_party/equilibrium, checked out recursively, which drags in its own eight submodules (googletest, musl, mbedtls, tracy and the rest) that sit behind EQ_BUILD_TESTS and never compile here. Every CI job cloned all of them, and `src = self` carries no submodules at all, so the nix build could not see equilibrium in the first place. RECREATION_EQUILIBRIUM_DIR now names the checkout, the submodule stays the default, the sandbox gets a tarball input at the revision the submodule records, and the checkouts that only need equilibrium itself ask for one level instead of eight. With that and slangc (rx compiles .slang shaders now), `nix build` produces recreation, recreation-server and esminfo for the first time. The sibling pins move together, because the retarget needed fixes in three of them. zetanet spells out the <string.h> its bare mem* calls always needed (Force67/zetanet#2). libultragui can load a font from memory instead of only from a path (Force67/libultragui#5). rx uses that for the splash wordmark, which deletes the temp file it used to write, the pid in its name and the <unistd.h> that came with it -- the include MSVC does not have, and the reason a windows build of anything linking rx::ui failed (Force67/rx#60). Verified against rx 2198552: full build, 107/107 tests, and a windowed run that shows the plate and hands the frame back to the game's own UI when it expires. RECREATION_NET=OFF, the no-SDL3 path and an out-of-tree equilibrium all configure. `nix build .#default` completes.
Force67
added a commit
to Force67/recreation
that referenced
this pull request
Sep 16, 2026
Retarget the build onto rx main (2198552), which moved its ultragui integration out of apps/editor into an rx::ui module and put an engine splash on it. rx::app links that module, so the plate needs nothing from the game -- but libultragui stops being optional, and the engine now owns three files and six shaders recreation carried its own copies of. libultragui is configured ahead of rx: rx::ui adds the checkout itself when no ultragui target exists yet, and that copy would carry ultragui's Lua scripting instead of recreation's C# host (adding the same source directory twice is a hard error anyway). The platform and RHI shims now come from the engine, as do the ugui pipeline shaders -- both repos embedded those six blobs to the same generated header path, which ninja refuses outright. RX_LIBULTRAGUI_DIR is forwarded for the builds that compile no HUD of their own; android checks the library out for that. The HUD's own Vulkan backend stays, renamed HudRenderBackend so it no longer collides with rx::ui::GuiRenderBackend. It keeps the growing descriptor pool chain a vanilla Scaleform screen's art needs, which the engine's single fixed block does not hold; it can go when that lands in rx::ui. The flake stops reading the siblings out of one developer's home directory. All six (rx, zetanet, nanobuf, libultragui, kinema, equilibrium) are fetched from github at the revisions build.yml pins, with submodules wherever CI checks them out recursively, so the sandbox and CI compile the same engine on any machine. What git does not carry does not arrive: rx's FidelityFX/DLSS/NRD/Jolt SDKs are downloads, so that build configures with those features off and a working copy stays the way to build with them. equilibrium moves to the head of devel5, the branch .gitmodules names and the pin had drifted off: a commit on no branch at all, five behind devel5 and one ahead, reachable by sha alone. devel5 brings the STL out of base's production surface, the security pass behind it, and a string option that owns its value instead of pointing at the buffer it was parsed from (Force67/equilibrium#15) -- the bug that had RX_UI_SHOT writing captures to a filename made of freed memory. equilibrium also stops being welded to the submodule. It is where base:: comes from, and it was reachable one way only: third_party/equilibrium, checked out recursively, which drags in its own eight submodules (googletest, musl, mbedtls, tracy and the rest) that sit behind EQ_BUILD_TESTS and never compile here. Every CI job cloned all of them, and `src = self` carries no submodules at all, so the nix build could not see equilibrium in the first place. RECREATION_EQUILIBRIUM_DIR now names the checkout, the submodule stays the default, the sandbox gets a tarball input at the revision the submodule records, and the checkouts that only need equilibrium itself ask for one level instead of eight. With that and slangc (rx compiles .slang shaders now), `nix build` produces recreation, recreation-server and esminfo for the first time. The sibling pins move together, because the retarget needed fixes in three of them. zetanet spells out the <string.h> its bare mem* calls always needed (Force67/zetanet#2). libultragui can load a font from memory instead of only from a path (Force67/libultragui#5). rx uses that for the splash wordmark, which deletes the temp file it used to write, the pid in its name and the <unistd.h> that came with it -- the include MSVC does not have, and the reason a windows build of anything linking rx::ui failed (Force67/rx#60). Verified against rx 2198552: full build, 107/107 tests, and a windowed run that shows the plate and hands the frame back to the game's own UI when it expires. RECREATION_NET=OFF, the no-SDL3 path and an out-of-tree equilibrium all configure. `nix build .#default` completes.
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.
SetFromStringstored the caller's pointer, so anOption<const char*>was onlyas alive as the buffer it was parsed out of.
InitOptionsFromEnvreads eachvariable into a local
StringU8and hands over itsc_str():Every string option set from the environment has pointed at freed memory since
base stopped calling
getenv(#14).It does not read as a crash
rx takes its screenshot path from
RX_UI_SHOT=<path>. With the dangling valuethe engine did not skip the capture — it wrote a 51 KB png to a filename made of
whatever the freed bytes spelled, which landed in the repository root as
@\2351\364\377\377. A knob likeREC_TERRAIN_EDITSdecides where terrainedits are saved. The failure mode is an arbitrary path, not an error.
Where the ownership belongs
Three call sites hand
SetFromStringa value:option_file.ccInternValue)InitOptionsFromEnvvalue.c_str()of astd::stringparameterTwo of three holding it wrong is an API pointing at its own answer: the option
should own its string.
ParseFromStringnow copies into a buffer the optionkeeps,
Reset()drops it again, and any caller can pass whatever it has athand.
detail::OwnedText<T>is empty for every otherTand sits behind[[no_unique_address]], so anOption<int>is the size it always was — thereis a test for that.
The
const char*branch ofdetail::ParseOptionis gone with it; nothingreaches it any more, and leaving a second, binding-not-copying path in place is
how this comes back.
Tests
base/option_test.ccis new — the option machinery itself had none, which ishow #14 shipped. It covers parsing per type, a value outliving the buffer it
came from, the same through the environment,
Reset(), a rejected parse leavingthe previous value in place, and the storage claim above. Three of the six fail
on
devel5without this change:All four of the suite's existing test binaries pass with it.