Skip to content

fix: escape text format label values - #347

Merged
krisztianfekete merged 3 commits into
prometheus:masterfrom
Karthik-Chowdary:fix/346-escape-label-values
Oct 13, 2026
Merged

krisztianfekete merged 3 commits into
prometheus:masterfrom
Karthik-Chowdary:fix/346-escape-label-values

Conversation

@Karthik-Chowdary

@Karthik-Chowdary Karthik-Chowdary commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #346.

Escape backslashes, double quotes, and line feeds when encoding label values in the text exposition format. This prevents label content from breaking out of its quoted value while preserving carriage returns and UTF-8 text.

The tests cover each special character and parse an injection-style value with the Python Prometheus client.

Validation:

  • cargo fmt --check
  • cargo test (64 unit tests and 33 doc tests)

Final benchmark comparison on 211210e (cargo bench --bench text, Criterion 0.8.2, 100 samples on the same host; the PR benchmark harness, including buffer clearing and new cases, was applied to master for an apples-to-apples baseline):

master baseline:
encode                         [34.633 ms 35.146 ms 35.729 ms]
encode_realistic_string_labels [36.049 ms 36.722 ms 37.470 ms]
encode_escaped_label_values    [34.766 ms 35.227 ms 35.758 ms]

this branch:
encode                         [34.575 ms 35.238 ms 35.975 ms]
change                         [-2.3573% +0.2606% +2.7915%], p=0.84
No change in performance detected.

encode_realistic_string_labels [35.540 ms 36.049 ms 36.651 ms]
change                         [-4.2559% -1.8329% +0.6581%], p=0.15
No change in performance detected.

encode_escaped_label_values    [42.614 ms 43.201 ms 43.843 ms]
change                         [+20.088% +22.636% +24.990%], p<0.05

The common and realistic no-escape paths are statistically unchanged. The escaped case is expected to do additional writes.

Final validation on 211210e:

  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • all 95 unit tests and 37 doc tests
  • cargo bench --bench text --no-run

EDIT (@krisztianfekete): I couldn't reproduce the "statistically unchanged" result above. On my machine both no-escape paths get slightly slower. Same comparison (master 6465716 with this PR's bench file vs. 211210e), pinned to a single core, Criterion defaults:

master baseline:
encode                         [14.533 ms 14.602 ms 14.669 ms]
encode_realistic_string_labels [15.154 ms 15.315 ms 15.504 ms]
encode_escaped_label_values    [14.823 ms 14.903 ms 14.983 ms]

this branch:
encode                         [14.798 ms 14.862 ms 14.925 ms]
change                         [+1.1590% +1.7811% +2.4310%], p<0.05
Performance has regressed.

encode_realistic_string_labels [16.305 ms 16.461 ms 16.627 ms]
change                         [+5.8716% +7.4811% +9.1025%], p<0.05
Performance has regressed.

encode_escaped_label_values    [18.364 ms 18.468 ms 18.583 ms]
change                         [+22.898% +23.924% +25.013%], p<0.05

Mean of medians over 3 interleaved runs per side, since single runs vary by a few percent:

Bench master this PR change
encode 14.78 ms 15.03 ms +1.7%
encode_realistic_string_labels 15.11 ms 15.83 ms +4.8%
encode_escaped_label_values 15.18 ms 18.29 ms +20.4%

Instruction counts per encode from cachegrind, which don't vary between runs, go up by 4.2%, 10.2% and 30.4%.

That's an acceptable cost for fixing the injection. The bigger follow-up win is caching the encoded histogram labels (the TODO in encode_labels), since they're currently re-encoded and re-checked on every bucket line.

Also, cargo test on 211210e runs 65 unit tests and 33 doc tests.

Host: aarch64 Linux VM on Apple Silicon, rustc 1.96.1.

Signed-off-by: Karthik Chowdary <karthikchowdary2001@gmail.com>

@krisztianfekete krisztianfekete left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for picking this up, the escaping logic looks correct.

Per the earlier discussion in #151, we need a benchmark to show escaping doesn't slow down encoding. I ran the existing text bench against this branch and it's about 12% slower than master (15.2 ms -> 17.1 ms), mostly on label values that don't need escaping at all.

Could you please:

  1. Add a fast path that writes the value unchanged when it contains no \, " or \n. A quick byte check at the top of write_str brought the regression down to about 5% locally. memchr::memchr3 may close the gap entirely, if you want to try it.
  2. Add a bench case to benches/encoding/text.rs whose label values need escaping, so both paths are measured.
  3. Paste the before/after criterion output into the PR description:
cargo bench --bench text -- --save-baseline master   # on master
cargo bench --bench text -- --baseline master        # on this branch

Avoid scanning label values twice when they do not contain escapable bytes, and benchmark both escaped and unescaped values.

Signed-off-by: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com>
@Karthik-Chowdary

Copy link
Copy Markdown
Contributor Author

Addressed the requested fast path and added a dedicated escaping benchmark in signed-off commit 79c8c88. Criterion output is now in the PR description; the common no-escape case shows no statistically significant change. I also fixed the benchmark harness to clear its retained buffer before each iteration so the comparison is stationary.

@krisztianfekete krisztianfekete left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the update, the escaping looks right and clearing the buffer in the bench was a good catch!

I dug into the numbers a bit more though. The existing bench only uses tiny label values (1–3 bytes), which hides the cost of the check. With more realistic labels like routes and pod names, encode ends up ~19% slower than master on my machine.

A few small changes get most of that back:

  • Swap any for a fold. With no early exit, LLVM vectorizes it, and a 32-byte value takes ~2ns to check instead of ~12ns:
    s.bytes().fold(false, |found, b| found | needs_escape(b))
  • Skip the check for integer/float/bool label values, since they can never need escaping. A small crate-private write_str_unescaped on LabelValueEncoder does the trick.
  • Move the escaping loop into a #[cold] fn and loop over bytes instead of chars (all three special chars are ASCII).
  • Add a quickcheck test against a simple reference escaper. Right now both tests still pass if the check forgets about \n.
  • Add a bench case with realistic string labels so we catch this kind of thing in future.

With those, I see about −2.5% on encode and +6% on string-heavy labels. That's fine for a fix like this I think. Happy to share the patch if it helps!

The bigger win would probably be caching the encoded labels for histograms (there's a TODO for it), since right now we re-encode and re-check them on every bucket line. This can be a separate PR though.

Signed-off-by: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com>
@Karthik-Chowdary

Copy link
Copy Markdown
Contributor Author

Implemented the follow-up performance and coverage suggestions in 211210e:

  • use a vectorizable fold for the escape check
  • bypass the check for integer, float, and boolean label values
  • move byte-oriented escaping into a cold path
  • compare against a reference escaper with 1,000 QuickCheck cases, including an explicit newline-only path
  • add a realistic pod-name string-label benchmark

Local validation passed: cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings, all 95 unit tests and 37 doc tests, and cargo bench --bench text --no-run. A short Criterion run completed successfully, but I am not treating the low-sample timings as authoritative.

Thanks for the detailed profiling and concrete suggestions.

@krisztianfekete

Copy link
Copy Markdown
Contributor

Thanks, can you please update the PR desc with the latest numbers?

@krisztianfekete
krisztianfekete added this pull request to the merge queue Oct 13, 2026
Merged via the queue into prometheus:master with commit 3403fcb Oct 13, 2026
10 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.

Bug - Label values are not escaped - Allows injection

2 participants