Skip to content

Regression in ContainerBuilder error handling code introduced by #6853 #7051

@tex3d

Description

@tex3d

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

Metadata

Metadata

Assignees

Labels

bugBug, regression, crash

Type

No type

Projects

Status

Done

Status

Triaged

Relationships

None yet

Development

No branches or pull requests

Issue actions