On Tue Apr 11 07:23:35 2023 +0000, Henri Verbeet wrote:
- if (!data || !data_size || flags || !blob) + if (!data || data_size < DXBC_HEADER_SIZE || flags || !blob)Ideally this would be a separate change with a test to validate its
correctness (AFAICS it is correct but there isn't a test already for it). Well, it more or less preserves existing behaviour. Previously this case would simply read beyond "data_size" and (typically) either hit the "data_size != total_size" condition in dxbc_parse() and return D3DERR_INVALIDCALL, or hit the "tag != TAG_DXBC" condition and return E_FAIL. None of that is particularly great, of course. In vkd3d-shader's parse_dxbc(), we instead hit the "data_size < VKD3D_DXBC_HEADER_SIZE" condition, which returns VKD3D_ERROR_INVALID_ARGUMENT. That's the same error as returned for the "tag != TAG_DXBC" case. None of this seems particularly worth worrying about on first sight, but in the case of D3DStripShader() we happen to have a test that hits these paths. (I.e., the earlier "blob.c:364: Test failed: Got unexpected hr 0x80004005." failure.) There are a couple of different ways we could address this of course, including:
- Explicitly check the tag in d3dcompiler_strip_shader().
- Explicitly check the size in d3dcompiler_strip_shader().
- Return a different error for these two cases in vkd3d-shader.
- Remove the offending test.
These aren't mutually exclusive options, of course. I chose the second option because it seemed the easiest, particularly because there are already existing "data_size" checks in d3dcompiler_strip_shader() and d3dcompiler_get_blob_part(), but I could certainly be convinced to take a different approach. The d3dcompiler_get_blob_part() change is just intended to keep the behaviour consistent with d3dcompiler_strip_shader(), which generally seems desirable, but in principle it could be dropped.
- /* some parts aren't full DXBCs, they contain only the data */ + /* some parts aren't full DXBCs, they contain only the data. */Inane nitpick: this change could be moved 2 patches back where nearby
code is already being touched. Actually, I didn't intend to touch that line at all; I've dropped it in v4.
Similar to the previous patch here. Also can get rid of the ||
!data_size part. Right, not removing the "!data_size" check was an oversight; I've dropped it in v4. For the DXBC_HEADER_SIZE check, see the explanation above.
Right, I guess I'm saying that I'd slightly prefer if the problematic behavior was fixed beforehand, introducing these minimum size checks (both to d3dcompiler_get_blob_part() and d3dcompiler_strip_shader()) in a separate patch. It could be a new "patch 1" for this MR.
Think of it as a bonus fix that you get to do simply because you happened to be the first to spot it :D
Even better if the patch comes with tests (it doesn't need to be anything fancy, something like my earlier attachment would do just fine), especially for D3DGetBlobPart(), given that it doesn't happen to have anything affected by the specific behavior, or the fix, right now.