Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

MarijnS95 · 2020-11-10T23:12:29Z

While this prefix may be an implementation detail the BSTR type guarantees to be able to store NULL values without accidentally truncating the string, hence its length must be available to replace the usual NULL terminator. This string length in bytes is stored in the four bytes preceding a BSTR pointer 1, whose value can be retrieved using SysStringLen.

This commit implements the type in the same way in WinAdapter such that users of the functions returning a BSTR (currently only in dxcisense) can expect and use the type in the same way on Linux as they would on Windows.
While this function and type currently only seem to be involved in names and logging where NULLs are unlikely, it is good practice to mirror the Windows type exactly for future-proofing.

Besides, SysAllocStringLen does not specify anything about taking ownership of strIn so the realloc here is wrong either way.

While this prefix may be an implementation detail the BSTR type guarantees to be able to store NULL values without accidentally truncating the string, hence its length must be available to replace the usual NULL terminator. This string length in bytes is stored in the four bytes preceding a BSTR pointer [1], whose value can be retrieved using SysStringLen. This commit implements the type in the same way in WinAdapter such that users of the functions returning a BSTR (currently only in dxcisense) can expect and use the type in the same way on Linux as they would on Windows. While this function and type currently only seem to be involved in names and logging where NULLs are unlikely, it is good practice to mirror the Windows type exactly for future-proofing. Besides, SysAllocStringLen does not specify anything about taking ownership of strIn so the realloc here is wrong either way. [1]: https://docs.microsoft.com/en-us/previous-versions/windows/desktop/automat/bstr

Needs microsoft/DirectXShaderCompiler#3250

AppVeyorBot · 2020-11-11T00:06:32Z

✅ Build DirectXShaderCompiler 1.0.3852 completed (commit 07eee7a7a0 by @MarijnS95)

ehsannas

LGTM! Thanks @MarijnS95

MarijnS95 · 2020-11-12T17:17:40Z

@ehsannas Thanks for merging this quickly! Keep in mind that this is a slightly breaking change for Unix users that are free()ing the pointer currently. Following Traverse-Research/hassle-rs#10 we had a discussion whether DXC should export symbols for CoTaskMemFree (and SysFreeString, SysString*Len) so that we do not have to "guess" what primitives DXC uses to complement their Windows counterpart? What do you think?

On a similar note, is there anything I can do to give #3062 the same approval + merge treatment?

ehsannas · 2020-11-12T17:44:57Z

Yeah I understand. If anyone out there is free() ing the pointer, I think they can switch to SysFreeString easily. I agree that it's better to simulate the Windows behavior more accurately here.

MarijnS95 · 2020-11-12T19:52:18Z

If anyone out there is free() ing the pointer, I think they can switch to SysFreeString easily

@ehsannas The problem is that SysFreeString and friends do not exist outside of Windows, and their shim implementation details on these platforms are uniquely defined by DirectXCompiler. Implementors against libdxcompiler.so currently have to look into the code and assume the way they interact with these shims remains correct.

Hence my request to export these functions (including CoTaskMemFree and SysString*Len) such that there is no guesswork: call it the same as Windows, and it'll just work, even if (though unlikely) shims are modeled differently in the future.

When running valgrind over a program using `DxcIntelliSense` (to validate our own deallocations [1]) a bunch of mismatching `new` with `free()` show up: Mismatched free() / delete / delete [] at 0x483B9AB: free (vg_replace_malloc.c:538) by 0x52B06A8: IMalloc::Free(void*) (WinAdapter.cpp:34) by 0x6DC7D19: DxcTranslationUnit::Release() (dxcisenseimpl.h:308) by 0x14278E: com_rs::unknown::IUnknown::release (unknown.rs:55) [...] Address 0x4c3c930 is 0 bytes inside a block of size 40 alloc'd at 0x483B07F: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:385) by 0x6DC06EB: DxcIndex::ParseTranslationUnit(char const*, char const* const*, int, IDxcUnsavedFile**, unsigned int, DxcTranslationUnitFlags, IDxcTranslationUnit**) (dxcisenseimpl.cpp:1192) by 0x13020A: hassle_rs::intellisense::ffi::IDxcIndex::parse_translation_unit (macros.rs:108) by 0x119B74: hassle_rs::intellisense::wrapper::DxcIndex::parse_translation_unit (wrapper.rs:101) [...] And so on for the other intellisense classes. All these classes have `DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL` which deallocates `this` on the associated `m_pMalloc` with `free()`: The "TM" version keep an IMalloc field that, if not null, indicate ownership of 'this' and of any allocations used during release. Yet are allocated using `new`, resulting in this mismatch. The solution is to follow a similar approach as the introduction of `IMalloc` to `DxcIntelliSense` in d5bb308 by rewriting all classes to take an `IMalloc *` in the constructor and invoking it either through `::Alloc` from `DXC_MICROCOM_TM_ALLOC` or `CreateOnMalloc`. [1]: microsoft#3250 (comment)

When running valgrind over a program using `DxcIntelliSense` (to validate our own deallocations [1]) a bunch of mismatching `new` with `free()` show up: Mismatched free() / delete / delete [] at 0x483B9AB: free (vg_replace_malloc.c:538) by 0x52B06A8: IMalloc::Free(void*) (WinAdapter.cpp:34) by 0x6DC7D19: DxcTranslationUnit::Release() (dxcisenseimpl.h:308) by 0x14278E: com_rs::unknown::IUnknown::release (unknown.rs:55) [...] Address 0x4c3c930 is 0 bytes inside a block of size 40 alloc'd at 0x483B07F: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:385) by 0x6DC06EB: DxcIndex::ParseTranslationUnit(char const*, char const* const*, int, IDxcUnsavedFile**, unsigned int, DxcTranslationUnitFlags, IDxcTranslationUnit**) (dxcisenseimpl.cpp:1192) by 0x13020A: hassle_rs::intellisense::ffi::IDxcIndex::parse_translation_unit (macros.rs:108) by 0x119B74: hassle_rs::intellisense::wrapper::DxcIndex::parse_translation_unit (wrapper.rs:101) [...] And so on for the other intellisense classes. All these classes have `DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL` which deallocates `this` on the associated `m_pMalloc` with `free()`: The "TM" version keep an IMalloc field that, if not null, indicate ownership of 'this' and of any allocations used during release. Yet are allocated using `new`, resulting in this mismatch. The solution is to follow a similar approach as the introduction of `IMalloc` to `DxcIntelliSense` in d5bb308 by rewriting all classes to take an `IMalloc *` in the constructor and invoking it either through `::Alloc` from `DXC_MICROCOM_TM_ALLOC` or `CreateOnMalloc`. [1]: #3250 (comment)

MarijnS95 · 2020-11-17T14:31:54Z

@ehsannas I'm afraid I forgot to tag you in the reply above and it seems like you have not received a notification for it. Are there any objections to exporting aforementioned shimmed functions for use on non-Windows platforms?

ehsannas · 2020-11-17T16:08:00Z

I'm afraid I'm not following what you exactly mean by exporting them. Would you please make a separate PR so that discussions can take place there? Thanks

Since microsoft/DirectXShaderCompiler#3250

We [recently] fixed memory leaks when calling into the intellisense part of DXC and made the decision shortly after merging to [implement null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows, which was promptly accepted. Currently our implementation of `SysFreeString` together with that PR segfaults because it frees the pointer at offset `+4` of the allocation. That is corrected together with `SysStringLen` now reading the size of the string from this prefix instead of looking for a (wrong!) null character. At least our Windows and Linux implementation `utils.rs` is the same again. [recently]: #10 [implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250

We [recently] fixed memory leaks when calling into the intellisense part of DXC and made the decision shortly after merging to [implement null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows, which was promptly accepted. Currently our implementation of `SysFreeString` together with that PR segfaults because it frees the pointer at offset `+4` of the allocation. That is corrected together with `SysStringLen` now reading the size of the string from this prefix instead of looking for a (wrong!) null termination character. At least our Windows and Linux implementation `utils.rs` is the same again. [recently]: #10 [implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250

…ize (#11) We [recently] fixed memory leaks when calling into the intellisense part of DXC and made the decision shortly after merging to [implement null-invariant `BSTR`s properly] in DXC's shimmed type for non-Windows, which was promptly accepted. Currently our implementation of `SysFreeString` together with that PR segfaults because it frees the pointer at offset `+4` of the allocation. That is corrected together with `SysStringLen` now reading the size of the string from this prefix instead of looking for a (wrong!) null termination character. At least our Windows and Linux implementation `utils.rs` is the same again. [recently]: #10 [implement null-invariant `BSTR`s properly]: microsoft/DirectXShaderCompiler#3250

MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Nov 10, 2020

Linux: DXC BSTR now resembles its Windows counterpart with explicit size

5cfd0f5

Needs microsoft/DirectXShaderCompiler#3250

ehsannas self-requested a review November 12, 2020 16:43

ehsannas added the linux Linux-specific work label Nov 12, 2020

ehsannas approved these changes Nov 12, 2020

View reviewed changes

ehsannas merged commit 2ade6f8 into microsoft:master Nov 12, 2020

MarijnS95 deleted the linux-bstr branch November 12, 2020 17:17

MarijnS95 mentioned this pull request Nov 12, 2020

dxcisense: Allocate "TM" classes using IMalloc instead of new #3258

Merged

MarijnS95 mentioned this pull request Nov 17, 2020

[RFC] WinAdapter: Export helper functions to interact with shimmed types #3265

Open

MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Nov 25, 2020

Linux: DXC BSTR now resembles its Windows counterpart with explicit size

1c80cf8

Since microsoft/DirectXShaderCompiler#3250

MarijnS95 mentioned this pull request Nov 25, 2020

Linux: Update to DXC's null-invariant BSTR Traverse-Research/hassle-rs#11

Merged

Jasper-Bekkers mentioned this pull request Dec 10, 2020

Traverse Research pull requests Traverse-Research/opensource-ecosystem#1

Open

38 tasks

MarijnS95 added a commit to Traverse-Research/hassle-rs that referenced this pull request Dec 29, 2020

Linux: DXC BSTR now resembles its Windows counterpart with explicit size

ca50c59

Since microsoft/DirectXShaderCompiler#3250

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

MarijnS95 commented Nov 10, 2020

AppVeyorBot commented Nov 11, 2020

ehsannas left a comment

MarijnS95 commented Nov 12, 2020

ehsannas commented Nov 12, 2020

MarijnS95 commented Nov 12, 2020 •

edited

MarijnS95 commented Nov 17, 2020

ehsannas commented Nov 17, 2020

Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

Linux: Implement prefix-counted BSTR allocation in SysAllocStringLen #3250

Conversation

MarijnS95 commented Nov 10, 2020

AppVeyorBot commented Nov 11, 2020

ehsannas left a comment

Choose a reason for hiding this comment

MarijnS95 commented Nov 12, 2020

ehsannas commented Nov 12, 2020

MarijnS95 commented Nov 12, 2020 • edited

MarijnS95 commented Nov 17, 2020

ehsannas commented Nov 17, 2020

MarijnS95 commented Nov 12, 2020 •

edited