Skip to content

Modules: fix SharedDict.pop() for unexpired keys in timeout zones - #1102

Open
horus wants to merge 1 commit into
nginx:masterfrom
horus:fix/shareddict-pop-timeout
Open

Modules: fix SharedDict.pop() for unexpired keys in timeout zones#1102
horus wants to merge 1 commit into
nginx:masterfrom
horus:fix/shareddict-pop-timeout

Conversation

@horus

@horus horus commented Jul 28, 2026

Copy link
Copy Markdown

Proposed changes

Problem

SharedDict.pop() returns undefined for unexpired entries in zones declared with timeout= — the entry is deleted, but its value is lost. Both engines are affected (njs VM and QuickJS). Present in current master.

Runtime probe against a timeout=5m zone (from a production OIDC deployment where every login callback failed because session states live in a timeout zone):

add=true  get=alive  pop=undefined  get_after_pop=undefined

Root cause

ngx_js_dict_delete() (and ngx_qjs_dict_delete()) removes the node from the expire rbtree before evaluating expiration:

if (dict->timeout) {
    ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire);
}

ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);

if (retval != NULL) {
    ...
    if (!dict->timeout || now < node->expire.key) {   /* already poisoned */

nginx core ngx_rbtree_delete() poisons the removed node unconditionally — node->key = 0, left/right/parent = NULL (the /* DEBUG stuff */ block is not behind an NGX_DEBUG guard). So node->expire.key is already 0 when the comparison runs, now < node->expire.key is always false, the helper returns NGX_DECLINED, and pop() maps that to undefined.

Scope: only pop() — the other callers of the same helper pass retval = NULL, so the bogus expire check never runs; and only zones with timeout= — otherwise !dict->timeout short-circuits. get()/delete()/set()/incr() are unaffected.

Fix

Snapshot node->expire.key into a local variable before the rbtree removal, and compare against the snapshot — in both ngx_js_dict_delete() and ngx_qjs_dict_delete(). nginx core's key zeroing is legitimate node poisoning; njs must not read the field after removal.

Tests

nginx/t/js_shared_dict.t previously covered "pop expired key from a timeout zone" (zone=foo, timeout=2s) and "pop unexpired key from a no-timeout zone" (zone=bar) — but never "pop unexpired key from a timeout zone". Added the missing pair:

http_get('/set?dict=foo&key=POP&value=alive');
like(http_get('/pop?dict=foo&key=POP'), qr/alive/, 'pop unexpired foo.POP');
like(http_get('/pop?dict=foo&key=POP'), qr/undefined/, 'pop deleted foo.POP');

Verification

Minimal reproduction (runs in one minute):

nginx.conf:

events {}
http {
    js_import probe from probe.js;
    js_shared_dict_zone zone=foo:1M timeout=5m;

    server {
        listen 127.0.0.1:8080;
        location /t { js_content probe.t; }
    }
}

probe.js:

function t(r) {
    ngx.shared.foo.add('k', 'alive');
    var g = ngx.shared.foo.get('k');
    var p = ngx.shared.foo.pop('k');
    var g2 = ngx.shared.foo.get('k');
    r.return(200, `get=${g} pop=${p} get_after=${g2}\n`);
}
export default { t };
$ curl http://127.0.0.1:8080/t
get=alive pop=undefined get_after=undefined   # before the fix
get=alive pop=alive get_after=undefined       # after the fix

(Repeated curls print the same line: each request re-adds the key that the previous pop deleted.)

Verified on macOS/arm64 and Linux/amd64 (Debian 12, nginx 1.30.4). The new pop unexpired foo.POP test fails without the fix, while the full js_shared_dict.t suite passes with it. The patched Linux module also restores the OIDC callback flow that originally exposed the issue.

The QuickJS path is patched identically and verified at runtime with js_engine qjs on nginx 1.30.4 — pop() correctly returns unexpired values from a timeout= zone with both QuickJS flavors (quickjs-ng v0.9.0 and bellard quickjs-2026-06-04). The js_shared_dict.t suite itself runs the default njs VM engine.

Checklist

  • I have read the CONTRIBUTING document.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have checked that the relevant tests pass after adding the changes.

Previously, SharedDict.pop() removed an entry from the expiration
rbtree before checking whether it had expired. ngx_rbtree_delete()
clears the removed node's key, so the subsequent expiration check
treated every entry in a timeout zone as expired. pop() therefore
deleted unexpired entries but returned undefined instead of their
values.

The fix preserves the expiration key before removing the node and
uses the saved value for the check in both the njs and QuickJS
implementations. Tests are added for popping an unexpired entry from
a timeout zone.
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@horus

horus commented Jul 28, 2026

Copy link
Copy Markdown
Author

I have hereby read the F5 CLA and agree to its terms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

1 participant