Zero-fill padding in detach_abi(com_array) #1165
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A code analysis warning recently fired for a customer on detach_abi(com_array<T>). This function returns a std::pair<uint32_t, some_pointer_type>, which will have, on 64-bit builds, 4 bytes of padding between the uint32 size and the pointer members. Currently, that padding is uninitialized. The idea behind the code analysis warning is that information may be leaked via those unitialized bytes. In practice, that's almost never going to be an issue for this function, because the std::pair is not an interesting object to pass around, and only exists as a convenience to return both the size and buffer of the com_array at the same time. However, the fix removes a pain point for a customer, is simple, risk-free, and actually gets optimized away in the 99% use case (return value stored in a local variable, access only the first/second members, not the padding bytes). Demo showing optimization: https://godbolt.org/z/T4vPhMKxn
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kennykerr
approved these changes
Jul 6, 2022
alvinhochun
added a commit
to alvinhochun/cppwinrt
that referenced
this pull request
Dec 29, 2022
`detach_abi(com_array<T>&)` uses two `memset` calls to zero two objects, one is an `std::pair` (from [microsoft#1165]), the other is a `winrt::com_array` (to clear its data pointer to prevent its destructor from freeing the array). GCC rightfully warns about this because these types are not trivially copyable, so calling memset on it is UB. This change fixes the UB and the GCC warning by reverting the first `memset` back to using the `std::pair` constructor as before [microsoft#1165], and changing the second `memset` to setting the member fields directly. Since this requires access to protected members, the function body is moved into a private member function, then the `winrt::detach_abi` free function is made `friend` of `com_array` to allow it to call the member function. This fix is not applied when `_MSC_VER` is defined in order to preserve the current behaviour on MSVC, in case [microsoft#1165] is still relevant. [microsoft#1165]: microsoft#1165
alvinhochun
added a commit
to alvinhochun/cppwinrt
that referenced
this pull request
Dec 29, 2022
`detach_abi(com_array<T>&)` uses two `memset` calls to zero two objects, one is an `std::pair` (from [microsoft#1165]), the other is a `winrt::com_array` (to clear its data pointer to prevent its destructor from freeing the array). GCC rightfully warns about this because these types are not trivially copyable, so calling memset on it is UB. This change fixes the UB and the GCC warning by reverting the first `memset` back to using the `std::pair` constructor as before [microsoft#1165], and changing the second `memset` to setting the member fields directly. Since this requires access to protected members, the function body is moved into a private member function, then the `winrt::detach_abi` free function is made `friend` of `com_array` to allow it to call the member function. This fix is not applied when `_MSC_VER` is defined in order to preserve the current behaviour on MSVC, in case [microsoft#1165] is still relevant. [microsoft#1165]: microsoft#1165
alvinhochun
added a commit
to alvinhochun/cppwinrt
that referenced
this pull request
Dec 29, 2022
`detach_abi(com_array<T>&)` uses two `memset` calls to zero two objects, one is an `std::pair` (from [microsoft#1165]), the other is a `winrt::com_array` (to clear its data pointer to prevent its destructor from freeing the array). GCC rightfully warns about this because these types are not trivially copyable, so calling memset on it is UB. This change fixes the UB and the GCC warning by reverting the first `memset` back to using the `std::pair` constructor as before [microsoft#1165], and changing the second `memset` to setting the member fields directly. Since this requires access to protected members, the function body is moved into a private member function, then the `winrt::detach_abi` free function is made `friend` of `com_array` to allow it to call the member function. This fix is not applied when `_MSC_VER` is defined in order to preserve the current behaviour on MSVC, in case [microsoft#1165] is still relevant. [microsoft#1165]: microsoft#1165
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A code analysis warning recently fired for a customer on detach_abi(com_array). This function returns a std::pair<uint32_t, some_pointer_type>, which will have, on 64-bit builds, 4 bytes of padding between the uint32 size and the pointer members. Currently, that padding is uninitialized.
The idea behind the code analysis warning is that information may be leaked via those unitialized bytes.
In practice, that's almost never going to be an issue for this function, because the std::pair is not an interesting object to pass around, and only exists as a convenience to return both the size and buffer of the com_array at the same time.
However, the fix removes a pain point for a customer, is simple, risk-free, and actually gets optimized away in the 99% use case (return value stored in a local variable, access only the first/second members, not the padding bytes). Demo showing optimization: https://godbolt.org/z/T4vPhMKxn