Loss of data while building a std::string from const char * or LPCSTR

Question

I have a Function which returns a LPSTR/const char * and I need to convert it to a std::string. This is how I am doing it.

std::string szStr(foo(1));

It works just fine in all the cases just when foo returns a 32 characters long string it fails. With this approach I get "". So I thought it had to do something with the length. So I changed it a bit.

std::string szStr(foo(1) , 32);

This gives me "0"

Then I tried another tedious method

const char * cstr_a = foo(1);
const char * cstr_b = foo(2);

size_t ln_a = strlen(cstr_a);
size_t ln_b = strlen(cstr_b);

std::string szStr_a( cstr_a , ln_a );
std::string szStr_b( cstr_b , ln_b );

But strangely enough in this method both the pointers are getting the same value, viz foo(1) should return abc and foo(2) should return xyz. But here cstr_a is first getting abc but the moment cstr_b gets xyz, the value of both cstr_a and cstr_b becomes xyz. I am dazed and confused with this.

And yes, I cannot use std::wstring.

What is foo?

foo is basically reading a value from the registry and returning it as a LPSTR. Now one the value in the registry which I need to read is a MD5 hashed string (32 charecters) That's where it fails.

The Actual Foo function:

LPCSTR CRegistryOperation::GetRegValue(HKEY hHeadKey, LPCSTR szPath, LPCSTR szValue)
{
    HKEY hKey;
    CHAR szBuff[255] = ("");
    DWORD dwBufSize = 255;

    ::RegOpenKeyEx(hHeadKey, (LPCSTR)szPath, 0, KEY_READ, &hKey);
    ::RegQueryValueEx(hKey, (LPCSTR)szValue, NULL, 0, (LPBYTE)szBuff, &dwBufSize);
    ::RegCloseKey(hKey);

    LPCSTR cstr(szBuff);

    return cstr;
}

The Original cast code:

StrResultMap RegValues;
std::string lid(CRegistryOperation::GetRegValue(HKEY_CURRENT_USER, REG_KEY_HKCU_PATH, "LicenseID"));
std::string mid(CRegistryOperation::GetRegValue(HKEY_CURRENT_USER, REG_KEY_HKCU_PATH, "MachineID"), 32);
std::string vtill(CRegistryOperation::GetRegValue(HKEY_CURRENT_USER, REG_KEY_HKCU_PATH, "ValidTill"));
std::string adate(CRegistryOperation::GetRegValue(HKEY_CURRENT_USER, REG_KEY_HKCU_PATH, "ActivateDT"));
std::string lupdate(CRegistryOperation::GetRegValue(HKEY_CURRENT_USER, REG_KEY_HKCU_PATH, "LastUpdate"));

RegValues["license_id"] = lid;
RegValues["machine_id"] = mid;
RegValues["valid_till"] = vtill;
RegValues["activation_date"] = adate;
RegValues["last_updated"] = lupdate;

Kindly help me get over it. Thanks.

And what is `foo` doing exactly ? Is the returned c-string null terminated ? — quantdev, Aug 19 '14 at 13:45
The line `std::string szStr(foo(1));` is 100% correct provided that `foo` returns zero-terminated sequence of characters. — Wojtek Surowka, Aug 19 '14 at 13:48
`foo is basically reading`... `That's where it fails.` If *that's where it fails* (and it probably is), then why isn't *that* code in the question? — eerorika, Aug 19 '14 at 13:52
See http://stackoverflow.com/questions/342772/convert-lptstr-to-char or http://msdn.microsoft.com/en-us/library/87zae4a3%28VS.80%29.aspx — MP24, Aug 19 '14 at 13:53
`LPCSTR` is a pointer, yes? It's pointing to a local array and is left dangling when the array is destroyed at the end of the function. Dereferencing the returned pointer (which is what the `string`'s constructor does) has undefined behaviour. — eerorika, Aug 19 '14 at 13:56
possible duplicate of [Can a local variable's memory be accessed outside its scope?](http://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope) — eerorika, Aug 19 '14 at 13:58
szBuff is casted to a LPCSTR variable before the array is destroyed and thats what it is returning. What am I missing? — Genocide_Hoax, Aug 19 '14 at 14:01
@Genocide_Hoax when you cast an array to a pointer, the array parameter first decays to a pointer to the first value. So what you return is a pointer to the first element of `szBuff` which is a local array. — eerorika, Aug 19 '14 at 14:05
Agreed ! Now this confuses me as to how did it work for all the other cases? I guess I was a victim of UB. Thanks a lot , will keep this in mind in the future. — Genocide_Hoax, Aug 19 '14 at 14:12

score 2 · Answer 1 · answered Aug 19 '14 at 14:13

As a complement to Nordic Mainframe's anwser, there are 3 common ways to return a buffer from a C or C++ function :

use a static buffer - simple and nice until you have re-entrancy problems (multiple threads or recursivity)
pass the buffer as an input parameter, and simply return the number of characters written to it - ok if the size of buffer is really a constant
malloc the buffer in the function (it is in the heap and not in the stack) and document in flashing red that it must be freed by caller

But as you tagged your question as C++, you could create the std::string in the function and return it. C++ functions are allowed to return std::string because the different operators (copy constructor, affectation, ...) take care automatically of the allocation problem.

Ferruccio · Answer 2 · 2014-08-20T10:27:36.073

You can avoid returning a pointer to a buffer which has gone out of scope by returning a std::string directly.

std::string CRegistryOperation::GetRegValue(HKEY hHeadKey, LPCSTR szPath, LPCSTR szValue)
{
    HKEY hKey = 0;
    CHAR szBuff[255] = { 0 };
    DWORD dwBufSize = sizeof(szBuf);

    if (::RegOpenKeyEx(hHeadKey, (LPCSTR)szPath, 0, KEY_READ, &hKey) == ERROR_SUCCESS) {
        ::RegQueryValueEx(hKey, (LPCSTR)szValue, NULL, 0, (LPBYTE)szBuff, &dwBufSize);
        ::RegCloseKey(hKey);
    }
    return std::string(szBuf);
}

Nordic Mainframe · Accepted Answer · 2014-08-20T16:20:10.453

The GetRegValue function returns a pointer to a buffer in GetRegValue's stack frame. This does not work: after GetRegValue terminates the pointer cstr points to undefined values somewhere in the Stack. Try to make szBuff static and see if that helps.

LPCSTR CRegistryOperation::GetRegValue(HKEY hHeadKey, LPCSTR szPath, LPCSTR szValue)
{
    HKEY hKey;
  //Here:
    static CHAR szBuff[255] = ("");
    szBuff[0]=0;
    DWORD dwBufSize = 255;

    ::RegOpenKeyEx(hHeadKey, (LPCSTR)szPath, 0, KEY_READ, &hKey);
    ::RegQueryValueEx(hKey, (LPCSTR)szValue, NULL, 0, (LPBYTE)szBuff, &dwBufSize);
    ::RegCloseKey(hKey);

    LPCSTR cstr(szBuff);

    return cstr;
}

UPDATE: I did not mandate to return std::string, or pass a buffer in, because that would change the interface. Returning a pointer to a static buffer is a common idiom and mostly unproblematic if the lifetime of the returned pointer value is limited to a few scopes (Like for building a std::string from the buffer value).

Multithreading isn't really an issue aynmore, because almost every compiler now has some form of thread local storage support right in the language. __declspec(thread) static CHAR szBuff[255] = (""); for example, should work for Microsoft compilers, __thread for gcc. C++11 even has a new storage class specifier for this (thread_local). You shouldn't call GetRegValue from a signal handler though, but that's OK - you can't do too much there anyway (for example allocate memory from the heap!).

UPDATE: Commenters argue, that I should not suggest this, because the pointer to the static buffer will point to invalid data when GetRegValue is called again. While this is obviously true, I think it is wrong to make an argument from that. Why? Look at these examples:

A pointer returned from strdup() is valid until free()
A pointer to something created with new is valid until deleted.
A const char * returned from string::c_str() is valid as long as the string is not modified.
A std::vector iterator is invalid, if an element from the std::vector is erased.
A std::list iterator is still valid, if an element from the std::vector is erased, unless it points to the erased element.
A pointer returned from GetRegValue is valid until GetRegValue is called again.
a std::ifstream is valid when,..you know, good(), fail() and so on.

There is no point in saying, "look, the thing gets invalid when you are not careful enought", because programming is not about being careless. We are handling objects, which have conditions under which they are valid or not and if an object has well defined conditions under which it is valid or not, then we can write programs with well defined behaviour. Returning a pointer to a static buffer (that is thread-local) has a well defined meaning and a developer can use this to write a well defined program. Unless said developer is negligent or too lazy to read the documentation of the routine of course.

The problem with making `szBuf` static is that if the function is called more than once and the callers hold on to the returned pointer, they will all be pointing to the same buffer which will have the last value returned. It would be better to return a `std::string`. — Ferruccio, Aug 19 '14 at 14:17
And in multithreaded environment, it will create more of mess! Either pass char-array to function, or use some string class. — Ajay, Aug 19 '14 at 15:00
It's not common idiom and IT'S VERY PROBLEMATIC. DON'T DO THIS. For example, in the OP's original code, `cstr_a` and `cstr_b` *would point to the same buffer*. — Puppy, Aug 20 '14 at 10:32
@Puppy: Please read OP's statement again. His orignal code is in the "The original cast code" box, which is safe with GetRegValue returning a pointer to a static buffer. — Nordic Mainframe, Aug 20 '14 at 11:05
It is safe for the moment, but code inevitably changes and someone will get tripped up on this. Code like this is like a forgotten landmine. I've done things like this in the past and I've **always** wound up regretting it. — Ferruccio, Aug 20 '14 at 11:33

Loss of data while building a std::string from const char * or LPCSTR

3 Answers3