Check if value in registry exists doesn't work.

[rtfm]

Hello!
I'm trying to write to registry and check whether or not a value exists. The value does exist and the value is created by the program, but it doesn't return ERR_VAL_EXISTS.

What's wrong with it?

#include <iostream>
#include <windows.h>

#define ERR_NOT_ADMIN 1006
#define ERR_VAL_EXISTS 1007


class Reg
{

    long lRet;
    HKEY hKey;

    public:
    int CreateRegValue();
};

int Reg::CreateRegValue()
{
    std::string newfile = "C:\\WINDOWS\\system32\\program.exe";
    DWORD dwBuflen = newfile.length();

    lRet = RegOpenKeyEx(HKEY_LOCAL_MACHINE, "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Run", 0, KEY_QUERY_VALUE | KEY_SET_VALUE, &hKey);

    if(RegQueryValueEx(hKey, TEXT("ProgramKey"), NULL, NULL, NULL, NULL) == ERROR_FILE_NOT_FOUND)
    {
        std::cout << "Writing to registry...\n";
        if(RegSetValueEx(hKey, "ProgramKey", 0, REG_SZ, (LPBYTE) newfile.c_str(), dwBuflen) != ERROR_SUCCESS)
        {
            return ERR_NOT_ADMIN;
        }
        else
        {
            RegCloseKey(hKey);
        }
    }
    else
    {
        RegCloseKey(hKey);
        return ERR_VAL_EXISTS;
    }

}


int main()
{
    Reg reg;
    int ret = reg.CreateRegValue();
    if(ret == ERR_VAL_EXISTS)
    {
        std::cout << "The value exists!" << std::endl;
    }
    else
    {
        std::cout << "The value doesn't exist." << std::endl;
    }

    std::cin.get();
    return 0;
}

Thanks in advance.

[webJose]

1. You are mixing ANSI and potential-Unicode (T-stuff). Bad combination.
2. You are not checking the result value of RegOpenKeyEx().
3. What does it return? Not all code paths return a value. WARNING! (Visual Studio should have given you this warning.)
4. Have you debugged the code? You should also collect the return value of RegQueryValueEx().

[rtfm]

Thanks for answering, webJose.

1) Where?
2) I don't need to (I only need to know if the value exists).
3) RegQueryValueEx() returns:

If the function succeeds, the return value is ERROR_SUCCESS.

If the function fails, the return value is a system error code.

If the lpData buffer is too small to receive the data, the function returns ERROR_MORE_DATA.

If the lpValueName registry value does not exist, the function returns ERROR_FILE_NOT_FOUND.

4) Depends on what you mean by debugging. I'm using Code::Blocks with GCC (haven't used OllyDBG or GDB).

Edit:

I've discovered something weird. When I remove the value, it prints out "Writing to registry...", but when there is a value it skips that part but still doesn't return ERR_VAL_EXISTS.

[webJose]

1. Line 23 and line 25 in conjunction with line 20.
2. Yes, you need to. In order to determine if the key value exists or not, you must verify that the key handle is being obtained in the first place. If you are getting an error code from RegOpenKeyEx() then you have no hopes in finding if the key value exists or not.
3. LOL!! I don't need a lecture on what the registry function returns. I am asking you what value YOUR function is returning in every case, so you realize that not all code paths are returning a value, which means you'll most likely get garbage values in some instances. Example: After line 34 the function just ends without a return statement. What's the value returned there? Undefined.
4. I mean running your code line by line and actually seeing with your own eyes how is each line executed and how your program flows. If the program flows unexpectedly, then verify the variable values to make sure they hold the expected values, and if they don't, pay close attention on how the values are obtained and correct accordingly.

[rtfm]

1) As you may have noticed I don't really know what you're talking about, so an elaboration would be nice.
2) Er, no. I know that the handle is correct since it creates the value, if the handle returned by RegOpenKeyEx() would be wrong in some way the value couldn't possibly be created.
3) I don't really see how that's relevant with the topic, I doubt RegCloseKey(hKey); would change the return value of RegOpenKeyEx().
4) Alright, I'll do that.

Thank you for your answer.

[webJose]

1. It's a long topic and you should read a decent tutorial on Windows programming. I'll just summarize. If you don't understand it, proceed to a thorough tutorial.

The Windows API is for the most part doubled: There is a set of functions that take and manage ANSI strings and another set that take and manage Unicode strings. These functions have different names: The ANSI ones' names will end with uppercase A, and the Unicode ones' names will end with uppercase W. Example: RegOpenKeyExA(), RegOpenKeyExW().

On top of that, the Windows header files #define macros for each of these functions like this:

#ifdef UNICODE
#define RegOpenKeyEx RegOpenKeyExW
#else
#define RegOpenKeyEx RegOpenKeyExA
#endif //UNICODE 

So, when you write code for Windows, you must always know which function you are using (if the -A or the -W) so as to pass along the right type of string. Your code is currently working fine for you under Code::Blocks, but if you ever pass along this code to, say Visual Studio 2010, you'll notice it doesn't compile! This is because by default, Visual Studio C++ projects are Unicode builds, meaning the UNICODE macro is automatically #defined. So there goes your code down to the drain.

In order to properly code for Windows, you MUST use one of the following combinations ONLY. Any other combination is just WRONG:

A. If you use -W functions, you must use wchar_t-based string types like LPWSTR, WCHAR[], LPCWSTR, std::wstring, std::wostringstream, std::wstringstream, and L-prepended string literals (i. e. L"Hello world!"). You also output to the console using std::wcout and you input using std::wcin.
B. If you use -A functions, you must use char-based string types like LPSTR, char[], LPCSTR, std::string, std::ostringstream, std::stringstream and regular string literals (i. e. "Hello world!"). You also output to the console using std::cout and you input using std::cin.
C. If you use function names with neither termination, you must aid yourself with some data types for the STL types:

typedef std::basic_string<TCHAR> tstring;
typedef std::basic_ostringstream<TCHAR> tostringstream;
typedef std::basic_stringstream<TCHAR> tstringstream;
//Etc.  Add more if you use more, like std:basic_istringstream<TCHAR> for input string streams.

#ifdef UNICODE
#define tcout std::wcout
#define tcin std::wcin
#else
#define tcout std::cout
#define tcin std::cin
#endif 

So now I state case C: If you use function names with neither termination, you must use TCHAR-based string types like LPTSTR, TCHAR[], LPCTSTR, tstring, tostringstream, tstringstream and TEXT- OR _T-macro-prepended string literals (i. e. TEXT("Hello world!") or _T("Hello world") ). You also output to the console using tcout and you input using tcin.

In your code, you use function names that don't end with A or W, meaning you must fall into case C above, and as you can see, you are mixing them with std::string and naked literal strings. Because of all this, your code will fail to compile inside a Visual Studio 2010 C++ project with default settings.

2. You still do need to check it, you just don't see it.
3. You don't get the point. Hopefully, you'll get it someday.

[andywestken]

You should also check the return code from RegOpenKeyEx.

As you are trying to open a subkey of HKLM with write access, I would expect this call to fail with ERROR_ACCESS_DENIED for users without admin rights (on Vista and newer).

And close the key if you successfully open it, rather than successfully query a value.

[rtfm]

I rewrote the code and fixed it. Thanks webJose for the nice explanation regarding ANSI and Unicode functions.
I don't know if I'm doing it right, but at least it works.

#include <iostream>
#include <Windows.h>
#include <string>

class Registry
{
    HKEY hKey;
    LPCTSTR subKey;
    LPCTSTR subValue;
    HKEY resKey;
    DWORD dataLen;

    public:

    bool OpenKey();
    bool GetValue();
    bool WriteValue();

};

bool Registry::OpenKey()
{
    hKey = HKEY_LOCAL_MACHINE;
    subKey = "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Run";

    long key = RegOpenKeyExA(hKey, subKey, 0, KEY_READ | KEY_WRITE, &resKey);
    if(key == ERROR_SUCCESS)
    {
        return true;
    }
    else
    {
        return false;
    }
}

bool Registry::GetValue()
{
    subValue = "ProgramData";
    long key = RegQueryValueExA(resKey, subValue, NULL, NULL, NULL, NULL);
    if(key == ERROR_FILE_NOT_FOUND)
    {
        return false;
    }
    else
    {
        return true;
    }
}

bool Registry::WriteValue()
{
    std::string data = "C:\\WINDOWS\\system32\\program.exe";
    DWORD dataLen = data.size()+1;

    long key = RegSetValueExA(resKey, subValue, 0, REG_SZ, (const BYTE*)data.c_str(), dataLen);
    if(key == ERROR_SUCCESS)
    {
        return true;
    }
    else
    {
        return false;
    }
}


int main()
{
    Registry r;

    if(r.OpenKey() == true)
    {
        std::cout << "OpenKey() returned true!" << std::endl;
        if(r.GetValue() == true)
        {
            std::cout << "GetValue() returned true!" << std::endl;
        }
        else
        {
            std::cout << "GetValue() returned false!" << std::endl;
            if(r.WriteValue() == true)
            {
                std::cout << "WriteValue() returned true!" << std::endl;
            }
            else
            {
                std::cout << "WriteValue() returned false." << std::endl;
            }
        }
    }
    else
    {
        std::cout << "It returned false." << std::endl;
    }

    std::cin.get();

    return 0;
}

Thank you both for your help.

[andywestken]

One more minor comment...

There is nothing wrong with your code, but it is equivalent to (e.g.) :

bool Registry::WriteValue()
{
    const std::string data = "C:\\WINDOWS\\system32\\program.exe";
    DWORD dataLen = data.size()+1;

    long key = RegSetValueExA(resKey, subValue, 0, REG_SZ, (const BYTE*)data.c_str(), dataLen);
    return (key == ERROR_SUCCESS);
}

That is, the return value of operator== is already true or false.

And I added a const. If it doesn't change, it should be const!

Andy

P.S. I would return the Windows error rather than just true or false,so you can report the error to the user (if you want to). Changing if(r.GetValue() == true) to if(r.GetValue() == ERROR_SUCCESS) is no big deal.

[webJose]

You're still doing it wrong: You're storing ANSI strings in variables of type LPCTSTR, which is TCHAR-based, not char-based. Change LPCTSTR for LPCSTR.

You'll get the hang of this eventually. Want your compiler to show you where you're screwing up? #define UNICODE at the top and see all the warnings and errors you'll get.

Your goal: To be able to compile your code regardless of the UNICODE #definition. It must always compile with no errors or warnings with and without the #define UNICODE. When you achieve this, you wrote correct code.

[rtfm]

Ah, okay. Thank you, Andy. I'll keep that in mind.

Thanks, webJose. I'll read about it when I have time.