Skip to content

Commit 89e8a4b

Browse files
authored
gpui_windows: Harden DirectWrite and DirectX text rendering (#60124)
This hardens the Windows text rendering path in gpui_windows against undefined behavior and out-of-bounds access. DirectWrite hands back glyph-run arrays (glyph indices, advances, offsets) and cluster maps as raw pointers that may be null; these are now interpreted through a shared `slice_from_nullable` helper that treats a null pointer with zero length as an empty slice and returns `E_INVALIDARG` when a null pointer arrives with a nonzero length, instead of constructing a slice from a null pointer. It also fixes a provenance bug: the renderer context passed to `IDWriteTextLayout::Draw` was derived via `&raw const` from a non-mut binding, but the `DrawGlyphRun` callback recovers that pointer and writes through it, which is undefined behavior under Stacked/Tree Borrows; the binding is now `mut` and the pointer is derived from `&raw mut` so its provenance matches the write (codegen is unchanged). On the DirectX side, the atlas upload is now bounded by the source slice length so a mismatched destination region cannot read past the end of the source, and the color-glyph staging texture is unmapped after readback so the mapping is no longer leaked and the resource pinned. The color-glyph rasterizer drives the shared, non-thread-safe D3D11 immediate context that `DirectXRenderer` and `DirectXAtlas` also use, so it must stay on the main UI thread — which it always is today; a comment documents that invariant. Since gpui_windows is a Windows-only crate, it cannot be compiled on non-Windows hosts, so Windows CI is the compile gate for these changes. Release Notes: - N/A
1 parent 715557f commit 89e8a4b

2 files changed

Lines changed: 79 additions & 9 deletions

File tree

crates/gpui_windows/src/direct_write.rs

Lines changed: 61 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -608,15 +608,15 @@ impl DirectWriteState {
608608
}
609609

610610
let mut runs = Vec::new();
611-
let renderer_context = RendererContext {
611+
let mut renderer_context = RendererContext {
612612
text_system: self,
613613
components,
614614
index_converter: StringIndexConverter::new(text),
615615
runs: &mut runs,
616616
width: 0.0,
617617
};
618618
text_layout.Draw(
619-
Some((&raw const renderer_context).cast::<c_void>()),
619+
Some((&raw mut renderer_context).cast::<c_void>().cast_const()),
620620
&components.text_renderer.0,
621621
0.0,
622622
0.0,
@@ -881,6 +881,10 @@ impl DirectWriteState {
881881
params: &RenderGlyphParams,
882882
glyph_bounds: Bounds<DevicePixels>,
883883
) -> Result<Vec<u8>> {
884+
// INVARIANT: the code below drives the *shared* D3D11 immediate context
885+
// (`Map`/`Unmap`/`Draw`/`CopyResource`), which `DirectXRenderer` and `DirectXAtlas` also
886+
// touch. An immediate `ID3D11DeviceContext` is not thread-safe, so this must only run on
887+
// the main UI thread (which it always is; text rasterization never leaves that thread).
884888
let bitmap_size = glyph_bounds.size;
885889
let subpixel_shift = params
886890
.subpixel_variant
@@ -1174,6 +1178,10 @@ impl DirectWriteState {
11741178
};
11751179
}
11761180

1181+
// Release the mapping now that the rows have been copied out; leaving `staging_texture`
1182+
// mapped would leak the mapping and keep the resource pinned for later reuse.
1183+
unsafe { device_context.Unmap(&staging_texture, 0) };
1184+
11771185
// Convert from premultiplied to straight alpha
11781186
for chunk in rasterized.chunks_exact_mut(4) {
11791187
let b = chunk[0] as f32;
@@ -1519,13 +1527,34 @@ impl IDWriteTextRenderer_Impl for TextRenderer_Impl {
15191527

15201528
let color_font = unsafe { font_face.IsColorFont().as_bool() };
15211529

1522-
let glyph_ids = unsafe { std::slice::from_raw_parts(glyphrun.glyphIndices, glyph_count) };
1523-
let glyph_advances =
1524-
unsafe { std::slice::from_raw_parts(glyphrun.glyphAdvances, glyph_count) };
1525-
let glyph_offsets =
1526-
unsafe { std::slice::from_raw_parts(glyphrun.glyphOffsets, glyph_count) };
1527-
let cluster_map =
1528-
unsafe { std::slice::from_raw_parts(desc.clusterMap, desc.stringLength as usize) };
1530+
let glyph_ids = unsafe {
1531+
slice_from_nullable(
1532+
glyphrun.glyphIndices,
1533+
glyph_count,
1534+
"DirectWrite returned a null glyph indices array",
1535+
)?
1536+
};
1537+
let glyph_advances = unsafe {
1538+
slice_from_nullable(
1539+
glyphrun.glyphAdvances,
1540+
glyph_count,
1541+
"DirectWrite returned a null glyph advances array",
1542+
)?
1543+
};
1544+
let glyph_offsets = unsafe {
1545+
slice_from_nullable(
1546+
glyphrun.glyphOffsets,
1547+
glyph_count,
1548+
"DirectWrite returned a null glyph offsets array",
1549+
)?
1550+
};
1551+
let cluster_map = unsafe {
1552+
slice_from_nullable(
1553+
desc.clusterMap,
1554+
desc.stringLength as usize,
1555+
"DirectWrite returned a null cluster map",
1556+
)?
1557+
};
15291558

15301559
let cluster_analyzer = ClusterAnalyzer::new(cluster_map, glyph_count);
15311560
let mut utf16_idx = desc.textPosition as usize;
@@ -1605,6 +1634,29 @@ impl IDWriteTextRenderer_Impl for TextRenderer_Impl {
16051634
}
16061635
}
16071636

1637+
/// Interprets an optional DirectWrite array pointer as a slice, treating a
1638+
/// null pointer with a zero length as an empty slice. A null pointer with a
1639+
/// nonzero length fails with `null_error_message`.
1640+
///
1641+
/// # Safety
1642+
///
1643+
/// When `ptr` is non-null, the caller must guarantee that it points to a valid
1644+
/// array of at least `len` elements that outlives the returned slice.
1645+
unsafe fn slice_from_nullable<'a, T>(
1646+
ptr: *const T,
1647+
len: usize,
1648+
null_error_message: &str,
1649+
) -> windows::core::Result<&'a [T]> {
1650+
if ptr.is_null() {
1651+
if len != 0 {
1652+
return Err(Error::new(E_INVALIDARG, null_error_message));
1653+
}
1654+
Ok(&[])
1655+
} else {
1656+
Ok(unsafe { std::slice::from_raw_parts(ptr, len) })
1657+
}
1658+
}
1659+
16081660
struct StringIndexConverter<'a> {
16091661
text: &'a str,
16101662
utf8_ix: usize,

crates/gpui_windows/src/directx_atlas.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,24 @@ impl DirectXAtlasTexture {
282282
bounds: Bounds<DevicePixels>,
283283
bytes: &[u8],
284284
) {
285+
// `UpdateSubresource` reads `row_pitch * height` bytes from `bytes` based on the
286+
// `D3D11_BOX` below. If the caller hands us a slice shorter than that, the driver would
287+
// over-read past the end of the source buffer (potentially by multiple megabytes), so bail
288+
// out instead. This is a first-insert path rather than a per-frame one, so the check is
289+
// effectively free.
290+
let row_bytes = bounds.size.width.to_bytes(self.bytes_per_pixel as u8) as usize;
291+
let expected = row_bytes * bounds.size.height.0.max(0) as usize;
292+
if bytes.len() < expected {
293+
log::error!(
294+
"DirectXAtlasTexture::upload: source slice is {} bytes but the {}x{} region \
295+
requires {} bytes; skipping upload to avoid a driver over-read",
296+
bytes.len(),
297+
bounds.size.width.0,
298+
bounds.size.height.0,
299+
expected,
300+
);
301+
return;
302+
}
285303
unsafe {
286304
device_context.UpdateSubresource(
287305
&self.texture,

0 commit comments

Comments
 (0)