Creating a System::String object from a BSTR in Managed C++ - is this way a good idea?

Question

My co-worker is filling a System::String object with double-byte characters from an unmanaged library by the following method:

RFC_PARAMETER aux;
Object* target;
RFC_UNICODE_TYPE_ELEMENT* elm;
elm = &(m_coreObject->m_pStructMeta->m_typeElements[index]);
aux.name = NULL;
aux.nlen = 0;
aux.type = elm->type;
aux.leng = elm->c2_length;
aux.addr = m_coreObject->m_rfcWa + elm->c2_offset;

GlobalFunctions::CreateObjectForRFCField(target,aux,elm->decimals);
GlobalFunctions::ReadRFCField(target,aux,elm->decimals);

Where GlobalFunctions::CreateObjectForRFCField creates a System::String object filled with spaces (for padding) to what the unmanaged library states the max length should be:

static void CreateObjectForRFCField(Object*& object, RFC_PARAMETER& par, unsigned dec)
{
    switch (par.type)
    {
        case TYPC:
            object = new String(' ',par.leng / sizeof(_TCHAR));
            break;
        // unimportant afterwards.
    }
}

And GlobalFunctions::ReadRFCField() copies the data from the library into the created String object and preserves the space padding:

static void ReadRFCField(String* target, RFC_PARAMETER& par)
{
    int lngt;
    _TCHAR* srce;
    switch (par.type)
    {
        case TYPC:
        case TYPDATE:
        case TYPTIME:
        case TYPNUM:
            lngt = par.leng / sizeof(_TCHAR);
            srce = (_TCHAR*)par.addr;
            break;

        case RFCTYPE_STRING:
            lngt = (*(_TCHAR**)par.addr != NULL) ? (int)_tcslen(*(_TCHAR**)par.addr) : 0;
            srce = *(_TCHAR**)par.addr;
            break;

        default:
            throw new DotNet_Incomp_RFCType2;
    }

    if (lngt > target->Length) lngt = target->Length;

    GCHandle gh = GCHandle::Alloc(target,GCHandleType::Pinned);
    wchar_t* buff = reinterpret_cast<wchar_t*>(gh.AddrOfPinnedObject().ToPointer());
    _wcsnset(buff,' ',target->Length);
    _snwprintf(buff,lngt,_T2WFSP,srce);
    gh.Free();
}

Now, on occasion, we see access violations getting thrown in the _snwprintf call. My question really is: Is it appropriate to create a string padded to a length (ideally to pre-allocate the internal buffer), and then to modify the String using GCHandle::Alloc and the mess above.

And yes, I know that System::String objects are supposed to be immutable - I'm looking for a definitive "This is WRONG and here is why".

Thanks, Eli.

Answer 1

Actually, the issue was not with the .NET string as an output buffer, but with the input buffer instead.

The sprintf("%s") class functions (including wsprintf and so on) will perform a strlen-type operation on any parameters on the string - EVEN if it is snwprintf - the "n" part only limits the amount WRITTEN to the string, NOT the characters read from the input buffer.

Turns out the input buffer was never guaranteed to be null terminated. Often times we get lucky because if the data returned was small, it would hit SOMETHING null before reaching bad memory.

However, if the data in there is big enough, it would go to the end of the memory page. When the strlen keeps going, it walks off the page, and Access Violation city!

Luckily I found this when testing something else with a native mode debugger attached and ready with all the debug symbols from MS for the C runtime!

We switched the snwprintf to a wcsncpy() instead (the snwprintf was a legacy operation when we had to do ANSI->Unicode conversion - why he didn't do MultiByteToWideChar() instead, I'll never know.

thanks for the advice at any rate.