2
\$\begingroup\$

I'm using the WinAPI GetLogicalDriveStrings() function that requires a LPWSTR and am wondering if there is a safer way to do this to ensure there is no memory leak.

Currently I construct an initial pointer to a buffer buf using:

auto buf = GetLogicalDriveStrings(0, nullptr);

Then I create the LPWSTR to be used in lieu of the null pointer in my actual call using:

auto driveStrings = static_cast<LPWSTR>(malloc((buf + 1) * sizeof(WCHAR)));

Next I create a pointer to driveStrings to free it later. After checking if driveStrings is a null pointer or if the buffer (buf) is NULL (in case memory couldn't be allocated), I call GetLogicalDriveStrings() using driveStrings.

After I get the result I manually free() the LPWSTR using the pointer I made after allocating it.

How can I use a smart pointer for LPWSTR instead so I don't have to use malloc() and free(), but so that it will still work with the GetLogicalDriveStrings() function?

Mininum working example:

auto buf = GetLogicalDriveStrings(0, nullptr);

auto driveStrings = static_cast<LPWSTR>(malloc((buf + 1) * sizeof(WCHAR)));
auto pDriveStrings = driveStrings;

if (driveStrings == nullptr || buf == NULL)
{
    std::stringstream msg;
    msg << "Can't allocate memory for drive list: ";
    msg << GetLastError();
    throw std::runtime_error(msg.str());
}

// get drive strings
if (GetLogicalDriveStrings(buf, driveStrings) == NULL)
{
    std::stringstream msg;
    msg << "GetLogicalDriveStrings error: ";
    msg << GetLastError();
    throw std::runtime_error(msg.str());
}

// iterate over results
while (*driveStrings)
{
    // GetDriveType() requires a LPCWSTR
    if (GetDriveType(driveStrings) == DRIVE_FIXED || GetDriveType(driveStrings) == DRIVE_REMOVABLE)
    {
        std::wcout << driveStrings << std::endl;
    }
    driveStrings += lstrlen(driveStrings) + 1;
}

free(pDriveStrings);

If I use a std::wstring, I can't figure out how to iterate over each string in the driveStrings buffer. If I use a std::vector<WCHAR>, I can't figure out how to cast each element to an LPCWSTR for GetDriveType().

This works fine as is, but is there a better/safer way to do this? I'm open to any and all improvements.

Cross-posted on StackOverflow, but it has close votes because it is a better fit here.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review, and thank you for declaring your cross-post. For the record, I don't believe that your question was off-topic on Stack Overflow. \$\endgroup\$ Mar 3, 2017 at 20:47

1 Answer 1

2
\$\begingroup\$

Your code is leaking memory. If you throw an exception, free no longer gets called. Using a std::vector makes sure the memory is freed.

Iterating through the array returned is quite straight-forward, considering that each drive letter has a null-terminator and than the whole vector has an extra null-terminator (requiredSize = writtenChars + 1)

auto requiredSize = GetLogicalDriveStringsW(0, nullptr);

std::vector<WCHAR> buffer(requiredSize + 1);
auto writtenChars = GetLogicalDriveStringsW(requiredSize, buffer.data());

if (writtenChars > requiredSize)
{
    //MSDN: If the buffer is not large enough, the return value is greater than nBufferLength.
}

if (writtenChars == 0)
{
    //handle error
}

const WCHAR* currentString = buffer.data();

while (*currentString != 0)
{
    std::wcout << currentString << L'\n';
    //pass currentString to other methods
    currentString += wcslen(currentString) + 1;
}
\$\endgroup\$
4
  • \$\begingroup\$ Please note that GetDriveType requires an LPCWSTR, not just a string. Would just passing its reference work for that? \$\endgroup\$
    – Dan
    Mar 3, 2017 at 20:46
  • \$\begingroup\$ Won't I then need to free *currentString? Would a smart pointer be be better here? \$\endgroup\$
    – Dan
    Mar 3, 2017 at 21:18
  • \$\begingroup\$ No, you wouldn't. currentString doesn't own the memory it's pointing to, the std::vector does is is responsible for freeing it. \$\endgroup\$
    – D. Jurcau
    Mar 4, 2017 at 6:53
  • \$\begingroup\$ Ahh ok that makes sense. \$\endgroup\$
    – Dan
    Mar 4, 2017 at 13:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.