fix: copy string params in eval_ctx (with ASan reproducer)#64
Draft
pawelchcki wants to merge 2 commits into
Draft
fix: copy string params in eval_ctx (with ASan reproducer)#64pawelchcki wants to merge 2 commits into
pawelchcki wants to merge 2 commits into
Conversation
Previously, the context held raw pointers to caller-provided string
parameters. The public header documented that "the caller is responsible
for freeing", but in practice this contract is fragile: any caller that
passes a buffer with a shorter lifetime than the next plcs_evaluate_buffer
call (e.g. a stack buffer that goes out of scope, or a heap buffer that
is freed) leaves the engine holding a dangling pointer, which is then
dereferenced inside the string evaluators.
This change makes plcs_eval_ctx_set_str_eval_param strdup() the value and
plcs_eval_ctx_reset() free its previous copy, so the engine owns its own
storage and is no longer at the mercy of caller lifetime management.
Changes:
- Add PLCS_EALLOCATION error for strdup failures.
- Add a portable plcs_strdup() with a CMake HAVE_STRDUP probe.
- Update existing eval_ctx test to assert the stored pointer differs
from the caller's pointer (proves the copy actually happens).
Resurrects the change proposed in #29.
Two utest cases exercise the contract violation that the previous
commit fixes:
1. repro_dangling_str.set_param_then_free_then_get
Set a string param from a heap buffer, free the buffer, read it
back via plcs_eval_ctx_get_string_param. Without the copy, this
returns a dangling pointer; touching it trips ASan
heap-use-after-free.
2. repro_dangling_str.set_param_then_free_then_evaluate
End-to-end through plcs_evaluate_buffer with a tiny in-memory
policy that targets PROCESS_ENVAR with a CMP_WILDCARD pattern.
Without the copy, the UAF surfaces inside string_evaluator_wildcard
reading the freed buffer, with the full call chain
plcs_evaluate_buffer -> evaluate_policy -> evaluate_rules
-> node_evaluator -> evaluate_string -> string_evaluator_wildcard.
These tests fail under -fsanitize=address on the prior tip of main
and pass with the copy-on-set fix in place. Without ASan they may
appear to "pass" since glibc may keep freed memory readable —
running CI on the clang-dev preset is what makes them reliable.
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
2 participants
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.
Resurrects #29:
plcs_eval_ctx_set_str_eval_paramnowstrdup()s the value andplcs_eval_ctx_resetfrees the engine-owned copy, so the engine no longer holds raw pointers to caller-provided buffers.Also adds
c/src/test/test_dangling_str_repro.cwith two ASan tests pinning the contract: one at the API boundary, one end-to-end throughplcs_evaluate_bufferintostring_evaluator_wildcard. Both fail under-fsanitize=addresson the prior tip ofmainand pass with the fix.Test plan
make -C c test— all 57 tests pass (55 existing + 2 new)clang-devpreset with-fsanitize=address,leak,undefinedcleanclang-ci,gcc-ci,msvc-ci