Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tex3d opened this issue Jan 8, 2025 · 0 comments · Fixed by #7056
Closed

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

tex3d opened this issue Jan 8, 2025 · 0 comments · Fixed by #7056
Assignees
Labels
bug Bug, regression, crash

Comments

@tex3d
Copy link
Contributor

tex3d commented Jan 8, 2025

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
@tex3d tex3d added bug Bug, regression, crash needs-triage Awaiting triage labels Jan 8, 2025
@damyanp damyanp removed the needs-triage Awaiting triage label Jan 9, 2025
@damyanp damyanp added this to the Release 1.8.2502 milestone Jan 9, 2025
@damyanp damyanp moved this to Triaged in HLSL Triage Jan 9, 2025
@damyanp damyanp moved this to Planning in HLSL Support Jan 9, 2025
@davidcook-msft davidcook-msft moved this from Planning to Needs Review in HLSL Support Jan 14, 2025
@github-project-automation github-project-automation bot moved this from New to Done in HLSL Roadmap Jan 30, 2025
@github-project-automation github-project-automation bot moved this from Needs Review to Closed in HLSL Support Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash
Projects
Status: Done
Status: Triaged
Development

Successfully merging a pull request may close this issue.

3 participants