grafana / grafana/pyroscope-python

py-spy copy_string fabricates Rust chars from target bytes (UB; yields Strings with invalid UTF-8)

Offen
#147 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
Rust
Sterne
4
Forks
2
Ø Merge
2 T. 4 Std.
Gemergte PRs (30 T.)
5

Beschreibung

> 🤖 **This issue was written by an AI agent** (Claude Code), while investigating
> #37. The code references, sizes and program output below were verified against
> the actual sources and binaries, but a human has not reviewed the writeup.

## What

py-spy's `copy_string` builds Rust `char` values directly out of bytes read from
the profiled process (`py-spy/src/python_data_access.rs`, the `kind == 4` /
UCS-4 branch):

```rust
let bytes = process.copy(obj.address(ptr as usize), obj.size() * kind as usize)?;

match (kind, obj.ascii()) {
(4, _) => {
#[allow(clippy::cast_ptr_alignment)]
let chars = unsafe {
std::slice::from_raw_parts(bytes.as_ptr() as *const char, bytes.len() / 4)
};
Ok(chars.iter().collect())
}
...
```

A Rust `char` must be a Unicode scalar value (`<= 0x10FFFF`, no surrogates).
Materializing one from arbitrary bytes is undefined behaviour, and these bytes
are arbitrary by nature: the profiler samples a live, unsuspended interpreter,
so a stale or torn read returns whatever is in that memory now.

The other branches are fine — `(2, _)` uses the checked `String::from_utf16`,
`(1, true)` uses `String::from_utf8`, `(1, false)` maps bytes as latin-1 — it is
only the UCS-4 path that fabricates `char`s.

## Why it matters

Besides being UB on its own, the concrete observable result is a `String` whose
bytes are **not valid UTF-8**, i.e. the `str` invariant is broken for a value
that then travels through the profiler and into the uploaded profile
(frame names and filenames). Reproduced with the exact expression from
`copy_string`:

```rust
fn copy_string_kind4(bytes: &[u8]) -> String {
let chars = unsafe { slice::from_raw_parts(bytes.as_ptr() as *const char, bytes.len() / 4) };
chars.iter().collect()
}
// input words: 0xFFFFFFFF, 0x00110000, 0x0000D800, 0x7FFFFFFF, 0x00000041
```

```
len = 16
bytes = [ff, bf, bf, bf, f4, 90, 80, 80, ed, a0, 80, ff, bf, bf, bf, 41]
is valid utf-8 as a str? false
from_utf8 error: invalid utf-8 sequence of 1 bytes from index 0
```
(rustc 1.98.0)

`ff bf bf bf` is not a valid leading byte, `f4 90 80 80` encodes above
U+10FFFF, and `ed a0 80` is a CESU-8 surrogate. Any downstream code that
relies on the invariant — char-boundary slicing, `format!` width/precision,
anything doing unchecked UTF-8 arithmetic — is then operating on an invalid
`str`.

The path is genuinely reached in practice: CPython stores strings containing
non-BMP characters as UCS-4, so function names and filenames with astral
characters go through it on every sample. A workload using Deseret identifiers
and emoji filenames produced 16192 UCS-4-decoded sequences in a single uploaded
profile.

## What this is *not*

This is **not** the cause of #37 — that turned out to be a niche/discriminant
confusion in `copy_struct` (see #37 and grafana/pyroscope-python#146). Feeding
this path non-scalar values on every sample for 25 s did not crash
(`repro37/poc_hostile_string.py` in that PR). So: real UB with an observable
broken invariant, but no crash attributed to it so far.

## Suggested fix

Validate instead of transmuting, e.g.

```rust
(4, _) => bytes
.chunks_exact(4)
.map(|c| u32::from_le_bytes(c.try_into().unwrap()))
.map(|w| char::from_u32(w).ok_or_else(|| format_err!("invalid scalar value {w:#x}")))
.collect::>(),
```

or use `char::REPLACEMENT_CHARACTER` for out-of-range words if a garbage read
should degrade rather than error. Either way the `unsafe` block goes away.

Upstream: this code lives in `benfred/py-spy`, so a fix there plus a pin bump
here.

Beitragsleitfaden

Für dieses Repository ist kein Beitragsleitfaden indexiert

Rechercherichtung

Start in py-spy/src/python_data_access.rs at the copy_string kind == 4 branch and review how its result reaches profile frame names and filenames. Reproduce the reported invalid-value cases, then verify that the upstream fix removes the unsafe conversion and that pyroscope-python uses the updated py-spy pin.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
rust
Bereich
devtools
Issue-Typ
Bug
Schwierigkeit
3/5
Geschätzter Aufwand
1-2 Tage
Aktivitätsstatus
Aktiv
Klarheit
Klar beschrieben
Anfängerfreundlichkeit
68/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.