Skip to content

Fixed call argument value snapshotting.#1060

Merged
xeioex merged 1 commit into
nginx:masterfrom
xeioex:fix_call_argument
May 18, 2026
Merged

Fixed call argument value snapshotting.#1060
xeioex merged 1 commit into
nginx:masterfrom
xeioex:fix_call_argument

Conversation

@xeioex
Copy link
Copy Markdown
Contributor

@xeioex xeioex commented May 16, 2026

Since fd5e523 (0.9.7), njs has evaluated all call argument expressions before emitting the frame and PUT_ARG instructions, this fixed await expressions in call arguments. This preserved argument side effects before callee validation, but left PUT_ARG reading argument values only after later argument expressions had already run.

As a result, an earlier argument backed by a mutable variable slot could observe a later mutation of the same slot. For example, f(a, a = 2) passed 2 as both arguments instead of preserving the first value as 1. The same issue applied to later argument effects such as getters.

The fix is to preserve arguments using temporary registers where appropriate.

This fixes #1059 issue on Github.

@VadimZhestikov
Copy link
Copy Markdown
Contributor

  1. Observation: njs_generate_argument_uses_index has dead recursive tree-walk logic (children are always
    NULL in current call sites). A comment explaining that this function is called only on leaf nodes would
    prevent future confusion — or the child checks could be removed with a njs_assert(node->left == NULL &&
    node->right == NULL).

  2. Suggestion: The node->left->index == index pre-check before recursion in
    njs_generate_argument_uses_index would silently fail to detect variable use inside compound subtrees if
    the function's call conditions ever change. Consider replacing with an unconditional recursive call
    (which node->temporary would likely prevent from being reached in practice anyway).

Both are minor.

Copy link
Copy Markdown
Contributor

@VadimZhestikov VadimZhestikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@github-project-automation github-project-automation Bot moved this from New to In Progress in NGINX OSS Unified Workspace May 18, 2026
Since fd5e523 (0.9.7), njs has evaluated all call argument expressions
before emitting the frame and PUT_ARG instructions.  This fixed await
expressions in call arguments and preserved argument side effects before
callee validation, but left PUT_ARG reading argument values only after later
argument expressions had already run.

As a result, an earlier argument backed by a mutable variable slot could
observe a later mutation of the same slot.  For example, f(a, a = 2) passed
2 as both arguments instead of preserving the first value as 1.  The same
issue applied to later argument effects such as getters.

The fix is to preserve affected argument values in temporary registers where
appropriate.

This fixes nginx#1059 issue on Github.
@xeioex
Copy link
Copy Markdown
Contributor Author

xeioex commented May 18, 2026

Observation: njs_generate_argument_uses_index has dead recursive tree-walk logic (children are always
NULL in current call sites)

The recursive walk is not dead. This is easily discoverable with simple assert statement + current tests.

It is reached by compound argument expressions whose result aliases a mutable slot, for example assignment expressions f(a = b, b = 2) through left, and comma expressions f(a, (a = 2, a), a = 3) through right.

@xeioex xeioex force-pushed the fix_call_argument branch from aa70e9a to 2880ef3 Compare May 18, 2026 15:46
@xeioex xeioex merged commit 7d1706c into nginx:master May 18, 2026
2 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in NGINX OSS Unified Workspace May 18, 2026
@xeioex xeioex deleted the fix_call_argument branch May 18, 2026 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Evaluation of arguments has changed

2 participants