Skip to content

fix: escape text format label values - #347

Open
Karthik-Chowdary wants to merge 3 commits into
prometheus:masterfrom
Karthik-Chowdary:fix/346-escape-label-values
Open

Karthik-Chowdary wants to merge 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

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

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

@krisztianfekete krisztianfekete 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.

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
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
Collaborator

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
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.

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