[DML EP] Fix wide string handling in OpKernelInfoWrapper::GetWideName - #31656
Conversation
…eName GetWideName passes the source length explicitly to MultiByteToWideChar, so the API never appends a null terminator. When the converted node name needs exactly as many code units as the caller's buffer holds, the call succeeds, returns bufferSizeInChars, and does not set ERROR_INSUFFICIENT_BUFFER, so writing the terminator at outputName[charsCopiedIfSucceeded] lands one wchar_t past the end of the buffer. Clamp the index so the terminator stays in bounds, truncating the last converted char in that case. The sibling GetUtf8Name reservesroom the same way via bufferSizeInBytes - 1.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
miaobin
changed the title
|
@fdwr PTAL, thanks! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a potential out-of-bounds write in the DML EP’s OpKernelInfoWrapper::GetWideName when converting UTF-8 node names to wide strings via MultiByteToWideChar. The issue occurs because MultiByteToWideChar does not append a null terminator when the source length is explicitly provided, so a successful conversion can exactly fill the caller’s buffer, and writing the terminator at outputName[charsCopied] can land one wchar_t past the end.
Changes:
- Clamp the null-terminator write index to
bufferSizeInChars - 1on successful conversion, preserving the “may truncate name” contract while preventing OOB writes. - Add inline comments documenting the
MultiByteToWideCharbehavior and rationale for clamping/truncation.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/core/providers/dml/DmlExecutionProvider/src/MLOperatorAuthorImpl.cpp | Prevents an off-by-one write when null-terminating the converted wide node name buffer. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
There was a problem hiding this comment.
So the case where this would happen is when the name is exactly 512 characters? 511 characters would be fine (since charsCopiedIfSucceeded == 511), and 513 characters would be fine too (falling through the ERROR_INSUFFICIENT_BEFFER path where bufferSizeInChars - 1 = 511). Thanks for fixing 🛠️.
Description
GetWideNamepasses the source length explicitly toMultiByteToWideChar, so the API never appends a null terminator. When the converted node name needs exactly as many code units as the caller's buffer holds, the call succeeds, returnsbufferSizeInChars, and does not setERROR_INSUFFICIENT_BUFFER, so writing the terminator atoutputName[charsCopiedIfSucceeded]lands one wchar_t past the end of the buffer.Motivation and Context
Clamp the index so the terminator stays in bounds, truncating the last converted char in that case. Truncation is already part of the contract: both in-tree callers in DmlOperator.cpp use a fixed wchar_t[512] and note "might truncate name", and the ERROR_INSUFFICIENT_BUFFER branch below already writes to bufferSizeInChars - 1. The sibling GetUtf8Name reserves room the same way via bufferSizeInBytes - 1.