Do not feed &"" to D3DCompile

[workingjubilee]

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:

• https://learn.microsoft.com/en-us/windows/win32/api/d3dcompiler/nf-d3dcompiler-d3dcompile
• Bevy segfaults on some Windows systems after Rust 1.79 update rust-lang/rust#126442

[workingjubilee]

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.

[teoxoy]

Thanks for the fix! We should backport this.

[teoxoy]

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]

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

[jimblandy]

@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]

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]

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

[RalfJung]

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]

With #5836 compile_fxc has been made correct.

[jimblandy]

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]

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]

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.