grafana / grafana/pyroscope-python
py-spy copy_string fabricates Rust chars from target bytes (UB; yields Strings with invalid UTF-8)
- Lenguaje dominante
- Rust
- Estrellas
- 4
- Forks
- 2
- Merge medio
- 2 d 4 h
- PR fusionados (30 d)
- 5
Descripción
> 🤖 **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.
Guía de contribución
No hay ninguna guía de contribución indexada para este repositorio
Línea de trabajo
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.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- rust
- Área
- devtools
- Tipo de issue
- Error
- Dificultad
- 3/5
- Tiempo estimado
- 1-2 días
- Estado de actividad
- Activo
- Claridad
- Bien especificado
- Aptitud para principiantes
- 68/100