Skip to content

Sanitize index-derived label values to valid UTF-8#1646

Merged
SuperQ merged 1 commit into
prometheus:mainfrom
lukeod:index-label-utf8
Jul 3, 2026
Merged

Sanitize index-derived label values to valid UTF-8#1646
SuperQ merged 1 commit into
prometheus:mainfrom
lukeod:index-label-utf8

Conversation

@lukeod

@lukeod lukeod commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

#968 replaced invalid UTF-8 in metric values with U+FFFD, but only on the pduValueAsString path. Labels built from table indexes go through indexesToLabels directly, so they never get sanitized. When a device returns non-UTF-8 bytes in a string-typed index (e.g. a DisplayString index containing Latin-1 or raw binary), NewConstMetric rejects the label value, the metric is replaced with an invalid metric, and the gather fails with an snmp_error, so the whole scrape errors out.

This applies the same strings.ToValidUTF8(..., "�") treatment to index label values, so those metrics survive with a replacement character instead of erroring out. Valid non-ASCII UTF-8 in index values passes through unchanged; only invalid byte sequences are replaced.

One known limitation: ToValidUTF8 replaces each run of invalid bytes with a single replacement character, so two rows whose indexes differ only within an invalid run (say one-byte DisplayString indexes 0xFE and 0xFF) end up with identical label sets, and the gatherer rejects the duplicate series. That still fails the scrape, but it requires colliding garbage rather than any garbage, and it's the same trade-off #968 already accepted for values. Lookups are unaffected either way, since they key off the raw index OIDs rather than the sanitized string.

This does change behavior asserted by two existing tests, which expected the invalid-UTF-8 error. Those assertions date back to #303, which was about turning a panic into a graceful error rather than about erroring being the desired end state. #968 later settled on sanitizing for the value path, and this brings the index path in line with that. I've updated the two test cases accordingly.

The practical motivation is to ensure safety for #1610, which adds display_hint formatting for index labels - the a/t hint formats write device bytes into labels and would hit that error path.

Invalid UTF-8 bytes in index values previously caused NewConstMetric to
fail, dropping the metric and emitting a scrape error. Replace invalid
sequences instead, matching how pduValueAsString handles metric values.

Signed-off-by: lukeod <l.odonnell11@gmail.com>

@SuperQ SuperQ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good find, thanks!

I think the limitation of duplicates is fine. For the most part we recommend users keep the numeric indexes (i.e ifIndex) to avoid string collisions already (i.e. ifDescr).

If the collision is from the new enum mapping, it's at least easier to fix in the MIB. For #1610, it could be more difficult to avoid collisions.

@SuperQ SuperQ merged commit 931faac into prometheus:main Jul 3, 2026
13 checks passed
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.

2 participants