Skip to content

fix: Avoid reading unmapped memory in FastTextRenderer - #3376

Open
BenjaBobs wants to merge 1 commit into
stride3d:masterfrom
BenjaBobs:fix/fast-text-renderer-vulkan-unmap
Open

fix: Avoid reading unmapped memory in FastTextRenderer#3376
BenjaBobs wants to merge 1 commit into
stride3d:masterfrom
BenjaBobs:fix/fast-text-renderer-vulkan-unmap

Conversation

@BenjaBobs

Copy link
Copy Markdown

PR Details

FastTextRenderer.Initialize retained a pointer to mapped index-buffer memory and passed it to Buffer.Index.New after unmapping the buffer.
On Linux/Vulkan the pointer was no longer valid, which caused an AccessViolationException when Buffer.Index.New tried to copy the index data.
This change builds the indices in a managed int[] and passes that array directly to Buffer.Index.New.
This also removes the temporary graphics buffer and the now-unnecessary unsafe modifier.
I rebuilt the local Stride packages and confirmed that a code-only game using Vulkan can render debug text without crashing.

Related Issue

Fixes #3375

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

I tested the change in a code-only game on Linux/Vulkan.
I did not run Game Studio because it is not currently available on Linux.

Vulkan says unmapped memory is no longer readable
@BenjaBobs

Copy link
Copy Markdown
Author

@dotnet-policy-service agree


graphicsContext.CommandList.UnmapSubResource(mappedIndices);

indexBufferBinding = new IndexBufferBinding(Buffer.Index.New(graphicsContext.CommandList.GraphicsDevice, new ReadOnlySpan<byte>((void*)indexPointer, indexBufferSize)), true, indexBufferLength);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem with this code is not using the now unmapped buffer to construct the index buffer, but constructing a new unnecessary index buffer using the pointer. Let me explain:

  • The index buffer is created above in line 96 as a dynamic buffer. It is then mapped and populated, then unmapped.
  • Then, when creating the IndexBufferBinding instead of using our already populated indexBuffer we do another Buffer.Index.New passing the now invalid pointer to the data to populate it. But that is exactly what we already did above!

This seems like a mistake from when this was ported from C++ to C#, and not a problem per-se of persisting the pointer incorrectly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, the way it was before two buffers were used because the final one should be immutable for read optimization, and so a dynamic one was created to fill in the actual data afterwards.

create mutable buffer > fill with data > copy to immutable buffer > cleanup mutable buffer.

My change would then change this to
build data in int[] > copy to immutable buffer

So I guess you could say the double buffering from before was unnecessary, but from as a consumer the fatal issue was what is effectively a "read-after-free".

What do you think if the solution?
I also considered using NativeMemory to avoid GC but that would also mean keeping the unsafe and doing a try/finally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FastTextRenderer reads a Vulkan mapping after it has been unmapped

2 participants