Skip to content

luci-base: don't crash in String.format() on an undefined argument#8745

Open
Cris7015 wants to merge 1 commit into
openwrt:masterfrom
Cris7015:fix-string-format-undefined-crash
Open

luci-base: don't crash in String.format() on an undefined argument#8745
Cris7015 wants to merge 1 commit into
openwrt:masterfrom
Cris7015:fix-string-format-undefined-crash

Conversation

@Cris7015

Copy link
Copy Markdown

Pull request details

Supersedes #8727. GitHub no longer allows reopening that PR after its source
branch was recreated, so this is the replacement PR with the corrected follow-up
from the review discussion there.

Description

The status Overview "Ports" widget (view/status/include/29_ports.js) can
crash with TypeError: Cannot read properties of undefined (reading 'toString')
when a port's netdev reports no statistics. In that case stats.rx_bytes and
similar values are undefined, and the widget formats them through:

'%1024mB'.format(stats.rx_bytes)

This aborts the entire Overview render.

Root cause: String.prototype.format() runs its conversion switch only when the
argument is defined (if (param !== undefined)). For %m, the parsed
pMinLength slot is used as the metric divisor (%1024m), not as a field
width, and the %m switch case clears it afterwards. With an undefined argument
the switch is skipped, so pMinLength stays set while subst stays undefined;
the common padding block then calls subst.toString() and throws.

Fix undefined arguments by using an empty substitution. Preserve the parsed
field width for normal conversions, so e.g. '%10s'.format(undefined) renders
ten spaces as padding plus an empty string. Keep %m special by clearing
pMinLength for undefined %m conversions too, since that slot is the metric
divisor rather than a field width. Also move pad initialization before the
undefined-argument check so the common padding block has a valid pad character
in both paths.

Tested on

OpenWrt version: OpenWrt SNAPSHOT r34203 (target econet/en751221, EcoNet
EN751221) - TP-Link Archer XR500v

LuCI version: master 26.151.60149~1bffbf4

Web browser(s): Chromium (used to capture the console stack trace)

Before: the Overview throws Cannot read properties of undefined (reading 'toString') and renders nothing.

After: the Ports panel renders normally, and formatter checks behave as expected:

'%10s'.format(undefined)       -> ten spaces
'%s'.format(undefined)         -> ""
'%1024m'.format(undefined)     -> ""
'%1024mB'.format(undefined)    -> "B"
'%10s'.format('hi')            -> "        hi"
'%-10s'.format('hi')           -> "hi        "
'%1024m'.format(2097152)       -> "2.00 Mi"

Maintainer

@jow-


Checklist

  • This PR is not from my main/master branch, but a separate branch (fix-string-format-undefined-crash).
  • Each commit has a valid Signed-off-by: Cristian Papa <pcristian292@gmail.com> row.
  • Each commit and the PR title have a valid <package name>: title first-line subject (luci-base: ...).
  • PKG_VERSION: N/A - luci-base has no PKG_VERSION; it is versioned via luci.mk/git.

A `%<n>m` conversion stores its divisor in the min-length slot, but when
the argument was undefined the `if (param !== undefined)` block was
skipped, so the divisor was never cleared while `subst` stayed undefined
and the trailing `subst.toString()` in the padding loop threw.

Use an empty substitution for an undefined argument and preserve the
parsed field width, so values such as `'%10s'.format(undefined)` render
as padding plus an empty string (ten spaces). `%m` stays special: it
reuses the min-length slot as the metric divisor, not a field width, so
clear it for undefined `%m` conversions just like the defined-argument
path does, to avoid padding the empty value out to ~1024 characters.

Also move the pad initialization ahead of the `param !== undefined` check
so the common padding block has a valid pad character for undefined
arguments too.

As a side effect a plain `%s`/`%d` on an undefined argument now yields an
empty string instead of the literal "undefined".

Signed-off-by: Cristian Papa <pcristian292@gmail.com>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit; no new issues found.


Generated by Claude Code

@maxskokov

Copy link
Copy Markdown
Contributor

Worth calling out why this specifically threw instead of just producing bad output: subst/pad are declared once with let subst, n, pad; outside the while loop, so the old code skipped the whole assignment when param was undefined and left subst holding a stale value from the previous placeholder, or genuine undefined for the first one. That's why it hit subst.toString() on undefined instead of just rendering garbage. The fix resets subst unconditionally on every iteration now, not just for the reported %m case, so it's not a narrow patch for one crash path.

@Cris7015

Cris7015 commented Jul 5, 2026

Copy link
Copy Markdown
Author

Good point, thanks.

You're right that the failure mode is broader than just the reported %m crash: since subst and pad are declared outside the formatting loop, skipping the defined-argument branch could leave subst either genuinely undefined or stale from a previous placeholder.

The intent of the patch is to reset the undefined-argument path in general by assigning an empty substitution, while still keeping %m special because its min-length slot is the metric divisor rather than a field width. I can fold that wording into the commit message as well if a reroll is preferred.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants