Groups | Blog | Home
all groups > dotnet interop > february 2008 >

dotnet interop : Marshal::StringToHGlobalAnsi and System.AccessViolationException


Jeroen Mostert
2/27/2008 12:00:00 AM
[quoted text, click to view]

Abominable. One cast should raise a flag. Three casts should raise a red
alert. This is clearly the work of someone who didn't know what they were
doing and just strung casts together until it appeared to work.

That said, this *does* happen to work, although it's unnecessarily
cast-happy. It's equivalent to

char* str = static_cast<char*>(Marshal::StringToHGlobalAnsi(s).ToPointer());

[quoted text, click to view]

This call is wrong. The second argument to strncpy_s is the *total* size of
the destination, including room for the null terminator. This call tries to
copy 19 characters plus null terminator into a buffer with a specified size
of 19 bytes, which will not fit. This should be

strncpy_s(someStr, 20, str, 19);

or even better

strncpy_s(someStr, _countof(someStr), str, _TRUNCATE);

to copy all the characters that will fit. If truncation is not supposed to
happen, then just leave it at

strncpy_s(someStr, _countof(someStr), str, strlen(str));

which will raise an error if the string does not fit.

[quoted text, click to view]
Here's a much shorter and cleaner way of doing the same thing:

{
pin_ptr<const wchar_t> ps = PtrToStringChars(s);
size_t charsConverted;
wcstombs_s(&charsConverted, someStr, _countof(someStr), ps, _TRUNCATE);
}

This copies as much characters of "s" as will fit into the array "someStr",
converting the characters from Unicode to ANSI along the way. It does not
allocate additional memory, which it achieves by temporarily pinning the string.

Of course, depending on what you actually do with the string, copying to an
array may be unnecessary in the first place.

--
Jeroen Mostert
2/27/2008 12:00:00 AM
[quoted text, click to view]
Yeah, well, except for the fact that it should be

const char* str = static_cast<const
char*>(Marshal::StringToHGlobalAnsi(s).ToPointer());

but other than that...

--
Jeroen Mostert
2/27/2008 12:00:00 AM
[quoted text, click to view]

Yes, it does. The line is still wrong, of course, but if "str" really
happens to be smaller, it will work.

If the rest of the code looks like this, however, I wouldn't be surprised if
it's corrupted the stack somewhere along the way, and you just happen to see
the problem around here. Good luck diagnosing.

[quoted text, click to view]
But if it does happen, you have to choose to either get an error or truncate
the remainder, depending on what's most appropriate.

--
Dilip
2/27/2008 12:21:46 PM

I have cross-posted this question to 3 newsgroups since I am not sure
exactly where it belongs (and some NG's have very less activity).
Apologies in advance for this. I have set the followup to the
dotnet.framework.interop NG but feel free to chage it as appopriate.

I have a question regarding converting a Managed System.String^ to
unmanaged const char*.
I inherited a code that does it like this:

String^ s = "00000";
LPCSTR str = static_cast<LPCTSTR>(const_cast<void*>(static_cast<const
void*>(Marshal::StringToHGlobalAnsi(s))));
char someStr[20];
strncpy_s(someStr, 19, str, 19);
someStr[19] = 0;
Marshal::FreeHGlobal(static_cast<IntPtr>(const_cast<void*>(static_cast<const
void*>(str))));

Leaving aside the fact that there are better ways to accomplish what I
want (like calling .ToPointer() for example) can someone tell me if
that piece of code as it stands has a potential to create problems? I
am getting some System.AccessViolationException around these parts --
although I can't be sure this is what is causing the problem I want to
eliminate it as a potential issue. Here are my issues with that block
of code:

1) It seems to do an unnecessary amount of back-and-forth castings for
what should essentially be cast to a void* and then to a const char*.
First it casts the code to a const void*, then it casts away the const
and then does a cast of const char*. I feel uneasy with so many casts
around.
2) Same with the FreeHGlobal -- too many casts
3) the code that does strncpy seems to be copying more than it
should. I think Marshal::StringTOHGlobalAnsi returns a null-
terminated string. Copying 19 bytes further than seems to be
trampling on memory that doesn't belong to the code.

Mark Salsbery [MVP]
2/27/2008 1:32:14 PM
[quoted text, click to view]


The casts are necessary if using the casting operators. Using the old
C-style casts it could be something like this:

String^ s = "00000";
const char *str = (const char *)(void *)Marshal::StringToHGlobalAnsi(s);
char someStr[20];
strncpy_s(someStr, 19, str, 19);
someStr[19] = 0;
Marshal::FreeHGlobal(System::IntPtr((void*)str));



[quoted text, click to view]


From the docs:
"These functions try to copy the first D characters of strSource to strDest,
where D is the lesser of count and the length of strSource."

If you pass _TRUNCATE as the last parameter to strncpy_s, then you don't
have to worry about "someStr[19] = 0;"

strncpy_s(someStr, 20, str, _TRUNCATE);

Which line are you getting the System.AccessViolationException on??

Mark

--
Mark Salsbery
Microsoft MVP - Visual C++


[quoted text, click to view]
Dilip
2/27/2008 2:20:09 PM
[quoted text, click to view]

Gotcha Jeroen! Thanks for your suggestions. I accept your points but
just so that I can conclusively isolate this code as suspicious -- in
my specific case, 'str' comes in as "00000". so when you do:

strncpy_s(someStr, 19, str, 19);

1) str is shorter than someStr, only 5 bytes and null-terminated (i.e
I am assuming the previous call to Marshal::StringToHGlobalAnsi
returns a pointer to a null terminated string). Isn't strncpy_s
supposed to stop copying characters when it encounters the null-
terminator?[1]
2) _TRUNCATE is necessary only when source is greater than
destination, right? In my case that almost never happens.


[1] "The strncpy_s() function copies not more than a specified number
of successive characters (characters that follow a null character are
not copied) from a source string to a destination character array."
https://buildsecurityin.us-cert.gov/daisy/bsi/articles/knowledge/coding/317.=
html
Dilip
2/27/2008 2:21:37 PM
On Feb 27, 3:32=A0pm, "Mark Salsbery [MVP]"
[quoted text, click to view]

Thanks Mark. The exact line isn't captured by the debugger. the
control lands at a place that is few lines below the suspicious code I
wrote about. I have responded to Jeroen's post. Do let me know what
Mihai N.
2/27/2008 6:36:05 PM

[quoted text, click to view]
If you know that is better, why not use it?

========================
String^ managedString = "Hello unmanaged world (from the managed world).";
char* stringPointer = (char*) Marshal::StringToHGlobalAnsi(managedString
).ToPointer();
Marshal::FreeHGlobal(IntPtr(stringPointer));

Official example at http://msdn2.microsoft.com/en-
us/library/system.runtime.interopservices.marshal.stringtohglobalansi.aspx
==================


[quoted text, click to view]
Potential problem here: why static_cast to LPCTSTR if what you want is
LPCSTR? The 'T' in LPCTSTR means that when compiled as Unicode the type
is really wchar_t const *, while LPCSTR is char const *
So you basically do char const * str = static_cast<wchar_t const *>(...)


--
Mihai Nita [Microsoft MVP, Windows - SDK]
http://www.mihai-nita.net
------------------------------------------
Mihai N.
2/27/2008 6:38:25 PM
[quoted text, click to view]
Extra: there is no need to "manually" put the ending zero if you use
strncpy_s


--
Mihai Nita [Microsoft MVP, Windows - SDK]
http://www.mihai-nita.net
------------------------------------------
Ben Voigt [C++ MVP]
3/1/2008 2:50:39 PM

[quoted text, click to view]

Don't cast and cast back. Instead:

IntPtr hglobalPtr = Marshal::StringToHGlobalAnsi(managedString);
char* stringPointer = static_cast<char*>(hglobalPtr.ToPointer());
....
Marshal::FreeHGlobal(hglobalPtr);



[quoted text, click to view]
Norman Diamond
3/5/2008 9:21:48 AM
The only problem that I see with the casts is that they're unreadable
sequences of garbage, but no obvious technical problems.

Your insight about the number of bytes is correct. The program tramples on
14 unknown bytes. We can only wonder why the program sometimes works.


[quoted text, click to view]
AddThis Social Bookmark Button