You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 if ppResult is nullptr or points to nullptr. The first case should result in E_INVALIDARG at the beginning of the function and the second case would be caught by a failing DxcResult::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 the DxcResult 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
The text was updated successfully, but these errors were encountered:
Description
PR #6853 changed error handling for
DxcContainerBuilder::SerializeContainer
in a way that violates the DXC API pattern for HRESULT returns on functions that useIDxcOperationResult
to capture error outputs. The pattern is to returnS_OK
if it was possible to produce theIDxcOperationResult
object which captures the output object, error code, and messages. If the return value is notS_OK
, the error is so fatal that it is not safe for the calling code to even dereference that interface pointer (which should benullptr
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 indxc.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 anullptr
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:
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.DxcResult
object.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
The text was updated successfully, but these errors were encountered: