3

i've got following code to encrypt a string in a C++ DLL

EXPORT WCHAR* EncryptString(WCHAR* stringToEncrypt) {
    aes_context ctx;

    WCHAR* in = stringToEncrypt;
    WCHAR* out;
    WCHAR* key = L"TestKey";

    BYTE* buffEnc = (BYTE*)malloc(16);
    BYTE* keyBuffEnc = (BYTE*)malloc(32);

    memset(buffEnc, 0, 16);
    memset(keyBuffEnc, 0, 32);

    memcpy(buffEnc, in, wcslen(in) * 2);
    memcpy(keyBuffEnc, key, wcslen(key) * 2);
    aes_set_key(&ctx, keyBuffEnc, 256);

    aes_encrypt(&ctx, buffEnc, buffEnc);
    out = (WCHAR*)buffEnc;

    // free(buffEnc);   
    // free(keyBuffEnc);

    return out;
}

My problem is that i can not free the allocated memory because otherwise the result is broken. I wonder how can i free the used memory without losing the result? Have i to change the type of return value?

Thanks in advance for your help. Greets Heinz

qwerty
  • 2,065
  • 2
  • 28
  • 39
Action Heinz
  • 722
  • 10
  • 23
  • Your code sample is not standard C++ (since `WCHAR` is not a standard type, but some system specific thing). Did you consider using `std::string` or `std::wstring` ? – Basile Starynkevitch Oct 02 '13 at 07:02
  • 1
    You should duplicate data, instead of returning a pointer – qwerty Oct 02 '13 at 07:03
  • `out` is borrowing the memory from buffEnc. The safest thing to do would be to return a std::wstring; using buffEnc in construction. Free the memory once std::wstring is constructed, then return. – Bathsheba Oct 02 '13 at 07:03
  • Or you should define a convention where the caller of your `EncryptString` is required to `free` it (or `delete` it if you return a heap pointer to some object). – Basile Starynkevitch Oct 02 '13 at 07:04

2 Answers2

8

This is indeed a problematic situation - you return a pointer to allocated memory and it's unclear who should free the memory. You have the following options:

  1. tell the caller free the memory using free() - this will only work if they use the same heap which is hard to guarantee. This is very unreliable and not really recommended.
  2. introduce a memory management interface (such as freeEncrypted() function that is implemented in your library) and tell the caller use it - then memory will be returned to the right heap.
  3. use something well known like CoTaskMemAlloc() for allocation and tell the caller to use the matching function such as CoTaskMemFree() for freeing memory. This is similar to point 2, just uses a well known common memory manager.
  4. change the interface such that it accepts pointer to already allocated data and its size so that the caller both allocates and frees the memory.
sharptooth
  • 167,383
  • 100
  • 513
  • 979
7

On Windows, the memory manager (which among other things keeps track of the allocated and free memory in your process) works in the C runtime library. This means that if you have two modules (say: your actual executable and a DLL, or two DLLs) and you want to allow one module to allocate some memory and the other one should own it (i.e. be responsible for freeing it or passing the maintainership on) thre are basically three options:

  1. You let the caller allocate a piece of memory and pass a pointer to that to the callee. In your example, that would boil down to the caller allocating a (hopefully sufficiently large) buffer and then passing a pointer to that to the EncryptString function. The advantage of this approach is that the caller can choose to just allocate a piece of memory on the stack and then pass a pointer to that, something like

    WCHAR buf[1024];
    EncryptString( "Hello", buf ); // Won't compile, "Hello" is a const string
    

    The disadvantage is that the caller has to figure out an appropriate buffer size first. You could just impose a maximum limit and document that, or you could have a dedicated function exposed which computes the required size, or you could say that if NULL is passed as the output buffer, the function returns the number of required characters. The latter is commonly used by the Windows API.

  2. You let the callee allocate the memory (as your function does right now) using e.g. malloc or new but then dictate that the caller has to use a special function to release the memory again, something like FreeEncryptedString(char *s). The idea is that this deallocation function is also exposed by your DLL and it just calls the appropriate deallocation function (i.e. free or delete or delete[] or the like), so that both allocation and deallocation happen within the same module.

  3. You ensure that both modules link against the same C runtime library dynamically, i.e. there is just one copy of the C runtime DLL in the process and both of your modules use it. In that case, you could just use malloc and free (or new and delete etc.) as you want to. The advantage of this is that it's very straightforward, the disadvantage is that it means you impose requirements on how your modules are built (Which may not be possible if you work on a program which loads plugins written by other people or so - those plugins may chose to link against the C runtime statically).

Frerich Raabe
  • 90,689
  • 19
  • 115
  • 207
  • It's very hard to guarantee that two modules are linked against exactly the same C runtime if they are developed and built by different people. – sharptooth Oct 02 '13 at 07:08
  • It's tagged as C++, so you should use `new` and `delete`, not `malloc` and `free`. – parrowdice Oct 02 '13 at 07:10
  • @parrowdice: The OP used `malloc`, that's why I stuck to it. – Frerich Raabe Oct 02 '13 at 07:10
  • @parrowdice: Sorry, but this just shifts focus from the real problem. – sharptooth Oct 02 '13 at 07:10
  • @parrowdice: I mentioned `new` and `delete` as examples of other functions now. – Frerich Raabe Oct 02 '13 at 07:12
  • 2
    Or since this is Windows API related one could use HeapAlloc/HeapFree. – bkausbk Oct 02 '13 at 07:31
  • @bkausbk: Good point. I wonder - do you know why `new` or `malloc` on Windows don't actually use `HeapAlloc` on the process heap (i.e. what `GetProcessHeap`) returns? This would make them behave more like they do on Unix, but I suppose it'd be a performance hit? The [comparison of allocation methods](http://msdn.microsoft.com/en-us/library/windows/apps/aa366533(v=vs.85).aspx) unfortunately didn't tell. – Frerich Raabe Oct 02 '13 at 07:50
  • @FrerichRaabe: HeapAlloc can not allocate more than 512KB. Internally malloc/new may use HeapAlloc for smaller block sizes. See http://msdn.microsoft.com/en-us/library/windows/desktop/aa366533%28v=vs.85%29.aspx There is also a special Low Frequency Heap that allows allocation of large amount of small blocks. But a good malloc implementation is faster than HeapAlloc. Search for `tcmalloc` or `dlmalloc`. – bkausbk Oct 02 '13 at 09:03