C++ application calling C# Dll marshalling strings

The code appears to work fine. The values are returned as expected from the DLL and the values are added correctly to the DLL.

Will the way I am passing the string to C# cause a memory leak?

Also, I have to return a string from C# as type LPSTR to add to a char pointer, how could I change it so I could use the string type for the return value.

Also, I assume its related to the .Net garbage collector not kicking in yet but here is the values in TaskManager at different points:

First Line Of C Code = 1,424k
Just After Create Instance = 6,752k
Just Before Calling EndString = 26,264k
After EndString = 26,276k
After CoUninit = 26,320k


I would have expected the values to drop back down? Or do we have to wait for the garbage collector?

Here is the sample code I put together in Visual Studio 6.0:

`#include "stdafx.h"
#include
using namespace std;

#import "C:\\Documents and Settings\\Administrator\\My Documents\\Visual Studio 2008\\Projects\\COMCallable\\COMCallable\\bin\\Release\\COMCallable.tlb" raw_interfaces_only
using namespace COMCallable;

MyNetInterfacePtr pComObject;

string RetString();

void AddValue(long i);

void AddValue2(long i);

int main(int argc, char* argv[])
{
HRESULT hr = S_OK;

hr = CoInitialize(NULL);

if(pComObject == NULL)
{
pComObject.CreateInstance(__uuidof(MyNETClass));
}

pComObject->StartString();

for(long i = 1; i EndString();

pComObject->Release();

CoUninitialize();

while(true)
{
Sleep(1000);
}

return 0;
}

string RetString()
{
char *c = NULL;

if(pComObject != NULL)
{
pComObject->TestMethodString(&c);
}

if(c != NULL)
{
return c;
}
else
{
return "";
}
}

void AddValue(long i)
{
char numstr[21] = {0}; // enough to hold all numbers up to 64-bits
sprintf(numstr, "%ld", i);

string s = string("Some String Here ") + string(numstr);

I would have expected the values to drop back down? Or do we have to wait for the garbage collector?

That's not how the GC works. AFAIK, it only returns memory to the OS if the OS says it'as low on memory. The assumption is that if the OS doesn't need the memory, then it's better to keep the memory as a part of the managed heap, so that the GC doesn't need to run that often.

using namespace std;

This is generally considered a bad practice in C++, since the std namespace contains many simple names that you might want to use yourself.

#import "C:\\Documents and Settings\\Administrator\\My Documents\\Visual Studio 2008\\Projects\\COMCallable\\COMCallable\\bin\\Release\\COMCallable.tlb" raw_interfaces_only

I think that both projects should be part of the same solution and in under the same parent directory. That way, this path can be relative, which will make it much short and it will also work if you copy the whole solution to another computer.

MyNetInterfacePtr pComObject;

I think this shouldn't be a global variable. If you need to pass the object from one function to another, use parameters.

HRESULT hr = S_OK;

hr = CoInitialize(NULL);

There is no reason to initialize hr if you're going to overwrite the value right away anyway.

And if you know that a function can fail, you should check whether it failed or not, not just ignore the HRESULT.

if(pComObject == NULL)

pComObject has to be NULL at this point, so there is no reason for this check.

while(true)
{
    Sleep(1000);
}

If you want to wait forever, the documentation says you should be able to use Sleep(INIFINITE);.

char *c = NULL;

if(pComObject != NULL)
{
    pComObject->TestMethodString(&c);
}

I don't know much about memory management in COM, but this certainly looks like a memory leak. pComObject can't know when are you done using it, so it can't deallocate it itself and you also don't deallocate it.

sprintf(numstr, "%ld", i);

The documentation says:


Using sprintf, there is no way to limit the number of characters written, which means that code using sprintf is susceptible to buffer overruns.

By my count, 21 characters should be enough here, but I would probably use the safer version anyway, just to be sure.

if(pComObject != NULL)
{
    pComObject->AddString((char *)s.c_str());
}

So, if pComObject is NULL, this function doesn't do anything? I think that's a bad design, when something bad happens, the user should learn about it ASAP, not weeks later when they find out that their database is full of bogus data (or whatever).

sb = null;
sb = new StringBuilder();

That first line is completely unnecessary. If you're going to set the field to a new value anyway, setting it to null first won't change anything.

I don't understand the design of that class. What is the purpose of StartString and EndString? Practically, both do exactly the same thing, because of the null check in AddString().

You could provide just a single Reset() method that sets sb to a new StringBuilder and that would be enough. You could also remove the null check from AddString(), because sb would never be null.

Or even better, make MyNETClass single use. Instead of resetting, you would just create a new instance.

StackExchange Code Review Q#64410, answer score: 3