Support building cpprestsdk with /MT on Windows #884

ioannis-e · 2018-10-02T02:22:03Z

The basic concept for building cpprestsdk with /MT on Windows is to use:

standard library containers whose allocators and deallocators use HeapAlloc and HeapFree directly, thus bypassing the risk of heap corruption when the standard library containers get destroyed.
uses of new to be replaced with utility::make_shared and utility::make_unique, that have been implemented to use HeapAlloc and HeapFree for allocation and destruction respectively.

In this respect all references to std::string, std::vector, etc... have been replaced with utility::string, utility::vector, etc..., and most of the references to new have been replaced with utility::make_shared and utility::make_unique.

Additionally plaintext_string has been replaced with utility::secure_unique_string which on Windows will call SecureZeroMemory

basic_container_buffer, basic_rawptr_buffer, winhttp_request_context

in Windows when compiled with /MT

…shared or make_unique

…shared or make_unique in pplx

msftclas · 2018-10-02T02:22:15Z

All CLA requirements met.

ioannis-e · 2018-10-02T12:18:17Z

@BillyONeal, @ras0219-msft Can you please let me know whether I went overboard with any of the changes to the library.

Also can someone provide nuget packages of OpenSSL and zlib that are build with /MT instead of /MD to include as dependencies?

Thanks in advance.

BillyONeal · 2018-10-02T21:11:19Z

You use utility:: things but I don't see their definitions anywhere; I don't understand this change. If utility:: is not the same as std:: in this context, that's a huge breaking change we probably cannot accept. If it is the same as std::, then changing all these places appears unmotivated.

The standard containers, at least in the versions of VS that cpprestsdk supports, call HeapAlloc and HeapFree, because malloc and free were changed to call those in the UCRT transition -- so it isn't clear what supposed corruption avoidance this gains in any case.

BillyONeal · 2018-10-02T21:13:22Z

Release/include/cpprest/details/basic_types.h

+        // construct by copying (do nothing)
+    }
+
+    allocator(_Alloc&& _Right) CPPREST_NOEXCEPT : _Mybase(std::move(_Right)) {


This constructor doesn't do anything.

This is copy paste from std::allocator. I don't claim to know all std::allocator requirements, but if any function is not being used it will not be compiled right? In any case anything not being used can be removed.

BillyONeal · 2018-10-02T21:13:32Z

Release/include/cpprest/details/basic_types.h

+    }
+
+    template<class _Other>
+    allocator(allocator<_Other>&& _Right) CPPREST_NOEXCEPT : _Mybase(std::forward<_Other>(_Right)) {


Ditto constructor doesn't do anything.

BillyONeal · 2018-10-02T21:13:45Z

Release/include/cpprest/details/basic_types.h

+        return (*this);
+    }
+
+    _Alloc& operator=(_Alloc&& _Right) {


Ditto does nothing

BillyONeal · 2018-10-02T21:13:55Z

Release/include/cpprest/details/basic_types.h

+    }
+
+    template<class _Other>
+    _Alloc& operator=(const allocator<_Other>&) {


Not part of the allocator requirements.

BillyONeal · 2018-10-02T21:16:07Z

Release/include/cpprest/details/basic_types.h

+
+    void deallocate(pointer _Ptr, size_type _Count) {
+        // deallocate object at _Ptr
+        ::SecureZeroMemory(_Ptr, _Count * sizeof(_Ty));


Note that this will zero allocated memory blocks but will not necessarily zero everything a container touches. For example, node-based containers with container-internal sentinel nodes, or std::string's small string optimization.

In either case, this doesn't seem appropriate to include here...

The reasoning I included the secure_allocator was for std::vector when encrypting/decrypting user supplied credentials, where for example on line 109 the buffer may be resized and moved to another memory block, how would one ensure that the previous memory block has been securely zeroed ?

https://github.com/Microsoft/cpprestsdk/blob/f897582260f2942284f9213ab9027486bde21b73/Release/src/utilities/web_utilities.cpp#L92-L115

Yikes, you're correct, that's broken. :( For std::vector (and only std::vector) I think an allocator like this can work.

That said I would prefer to just fix that code to make the vector the correct size at the beginning :)

Unfortunately this allocator doesn't make the type Win32Encryption safe, and I don't think any change in the memory allocation strategy can do that (for precisely the reason in this comment, the container body itself is not zeroed).

It looks like all the APIs that touch that though are deprecated for precisely this reason.

I hardened what was possible to harden here: #896

BillyONeal · 2018-10-02T21:16:26Z

Release/include/cpprest/details/basic_types.h

+
+    _DECLSPEC_ALLOCATOR pointer allocate(size_type _Count) {
+        // allocate array of _Count elements
+        return static_cast<pointer>(::HeapAlloc(::GetProcessHeap(), NULL, _Count * sizeof(_Ty)));


This allocate is damaged vs. std::allocator -- it is not checking for overflow of _Count.

Would the following resolve this ?

__declspec(allocator) allocate(size_type _Count) { // allocate array of _Count elements if (_Count == 0) return nullptr; // check overflow of multiply if ((size_t)(-1) / sizeof(_Ty) < _Count) _Xbad_alloc(); // report no memory return static_cast<pointer>(::HeapAlloc(::GetProcessHeap(), NULL, _Count * sizeof(_Ty))); }

BillyONeal · 2018-10-02T21:16:40Z

Release/include/cpprest/details/basic_types.h

+        return (*this);
+    }
+
+    _DECLSPEC_ALLOCATOR pointer allocate(size_type _Count) {


This is user code so it isn't allowed to be _Ugly.

I don't understand, can you please explain how to resolve ?

http://eel.is/c++draft/lex.name#3 says that _Ugly and __ugly names are reserved for use by the C++ implementation (compiler and standard library), not for user code. That said I think this is kind of moot given the compat problems here.

BillyONeal · 2018-10-02T21:17:13Z

Release/include/cpprest/details/basic_types.h

+        return (*this);
+    }
+
+    _DECLSPEC_ALLOCATOR pointer allocate(size_type _Count) {


_DECLSPEC_ALLOCATOR is not a publicly documented supported _Ugly name and will be removed as soon as real declspec allocator is usable unconditionally in the standard library.

Should i use __declspec(allocator) instead ?

BillyONeal · 2018-10-02T21:18:52Z

Release/include/cpprest/details/basic_types.h

+using queue = std::queue<_Ty, _Container>;
+
+template<class _Ty, class _Traits = std::char_traits<_Ty>, class _Alloc = utility::allocator<_Ty>>
+using basic_string = std::basic_string<_Ty, _Traits, _Alloc>;


Ah, I see this now; I don't think we can accept this change because it will be massively breaking to any existing customers.

First of all thank you for the comments, I will try to go through them.

I don't consider this to be breaking existing customers compatibility because the utility:: should be equal to the std:: counterparts (i.e. utility::string = std::string) unless _WIN32 is defined and _DLL is not defined which is the case for /MT, which none of the existing customers would be using anyway, please correct me if i am wrong.

I believe getting a configuration that uses /MT is already in wide use through vcpkg.

BillyONeal · 2018-10-02T21:35:13Z

Have you considered customizing std::allocator to do what you want by statically linking in an ::operator new and ::operator delete that does what you want instead of needing to rewrite the universe?

ioannis-e · 2018-10-03T23:10:43Z

Have you considered customizing std::allocator to do what you want by statically linking in an ::operator new and ::operator delete that does what you want instead of needing to rewrite the universe?

@BillyONeal Thank you for all of your comments, I agree that there is a lot of rewriting but I consider that it is the library's job to maintain the correct types being supplied by the user of the library in order to function correctly and not corrupt the heap (since for example user supplied strings are std::moved into the library and subsequently freed by the library and vise versa with server handler methods.

Maybe I am overthinking it, but this is the solution I came up with.
I would greatly appreciate if I could build as a dynamic library with /MT without obviously heavily modifying the library or the project using the library.
I would also appreciate even more if you could provide with a working example to get me started.

Thanks again.

ioannis-e · 2018-10-03T23:20:25Z

I have two commits 1fe626f and d104d95 that not relate directly to the whole endeavor.
Can these be cherry-picked or should i make separate pull requests ?

BillyONeal · 2018-10-09T18:26:47Z

I consider that it is the library's job to maintain the correct types being supplied by the user of the library in order to function correctly and not corrupt the heap

I agree it would be a good thing in theory, but in this case we have existing users who would be heavily broken, and it is unclear what actual benefit this provides. Again, back in the era where each DLL had their own heap and users had to worry about that this may have been a can of worms worth opening, but in the UCRT era malloc, ::operator new, and std::allocator use the process heap and couldn't care less about /MT, just as your custom allocator does.

I would greatly appreciate if I could build as a dynamic library with /MT without obviously heavily modifying the library or the project using the library.

I have two commits 1fe626f and d104d95 that not relate directly to the whole endeavor.
Can these be cherry-picked or should i make separate pull requests ?

I can cherry pick them if you wish. f743245 and 5247191 also look fine.

ioannis-e added 8 commits October 2, 2018 05:07

Remove _ASYNCRTIMP from ~http_listener() and implement inline

f743245

Correct relative include path to resource.h and version.h

d104d95

Allow ppltasks.h and pplxtasks.h to co-exist

5247191

Make class constructors public to allow use of make_shared

4be5657

basic_container_buffer, basic_rawptr_buffer, winhttp_request_context

Correct spelling mistake

1fe626f

Use container allocators and deallocators with HeapAlloc and HeapFree

6009909

in Windows when compiled with /MT

Replace std:: containers with utility:: containers and new with make_…

893c61f

…shared or make_unique

Replace std:: containers with utility:: containers and new with make_…

8e5a572

…shared or make_unique in pplx

ioannis-e mentioned this pull request Oct 2, 2018

Add an option in CMake to build cpprestsdk using /MT #881

Open

BillyONeal reviewed Oct 2, 2018

View reviewed changes

BillyONeal closed this Nov 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support building cpprestsdk with /MT on Windows #884

Support building cpprestsdk with /MT on Windows #884

ioannis-e commented Oct 2, 2018

msftclas commented Oct 2, 2018 •

edited

Loading

ioannis-e commented Oct 2, 2018

BillyONeal commented Oct 2, 2018

BillyONeal Oct 2, 2018

ioannis-e Oct 3, 2018

BillyONeal Oct 2, 2018

BillyONeal Oct 2, 2018

BillyONeal Oct 2, 2018

BillyONeal Oct 2, 2018

ioannis-e Oct 3, 2018

BillyONeal Oct 9, 2018

BillyONeal Oct 9, 2018

BillyONeal Oct 9, 2018

BillyONeal Oct 2, 2018

ioannis-e Oct 3, 2018

BillyONeal Oct 2, 2018

ioannis-e Oct 3, 2018

BillyONeal Oct 9, 2018

BillyONeal Oct 2, 2018

ioannis-e Oct 3, 2018

BillyONeal Oct 2, 2018

ioannis-e Oct 3, 2018

BillyONeal Oct 9, 2018

BillyONeal commented Oct 2, 2018

ioannis-e commented Oct 3, 2018

ioannis-e commented Oct 3, 2018

BillyONeal commented Oct 9, 2018

Support building cpprestsdk with /MT on Windows #884

Support building cpprestsdk with /MT on Windows #884

Conversation

ioannis-e commented Oct 2, 2018

msftclas commented Oct 2, 2018 • edited Loading

ioannis-e commented Oct 2, 2018

BillyONeal commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Oct 2, 2018

ioannis-e commented Oct 3, 2018

ioannis-e commented Oct 3, 2018

BillyONeal commented Oct 9, 2018

msftclas commented Oct 2, 2018 •

edited

Loading