googlefonts / googlefonts/fontations

Soundness of read_ref_unchecked

Open
#436 15 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
826
Forks
75
Avg merge
22h 33m
Merged PRs (30d)
75

Description

[`read_ref_unchecked`](https://github.com/googlefonts/fontations/blob/ea58ee9f6262f85ce84ca4fad2289f2614066b97/read-fonts/src/font_data.rs#L128C15-L131) will create a reference into a memory buffer, changing the representation from &[u8] to &T.

The number of bytes used for this &[u8] is T::RAW_BYTE_LEN which is [defined to be](https://github.com/googlefonts/fontations/blob/ea58ee9f6262f85ce84ca4fad2289f2614066b97/font-types/src/raw.rs#L21C9-L32) the number of bytes required to represent the type in a font file, which may be smaller than the size of the native type when the native type has tail padding (for alignment purposes).

There are 2 possible problematic outcomes:

## Overlapping refs

Two consecutive T elements in the buffer may overlap in memory. Let's say T has `size_of` equal to 2, but T::RAW_BYTE_LEN is 1.

```
| T1 | T2 | T3 | ... |
```

- `read_ref_unchecked(0)` will return `&T` that points to `| T1 | T2 |`.
- `read_ref_unchecked(1)` will return `&T` that points to `| T2 | T3 |`.

If we were to `*read_ref_unchecked(0) = another_t`, we'd expect `size_of` bytes to be memcpy'd which would clobber `T2`.

This is happily mitigated by the fact that there *is no mutable reference version* of this function. I think we should document why there is no mutable reference version somewhere in the code (rustdocs) so that one does not appear in the future.

## Invalid refs

The last T element in the buffer actually goes off the end of the buffer. Again, let's say T has `size_of` equal to 2, but T::RAW_BYTE_LEN is 1.

```
| T1 | T2 | ... | T9 |
```

A call to `read_ref_unchecked(8)` will return `&T` that points to `| T9 | invalid memory |`. If that `&T` is moved (which it can't be, see [Overlapping refs](#overlapping-refs)) or copied, we'd see two bytes of memory copied, which would involve a read out of bounds, which is a memory safety bug.

I _think_ this could be mitigated by requiring T to not be `Copy`, or by requiring `size_of() - T::RAW_BYTE_LEN` bytes of padding to exist at the back of the buffer?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.