1
\$\begingroup\$

This is the function I coded, obviously used to get the system name:

const char* os(){
 OSVERSIONINFO os;
 ZeroMemory(&os, sizeof(OSVERSIONINFO));
 os.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
 GetVersionEx(&os);
 std::string ret = "Windows ";
 if (os.dwMajorVersion == 10)
 ret += "10";
 else if (os.dwMajorVersion == 6){
 if (os.dwMinorVersion == 3)
 ret += "8.1";
 else if (os.dwMinorVersion == 2)
 ret += "8";
 else if (os.dwMinorVersion == 1)
 ret += "7";
 else
 ret += "Vista";
 }
 else if (os.dwMajorVersion == 5){
 if (os.dwMinorVersion == 2)
 ret += "XP SP2";
 else if (os.dwMinorVersion == 1)
 ret += "XP";
 }
 return ret.c_str();
}

This function returns a the pointer to a memory buffer which is deallocated at the function leaving (object dynamically allocated in the heap, not in the stack)

200_success
146k22 gold badges190 silver badges479 bronze badges
asked Oct 17, 2015 at 21:48
\$\endgroup\$
2
  • \$\begingroup\$ So, it's obviously broken, and you know it. Why are you posting here then? \$\endgroup\$ Commented Oct 18, 2015 at 1:17
  • \$\begingroup\$ If you know this results in a dangling pointer, why don't you just return the string? \$\endgroup\$ Commented Oct 18, 2015 at 6:58

1 Answer 1

6
\$\begingroup\$

Aggregate initialization

Prefer SecureZeroMemory since according to MSDN, ZeroMemory can be optimized out. Or better yet, don't use a funky macro:

OSVERSIONINFO os = {};

Invalid pointer

This function returns a the pointer to a memory buffer which is deallocated at the function leaving (object dynamically allocated in the heap, not in the stack)

That's a lot of mumbo-jumbo for "this function returns a pointer to garbage". Just return a std::string and if the caller decides they want to do c_str(), let them.

Version numbers

Some operating systems will have identical version numbers. For example, both Windows 8 and Windows Server 2012 are 6.2. The recommended way to distinguish them is to use OSVERSIONINFOEX.wProductType == VER_NT_WORKSTATION (true for Windows 8, false for Windows Server 2012). A full table can be found here.

Better way?

Yes, the almighty documentation says:

To obtain the full version number for the operating system, call the GetFileVersionInfo function on one of the system DLLs, such as Kernel32.dll, then call VerQueryValue to obtain the \StringFileInfo\ProductVersion subblock of the file version information.

The order you must call the functions are GetFileVersionInfoSize, GetFileVersionInfo, then VerQueryValue.

answered Oct 18, 2015 at 1:50
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first answer. \$\endgroup\$ Commented Oct 18, 2015 at 1:52
  • \$\begingroup\$ Welcome to CR, and great answer! Small nitpick though: since the ZeroMemory call here is clearly affecting behavior, the compiler cannot legally optimize it out. SecureZeroMemory is typically used before you deallocate something that's consider sensitive (password, private key, etc). Since the memory is about to be released in that situation, the compiler can look at it and go "oh we're done with that anyway, so there's no reason to zero it out." In this case, that's not a concern since optimizing it out would break the program. \$\endgroup\$ Commented Oct 18, 2015 at 6:56

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.