2

Today I was able to write a simple C++ program that granted a user the "Log on as a service" privilege. Part of this involved converting between a LPCWSTR and an LSA_UNICODE_STRING. The code to do that is here:

LSA_UNICODE_STRING StringToLsaUnicodeString(LPCWSTR string) {
    LSA_UNICODE_STRING lsaString;
    DWORD dwLen = 0;

    dwLen = wcslen(string);
    lsaString.Buffer = (LPWSTR) string;
    lsaString.Length = (USHORT)((dwLen) * sizeof(WCHAR));
    lsaString.MaximumLength = (USHORT)((dwLen + 1) * sizeof(WCHAR));
    return lsaString;
}

When I had some small errors in this function, my call to LsaLookupNames2() failed with a code 87(hex 0x57) "The parameter is incorrect." I am trying to make this call in a C++ app that uses std::wstring and it is failing. My current function there is as follows:

#if defined(_UNICODE)
    LSA_UNICODE_STRING toLsaUnicodeString (std::wstring str) {
        LSA_UNICODE_STRING lsaWStr;
        DWORD len = 0;

        LPWSTR cstr = (LPWSTR)str.c_str();
        len = wcslen(cstr);
        lsaWStr.Buffer = cstr;
        lsaWStr.Length = (USHORT)((len) * sizeof(WCHAR));
        lsaWStr.MaximumLength = (USHORT)((len + 1) * sizeof(WCHAR));
        return lsaWStr;
    } 
#endif

What am I doing wrong?

Mormegil
  • 7,955
  • 4
  • 42
  • 77
Justin Dearing
  • 14,270
  • 22
  • 88
  • 161
  • You don't need the `#if defined(_UNICODE)`. The code is correct regardless. – MSalters Jan 16 '12 at 09:35
  • To add to MSSalters' comment, it is better to pass instances of std::wstring by const reference (i.e.: const std::wstring & str) instead of by value, to avoid useless deep-copies. –  Jan 16 '12 at 09:59

3 Answers3

4

You're likely encountering a lifetime issue with the wchar_t* returned from str.c_str(). str.c_str() will return a pointer to an underlying string whose lifetime is governed by str. Since str is passed by-value, it will be destroyed at the end of the toLsaUnicodeString function, resulting in the returned LSA_UNICODE_STRING pointing at memory that has been deallocated. In order to avoid this, you'll need to make a copy of the underlying string in the toLsaUnicodeString function, and associate the copy with the returned LSA_UNICODE_STRING, something like:

LSA_UNICODE_STRING toLsaUnicodeString (const std::wstring& str) {
    LSA_UNICODE_STRING lsaWStr;
    DWORD len = 0;

    len = str.length(); 
    LPWSTR cstr = new WCHAR[len + 1];
    memcpy(cstr, str.c_str(), (len + 1) * sizeof(WCHAR));
    lsaWStr.Buffer = cstr;
    lsaWStr.Length = (USHORT)((len) * sizeof(WCHAR));
    lsaWStr.MaximumLength = (USHORT)((len + 1) * sizeof(WCHAR));
    return lsaWStr;
}

Since the memory is now allocated on the heap, you are responsible for making sure it is deallocated. You can use a function like the following to take care of this.

void freeLsaUnicodeString(LSA_UNICODE_STRING& str) {
    delete [] str.Buffer;
    str.Buffer = 0;
    str.Length = 0;
    str.MaximumLength = 0;
}

Even better would be to use RAII to manage the memory and guarantee that it is released when the variable is no longer in use. See Mr_C64's answer for details on this approach.

DRH
  • 7,868
  • 35
  • 42
  • Or, you could just pass a const std::wstring& to the function instead of a std::wstring. – StilesCrisis Jan 16 '12 at 05:54
  • 1
    @StilesCrisis: While that would solve the immediate crash, it would introduce a very risky API. E.g. if the caller passes `L"Foo"`, you'd get a silent conversion to a temporary `std::wstring`. That temporary goes out of scope when `toLsaUnicodeString` returns, but the `LSA_UNICODE_STRING` returned would still point to it. – MSalters Jan 16 '12 at 09:38
  • I think passing "const std::wstring &" as function parameter is a good suggestion: there is no need for useless deep-copies caused by passing std::wstring by value. Moreover, in the freeLsaUnicodeString function the 'str' parameter should be passed as a reference (and not by value), so its content can be modified by the function and the modifications be visible to the caller. Moreover, after delete[]ing the buffer pointer, the pointer should be set to NULL to avoid double deletions (and other members of LSA_UNICODE_STRING should be updated to reflect the new state of empty string). –  Jan 16 '12 at 10:37
  • Moreover, since we are in C++ world (and not in the C world), it should be better to use RAII and C++ destructors, instead of programming using "C++ as a better C", defining create/free functions that are not exception-safe and can generate memory leaks if not called properly (instead C++ destructors are called automatically by the compiler, so proper use of RAII and destructors makes it possible to write code that is structurally incapable of leaking, and easier to write and maintain). –  Jan 16 '12 at 10:46
  • @Mr_C64, thanks for the recommendations, I've updated the code to use references and to clear the LSA_UNICODE_STRING. I agree that RAII would be a useful idiom here and added a reference to your answer. – DRH Jan 16 '12 at 14:56
  • @DRH: I noticed your code update, and thanks for mentioning my answer. –  Jan 17 '12 at 20:59
3

I think the correct way of doing this in C++ is to write a RAII wrapper class around the raw C structure LSA_UNICODE_STRING.

The constructor overloads of this class properly initialize it, the destructor releases allocated resources (helping writing exception-safe code), and you can provide some operator= overloads to do proper deep-copies.

Instead of using explicit new[] and delete[], the dynamically-allocated WCHAR buffer is managed by an instance of std::vector, which simplifies the code (e.g. std::vector's destructor will automatically release the allocated memory).

Something like this:

#include <windows.h>     // Win32 SDK header
#include <LsaLookup.h>   // LSA_UNICODE_STRING
#include <vector>        // std::vector
#include <string>        // std::wstring


//
// C++ RAII wrapper to LSA_UNICODE_STRING
//
class LsaUnicodeString
{
public:

    LsaUnicodeString()
    {
        SetEmpty();
    }


    LsaUnicodeString(const LsaUnicodeString & source)
    {
        CopyFrom(source);
    }


    explicit LsaUnicodeString(const std::wstring & source)
    {
        CopyFrom(source);
    }


    ~LsaUnicodeString()
    {
        // Nothing to do:
        // the string buffer is managed by std::vector data member
    }


    LsaUnicodeString & operator=(const LsaUnicodeString & source)
    {
        if (&source != this)
        {
            CopyFrom(source);
        }
        return *this;
    }


    LsaUnicodeString & operator=(const std::wstring & source)
    {
        CopyFrom(source);
        return *this;
    }


    const LSA_UNICODE_STRING & Get() const
    {
        return m_us;
    }


    //
    // Implementation
    //
private:
    LSA_UNICODE_STRING m_us;        // raw C structure
    std::vector<WCHAR> m_buffer;    // string content


    void SetEmpty()
    {
        m_buffer.resize(1);
        m_buffer[0] = L'\0'; // end-of-string

        m_us.Length = 0;
        m_us.MaximumLength = sizeof(WCHAR);
        m_us.Buffer = &m_buffer[0];
    }


    void CopyFrom(const std::wstring & source)
    {
        if ( source.empty() )
        {
            SetEmpty();
            return;
        }

        const int len = source.length();
        m_buffer.resize(len + 1);
        ::CopyMemory(&m_buffer[0], source.c_str(), (len+1)*sizeof(WCHAR));

        m_us.Length = len * sizeof(WCHAR);
        m_us.MaximumLength = m_us.Length + sizeof(WCHAR);
        m_us.Buffer = &m_buffer[0];
    }


    void CopyFrom(const LsaUnicodeString & source)
    {
        if (source.m_us.Length == 0)
        {
            SetEmpty();
            return;
        }

        m_buffer = source.m_buffer;
        m_us.Length = source.m_us.Length;
        m_us.MaximumLength = source.m_us.MaximumLength;
        m_us.Buffer = &m_buffer[0];
    }
};
1

You can use the RtlInitUnicodeString function in order to initialize a unicode string. After using the UNICODE_STRING call RtlFreeUnicodeString.

The UNICODE_STRING and LSA_UNICODE_STRING are identical.

Norbert Willhelm
  • 2,579
  • 1
  • 23
  • 33