Do not feed &"" to D3DCompile #5812

workingjubilee · 2024-06-14T06:36:37Z

A recent change by rustc, now in 1.79-stable, makes empty str constants point to the same location: 0x01. This is an optimization of sorts, not stable behavior. Code must not rely on constants having stable addresses nor should it pass &"" to APIs expecting CStrs or NULL addresses. D3DCompile will segfault if you give it such a pointer, or worse: read random garbage addresses!

Pass the NULL pointer to D3DCompile if wgpu lacks a decent CString.
For {d,f}xc_compile, accept source_name: Option<&CStr>, as otherwise the API must be unsafe.

refs:

A recent change by rustc, now in 1.79-stable, makes empty str constants point to the same location: 0x01. This is an optimization of sorts, not stable behavior. Code must not rely on constants having stable addresses nor should it pass &"" to APIs expecting CStrs or NULL addresses. D3DCompile will segfault if you give it such a pointer, or worse: read random garbage addresses! Pass the NULL pointer to D3DCompile if wgpu lacks a decent CString. refs: - https://learn.microsoft.com/en-us/windows/win32/api/d3dcompiler/nf-d3dcompiler-d3dcompile Co-authored-by: Jan Hohenheim <[email protected]> Co-authored-by: Brezak <[email protected]>

workingjubilee · 2024-06-14T06:38:13Z

I had originally intended to test this more thoroughly but I stopped when I discovered I had to fuck around with patching multiple libraries, not just the Bevy monorepo, just to get the Bevy example in question to build.

wgpu-hal/src/dx12/shader_compilation.rs

teoxoy · 2024-06-14T12:03:47Z

Thanks for the fix! We should backport this.

teoxoy · 2024-06-14T12:24:14Z

Something else I noticed while looking at this is that pSrcData is documented as:

A pointer to uncompiled shader data; either ASCII HLSL code or a compiled effect.

but we pass it UTF-8.

pSourceName, pEntrypoint and pTarget are also LPCSTR which is documented as:

A pointer to a constant null-terminated string of 8-bit Windows (ANSI) characters. For more information, see Character Sets Used By Fonts.

There is also this page which says you can change the default code page to UTF-8 and therefore rely on LPCSTR being UTF-8 but that's only possible for apps/binaries as far as I understand.

We can probably assume using ASCII for those is safe. Microsoft's docs don't seem to guarantee that those code pages are extended ASCII; Wikipedia does though, maybe they went through all the listed ones and checked that they are.

I will open a new issue to track this.

torokati44 · 2024-06-17T21:14:51Z

A new release with this fix would be greatly appreciated! 🙏

A recent change by rustc, now in 1.79-stable, makes empty str constants point to the same location: 0x01. This is an optimization of sorts, not stable behavior. Code must not rely on constants having stable addresses nor should it pass &"" to APIs expecting CStrs or NULL addresses. D3DCompile will segfault if you give it such a pointer, or worse: read random garbage addresses! Pass the NULL pointer to D3DCompile if wgpu lacks a decent CString. refs: - https://learn.microsoft.com/en-us/windows/win32/api/d3dcompiler/nf-d3dcompiler-d3dcompile Co-authored-by: Jan Hohenheim <[email protected]> Co-authored-by: Brezak <[email protected]>

jimblandy · 2024-06-17T22:23:59Z

@glandium The arguments to D3DCompile are (per docs):

HRESULT D3DCompile(
  [in]            LPCVOID                pSrcData,
  [in]            SIZE_T                 SrcDataSize,
  [in, optional]  LPCSTR                 pSourceName,
  [in, optional]  const D3D_SHADER_MACRO *pDefines,
  [in, optional]  ID3DInclude            *pInclude,
  [in, optional]  LPCSTR                 pEntrypoint,
  [in]            LPCSTR                 pTarget,
  [in]            UINT                   Flags1,
  [in]            UINT                   Flags2,
  [out]           ID3DBlob               **ppCode,
  [out, optional] ID3DBlob               **ppErrorMsgs
);

So the textual arguments are as follows:

The source code is passed via pSrcData and SrcDataSize as a pointer and byte length. This doesn't seem to require NUL termination.
The filename pSourceName is a nul-terminated C string, which has been a CString for a long time.
The entry point name pEntryPoint is constructed via CString::new, so it's NUL-terminated.
The pTarget is constructed from a String whose creator has added an explicit \0 character, which is gross but not wrong.

So it seems to me that all cases are now addressed. What specifically do you think is still wrong?

glandium-moz · 2024-06-17T22:29:00Z

The filename pSourceName is a nul-terminated C string, which has been a CString for a long time.

It was, in fact, changed to a CString in this PR. I had missed it.

The pTarget is constructed from a String whose creator has added an explicit \0 character, which is gross but not wrong.

I must say I didn't look at callers to see what they were doing.

workingjubilee · 2024-06-17T22:38:20Z

Hm, the full_stage should also be changed to be &CStr then.

RalfJung · 2024-06-18T06:13:30Z

The pTarget is constructed from a String whose creator has added an explicit \0 character, which is gross but not wrong.

That's still unsound in compile_fxc: the function relies on the caller to uphold certain invariants, or else there is UB. As this is a private function, the unsoundness is contained within the crate, but it is still generally considered a bug to have a private unsound function. You're needlessly giving up on having rustc help you with tracking the soundness requirements.

workingjubilee · 2024-06-18T08:54:24Z

With #5836 compile_fxc has been made correct.

jimblandy · 2024-06-18T16:12:59Z

You're needlessly giving up on having rustc help you with tracking the soundness requirements.

I agree with all this - I didn't mean to suggest otherwise. I'm glad to see #5836.

jimblandy · 2024-06-18T16:15:30Z

It was, in fact, changed to a CString in this PR. I had missed it.

Again, I was looking at the actual origin of the data, which is this:

0a609e74c0 (Dzmitry Malyshau      2021-07-11 573) #[derive(Debug)]
0a609e74c0 (Dzmitry Malyshau      2021-07-11 574) pub struct ShaderModule {
0a609e74c0 (Dzmitry Malyshau      2021-07-11 575)     naga: crate::NagaShader,
726cf4f614 (Dzmitry Malyshau      2021-07-23 576)     raw_name: Option<ffi::CString>,
0a609e74c0 (Dzmitry Malyshau      2021-07-11 577) }

But yes, we agree that this PR fixed the actual value being passed to D3DCompile.

jimblandy · 2024-06-18T16:40:35Z

Since I may have been misunderstood:

I think using terms like "morally wrong" for code like the original compile_fxc is fine as a joke - I'm sure I say stuff like that too, because I do think it's funny - but it may foster the impression that using argument types that properly capture the function's expectations is a matter of principle or style. The truth is that we have so much work to do in wgpu that we simply cannot afford the kind of distraction that comes from weird unstated safety contracts between functions and their callers.

A single instance of this kind of problem is not a big deal. A Rust update makes Bevy crash? Fine, we debug it, fix it, and move on. But if we make a practice of tolerating unsound code, we'll have not one but a steady stream of such distractions. And that will be a disaster, because wgpu will not be able to keep up with its users' expectations.

Properly used Rust is a huge improvement in development velocity, which we as a project can't afford to give up. People doing code review for wgpu should reject PRs that introduce unsoundness like that addressed in this PR and #5836, even when the code runs correctly.

A recent change by rustc, now in 1.79-stable, makes empty str constants point to the same location: 0x01. This is an optimization of sorts, not stable behavior. Code must not rely on constants having stable addresses nor should it pass &"" to APIs expecting CStrs or NULL addresses. D3DCompile will segfault if you give it such a pointer, or worse: read random garbage addresses! Pass the NULL pointer to D3DCompile if wgpu lacks a decent CString. refs: - https://learn.microsoft.com/en-us/windows/win32/api/d3dcompiler/nf-d3dcompiler-d3dcompile Co-authored-by: Jubilee <46493976 [email protected]> Co-authored-by: Jan Hohenheim <[email protected]> Co-authored-by: Brezak <[email protected]>

Permalink: <gfx-rs#5812>

Permalink: <#5812>

- Revert #13834 once wgpu released the fix for gfx-rs/wgpu#5812

- Revert bevyengine#13834 once wgpu released the fix for gfx-rs/wgpu#5812

A recent change by rustc, now in 1.79-stable, makes empty str constants point to the same location: 0x01. This is an optimization of sorts, not stable behavior. Code must not rely on constants having stable addresses nor should it pass &"" to APIs expecting CStrs or NULL addresses. D3DCompile will segfault if you give it such a pointer, or worse: read random garbage addresses! Pass the NULL pointer to D3DCompile if wgpu lacks a decent CString. refs: - https://learn.microsoft.com/en-us/windows/win32/api/d3dcompiler/nf-d3dcompiler-d3dcompile Co-authored-by: Jan Hohenheim <[email protected]> Co-authored-by: Brezak <[email protected]>

Permalinks: * <gfx-rs#5812> * <gfx-rs#5833>

This also updates `wgpu-core` and `wgpu-hal` to the corresponding versions (which are 0.21.x). This fixes some undefined behavior when building with Rust 1.79 and using the Direct3D 12 backend: gfx-rs/wgpu#5812 It also fixes an issue on OpenGL where non-sRGB was still using sRGB: gfx-rs/wgpu#5642

* docs(CHANGELOG): add entry for #5812/#5833 Permalinks: * <#5812> * <#5833> * chore: `wgpu-hal` 0.19.5 release * docs(CHANGELOG): add `Unreleased` again

This also updates `wgpu-core` and `wgpu-hal` to the corresponding versions (which are 0.21.x). This fixes some undefined behavior when building with Rust 1.79 and using the Direct3D 12 backend: gfx-rs/wgpu#5812 It also fixes an issue on OpenGL where non-sRGB was still using sRGB: gfx-rs/wgpu#5642

workingjubilee requested a review from a team as a code owner June 14, 2024 06:36

RalfJung reviewed Jun 14, 2024

View reviewed changes

wgpu-hal/src/dx12/shader_compilation.rs Outdated Show resolved Hide resolved

workingjubilee force-pushed the fix-bad-ptr-in-compile_fxc branch from 7c5b14d to 2ae705b Compare June 14, 2024 07:16

RalfJung mentioned this pull request Jun 14, 2024

Bevy segfaults on some Windows systems after Rust 1.79 update rust-lang/rust#126442

Closed

workingjubilee force-pushed the fix-bad-ptr-in-compile_fxc branch 3 times, most recently from 7bd82b5 to 525364d Compare June 14, 2024 09:58

msvbg reviewed Jun 14, 2024

View reviewed changes

wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved

workingjubilee force-pushed the fix-bad-ptr-in-compile_fxc branch from 525364d to b0f603f Compare June 14, 2024 10:49

Alter compile_{d,f}xc to take Option<&CStr>

b0f603f

teoxoy approved these changes Jun 14, 2024

View reviewed changes

teoxoy added the PR: needs back-porting PR with a fix that needs to land on crates label Jun 14, 2024

teoxoy merged commit d309bba into gfx-rs:trunk Jun 14, 2024
25 checks passed

teoxoy mentioned this pull request Jun 14, 2024

FXC requires ASCII HLSL code #5814

Open

workingjubilee deleted the fix-bad-ptr-in-compile_fxc branch June 14, 2024 16:57

This was referenced Jun 14, 2024

STATUS_ACCESS_VIOLATION in CorePipelinePlugin bevyengine/bevy#13559

Closed

run windows ci on rust 1.78 bevyengine/bevy#13834

Merged

mockersf mentioned this pull request Jun 15, 2024

Windows CI example runner: back to using rust stable bevyengine/bevy#13863

Merged

torokati44 mentioned this pull request Jun 17, 2024

Backport Do not feed &"" to D3DCompile (#5812) to 0.20 #5831

Merged

This was referenced Jun 17, 2024

Backport Do not feed &"" to D3DCompile (#5812) to 0.19 #5833

Merged

Backport Do not feed &"" to D3DCompile (#5812) to 0.18 #5834

Closed

workingjubilee mentioned this pull request Jun 18, 2024

Make compile_fxc accept full_stage: &CStr #5836

Merged

ErichDonGubler added a commit to erichdongubler-mozilla/wgpu that referenced this pull request Jun 20, 2024

docs(CHANGELOG): add entry for gfx-rs#5812

fd84991

Permalink: <gfx-rs#5812>

ErichDonGubler mentioned this pull request Jun 20, 2024

docs(CHANGELOG): add entry for #5812 #5851

Merged

ErichDonGubler added a commit to erichdongubler-mozilla/wgpu that referenced this pull request Jun 22, 2024

docs(CHANGELOG): add entry for gfx-rs#5812

a4527b4

Permalink: <gfx-rs#5812>

ErichDonGubler added a commit to erichdongubler-mozilla/wgpu that referenced this pull request Jun 22, 2024

docs(CHANGELOG): add entry for gfx-rs#5812

23e9f42

Permalink: <gfx-rs#5812>

jimblandy pushed a commit to erichdongubler-mozilla/wgpu that referenced this pull request Jun 23, 2024

docs(CHANGELOG): add entry for gfx-rs#5812

3d0fbe7

Permalink: <gfx-rs#5812>

jimblandy pushed a commit that referenced this pull request Jun 23, 2024

docs(CHANGELOG): add entry for #5812

f227ca1

Permalink: <#5812>

github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Jun 27, 2024

Windows CI example runner: back to using rust stable (#13863)

63fd8fb

- Revert #13834 once wgpu released the fix for gfx-rs/wgpu#5812

zmbush pushed a commit to zmbush/bevy that referenced this pull request Jul 3, 2024

Windows CI example runner: back to using rust stable (bevyengine#13863)

275e2eb

- Revert bevyengine#13834 once wgpu released the fix for gfx-rs/wgpu#5812

ErichDonGubler added a commit to erichdongubler-mozilla/wgpu that referenced this pull request Jul 16, 2024

docs(CHANGELOG): add entry for gfx-rs#5812/gfx-rs#5833

1a874f2

Permalinks: * <gfx-rs#5812> * <gfx-rs#5833>

waywardmonkeys mentioned this pull request Jul 16, 2024

Update to wgpu 0.20.1 linebender/vello#631

Merged

cwfitzgerald pushed a commit that referenced this pull request Jul 16, 2024

v0.19.5 (#5963)

cab36d8

* docs(CHANGELOG): add entry for #5812/#5833 Permalinks: * <#5812> * <#5833> * chore: `wgpu-hal` 0.19.5 release * docs(CHANGELOG): add `Unreleased` again

Wumpf removed the PR: needs back-porting PR with a fix that needs to land on crates label Jul 24, 2024

workingjubilee mentioned this pull request Jul 27, 2024

zero-sized slices are sometimes placed at 0x1 (1.79 ) rust-lang/rust#128257

Closed

kylewillmon mentioned this pull request Aug 6, 2024

Dependency update not possible phylum-dev/cli#1478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not feed &"" to D3DCompile #5812

Do not feed &"" to D3DCompile #5812

workingjubilee commented Jun 14, 2024 •

edited

Loading

workingjubilee commented Jun 14, 2024

teoxoy commented Jun 14, 2024

teoxoy commented Jun 14, 2024

torokati44 commented Jun 17, 2024

jimblandy commented Jun 17, 2024

glandium-moz commented Jun 17, 2024

workingjubilee commented Jun 17, 2024

RalfJung commented Jun 18, 2024

workingjubilee commented Jun 18, 2024

jimblandy commented Jun 18, 2024

jimblandy commented Jun 18, 2024 •

edited

Loading

jimblandy commented Jun 18, 2024

Do not feed &"" to D3DCompile #5812

Do not feed &"" to D3DCompile #5812

Conversation

workingjubilee commented Jun 14, 2024 • edited Loading

workingjubilee commented Jun 14, 2024

teoxoy commented Jun 14, 2024

teoxoy commented Jun 14, 2024

torokati44 commented Jun 17, 2024

jimblandy commented Jun 17, 2024

glandium-moz commented Jun 17, 2024

workingjubilee commented Jun 17, 2024

RalfJung commented Jun 18, 2024

workingjubilee commented Jun 18, 2024

jimblandy commented Jun 18, 2024

jimblandy commented Jun 18, 2024 • edited Loading

jimblandy commented Jun 18, 2024

workingjubilee commented Jun 14, 2024 •

edited

Loading

jimblandy commented Jun 18, 2024 •

edited

Loading