- Notifications
You must be signed in to change notification settings - Fork 805
Description
Description
PR #6853 changed error handling for DxcContainerBuilder::SerializeContainer
in a way that violates the DXC API pattern for HRESULT returns on functions that use IDxcOperationResult
to capture error outputs. The pattern is to return S_OK
if it was possible to produce the IDxcOperationResult
object which captures the output object, error code, and messages. If the return value is not S_OK
, the error is so fatal that it is not safe for the calling code to even dereference that interface pointer (which should be nullptr
anyway).
With the change, the interface function will now return a failing HRESULT when a validation failure occurs and it constructs a valid DxcResult
object. Calling code in dxc.cpp
was also updated to ignore the HRESULT returned from the API, which is also incorrect. I suspect this was done to retrieve the error messages now that the call fails under normal validation error circumstances due to the first change. Doing so could lead to dereferencing a nullptr
or accessing an invalid object if the error was truly fatal as the method's return value is supposed to indicate.
Additional related error handling issues with the change in this function:
- Returns
S_OK
ifppResult
isnullptr
or points tonullptr
. The first case should result inE_INVALIDARG
at the beginning of the function and the second case would be caught by a failingDxcResult::Create
already.S_OK
doesn't make sense here in any case. - Adds some code after the try/catch block to compute and apply the hash, but this code should be placed inside the try/catch before the construction of the final
DxcResult
object. - When computing the hash inside the try/catch, the
IDxcBlob pResult
should be used directly, rather than waiting until after theDxcResult
is created and reading it back from there. IsDxilContainerLike
returning false shouldn't be ignored, that's a fatal internal error indicating a corrupt container.
All the details are outlined in my comments on the PR here: #6853 (review)
Steps to Reproduce
This is a regression in fatal error handling code (such as hitting OOM) found by code inspection, for which a repro can't easily be constructed.
Environment
- DXC version: latest main
- Host Operating System: any
Metadata
Metadata
Assignees
Labels
Type
Projects
Status
Status