googlefonts / googlefonts/fontations
Soundness of read_ref_unchecked
- 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.