vm fixes#416
Merged
Merged
Conversation
uc_vm_insn_store_val() and uc_vm_insn_update_val() pop an owned value and
hand it to ucv_key_set(). On success ucv_key_set() stores the value and
returns a new reference to it, which the caller pushes onto the stack; on
failure it stores nothing and returns NULL. The failure paths neither
stored nor released the value, so it leaked, for example when assigning a
property on a non-container (`let x = 1; x.y = {};`), on an immutable
target, or with an invalid array key (`let a = []; a["x"] = {};`).
These three cases originate in different places (the default switch
branch, a failing assert_mutable_value() and a NULL return from
ucv_key_set()), so a single per-branch guard cannot cover them all.
Instead release the value from one common cleanup point, and on the sole
success path clear the local so that the reference now owned by the stack
is not released a second time.
The third caller, uc_vm_insn_sobj(), passes borrowed stack values that
are freed by the subsequent stack pop and is deliberately left unchanged.
Signed-off-by: Felix Fietkau <nbd@nbd.name>
The single-name branch of uc_vm_insn_import() tested `from <= prog->exports.count`, allowing an index equal to the export count to read one element past the end of exports.entries[]. With an empty export table this dereferences entries[0] on a NULL array. The parallel wildcard import loop already bounds the index with `<`; use `<` here as well. Signed-off-by: Felix Fietkau <nbd@nbd.name>
The signed integer code paths in uc_vm_value_arith() evaluated INT64_MIN / -1 and INT64_MIN % -1 directly, which overflows and traps with SIGFPE on common architectures, crashing the interpreter. Return the mathematically correct unsigned result 2^63 for the division, consistent with the existing behaviour of INT64_MIN * -1 and 0 - INT64_MIN, and 0 for the modulo. Signed-off-by: Felix Fietkau <nbd@nbd.name>
uc_vm_insn_store_var() and uc_vm_insn_update_var() raised the reference error for assignments to undeclared variables in strict mode but then proceeded to perform the assignment anyway. When the exception was caught, the variable had come into existence in the global scope. Abort the instruction after raising the exception. Signed-off-by: Felix Fietkau <nbd@nbd.name>
uc_vm_stack_push() takes ownership of the passed value. uc_vm_resolve_upval() replaced a pushed upvalue with a new reference to its resolved value but never released the upvalue reference itself, leaking one reference per push. Triggered e.g. by iterating a wildcard module namespace object with for-in, where the object iterator pushes the contained upvalue references. Signed-off-by: Felix Fietkau <nbd@nbd.name>
uc_require_library() returns an owned reference to the module scope, the module registry keeps its own reference. uc_vm_insn_dynload() never released the returned reference, keeping the module scope alive even after removal from the module registry. Signed-off-by: Felix Fietkau <nbd@nbd.name>
uc_vm_get_error_context() accessed callframes.entries[-1] when an exception was raised while no callframe existed, e.g. when an embedder invokes uc_vm_call() on a non-callable value outside of any script execution, crashing with SIGSEGV. Signed-off-by: Felix Fietkau <nbd@nbd.name>
The signed integer code path in uc_vm_value_arith() negated the result whenever the base was negative, regardless of the exponent, yielding e.g. (-2) ** 2 = -4, (-2) ** 0 = -1 and (-2) ** -2 = -0.25. The double code path already produced the mathematically correct results via pow(), so both paths disagreed for equal values. Only negate the result when the exponent is odd. Adjust the exponentiation testcase which had codified the incorrect behaviour; the equivalent double expression (-2.0) ** -2 already yielded 0.25 before this change. Signed-off-by: Felix Fietkau <nbd@nbd.name>
uc_vm_insn_mobj() switches on ucv_type() but matched the array case with the json-c enumerator json_type_array, which only works because its value happens to coincide with UC_ARRAY. Use the correct enum. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Resource type prototypes live outside the GC value list (uc_type_declare() uses ucv_object_new(NULL)), so they are reclaimed by reference count alone. GC-managed values can hold a reference to them, e.g. a prototype returned by proto(), and those references are only dropped during ucv_freeall(). Releasing the prototypes before ucv_freeall() could therefore leave one at a nonzero refcount, and its final ucv_put() would then hit ucv_freeall()'s retain phase, which only marks the object UC_NULL and defers the free to the value-list sweep. As these prototypes are not on that list, the object leaks. Move the release after ucv_freeall() so the last reference is dropped outside the retain phase and the object is freed immediately. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Owner
|
Merged - thanks! |
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.
A number of fixes for vm issues discovered in a LLM code review session.