c++ COM COMDLG_FILTERSPEC array overrun

Question

So I've got this function which will take a list of string pairs and generate a COMDLG_FILTERSPEC array. pairs are as such: first = "All Types" second = "*.*"

The function works however I get buffer overruns as shown here:

I also get this nice message telling me I'll get buffer overruns

I have no idea how to fix this or why it's overrunning. Can anyone help?

Here's the code:

COMDLG_FILTERSPEC * CreateFILTERSPEC(std::list<std::pair<std::wstring, std::wstring>> _filters) {

    //TODO: Causes memory leak on deletion. Fix it.

    COMDLG_FILTERSPEC* filterSpecs = new COMDLG_FILTERSPEC[_filters.size()];

    int i = 0;
    for (std::pair<std::wstring, std::wstring> filter : _filters) {


        PWSTR f1_p = new wchar_t[filter.first.length()];
        filter.first.copy(f1_p, filter.first.length());

        PWSTR f2_p = new wchar_t[filter.second.length()];
        filter.second.copy(f2_p, filter.second.length());

        COMDLG_FILTERSPEC fs = { f1_p, f2_p };

        filterSpecs[i] = fs;
        i++;
    }

    return filterSpecs;

}

Any help is appreciated, thanks.

Answer 1

The problem is not due to a buffer overflow, but rather that you are simply not null-terminating the wchar[] strings you are adding to the filter.

Per the std::basic_string::copy() documentation on cppreference.com:

Copies a substring [pos, pos+count) to character string pointed to by dest. If the requested substring lasts past the end of the string, or if count == npos, the copied substring is [pos, size()). The resulting character string is not null-terminated.

So, you need to add those null terminators to your strings, eg:

COMDLG_FILTERSPEC* CreateFILTERSPEC(const std::list<std::pair<std::wstring, std::wstring>> &_filters) {

    COMDLG_FILTERSPEC* filterSpecs = new COMDLG_FILTERSPEC[_filters.size()];
    COMDLG_FILTERSPEC* filterSpec = filterSpecs;

    for (const auto &filter : _filters) {

        PWSTR f1_p = new wchar_t[filter.first.length() + 1]; // <-- note the +1 !
        filter.first.copy(f1_p, filter.first.length());
        f1_p[filter.first.length()] = L'\0'; // <-- add this !

        PWSTR f2_p = new wchar_t[filter.second.length() + 1]; // <-- note the +1 !
        filter.second.copy(f2_p, filter.second.length());
        f2_p[filter.second.length()] = L'\0'; // <-- add this !

        filterSpec->pszName = f1_p;
        filterSpec->pszSpec = f2_p;
        ++filterSpec;
    }

    return filterSpecs;
}

COMDLG_FILTERSPEC *filterSpecs = CreateFILTERSPEC(filters);

// use filterSpecs as needed ...

for(int i = 0; i < filters.size(); ++i) {
    delete[] filterSpecs[i].pszName;
    delete[] filterSpecs[i].pszSpec;
};
delete[] filterSpecs;

Alternatively, you can combine all of the new[]'ed memory into a single allocation for easier cleanup, eg:

COMDLG_FILTERSPEC* CreateFILTERSPEC(const std::list<std::pair<std::wstring, std::wstring>> &_filters) {

    size_t size = sizeof(COMDLG_FILTERSPEC) * _filters.size();
    for (const auto &filter : _filters) {
        size += (filter.first.length() + filter.second.length() + 2);
    }

    COMDLG_FILTERSPEC* filterSpecs = reinterpret_cast<COMDLG_FILTERSPEC*>(new BYTE[size]);
    COMDLG_FILTERSPEC* filterSpec = filterSpecs;
    wchar_t *strData = reinterpret_cast<wchar_t*>(filterSpecs + _filters.size());

    for (const auto &filter : _filters) {

        filterSpec->pszName = strData;
        filter.first.copy(strData, filter.first.length());
        strData += filter.first.length();
        *strData++ = L'\0';

        filterSpec->pszSpec = strData;
        filter.second.copy(strData, filter.second.length());
        strData += filter.second.length();
        *strData++ = L'\0';

        ++filterSpec;
    }

    return filterSpecs;
}

COMDLG_FILTERSPEC *filterSpecs = CreateFILTERSPEC(filters);

// use filterSpecs as needed ...

delete[] reinterpret_cast<BYTE*>(filterSpecs);

However, if the contents of the std::list will persist beyond the lifetime of the COMDLG_FILTERSPEC, then you don't need to new[] any memory for the strings at all, just use the existing std::wstring memory as-is, eg:

COMDLG_FILTERSPEC * CreateFILTERSPEC(const std::list<std::pair<std::wstring, std::wstring>> &_filters) {

    COMDLG_FILTERSPEC* filterSpecs = new COMDLG_FILTERSPEC[_filters.size()];
    COMDLG_FILTERSPEC* filterSpec = filterSpecs;

    for (const auto &filter : _filters) {
        filterSpec->pszName = filter.first.c_str();
        filterSpec->pszSpec = filter.second.c_str();
        ++filterSpec;
    }

    return filterSpecs;
}

COMDLG_FILTERSPEC *filterSpecs = CreateFILTERSPEC(filters);

// use filterSpecs as needed ...

delete[] filterSpecs;

In which case, you should consider returning a std::unique_ptr<COMDLG_FILTERSPEC[]> rather than a raw COMDLG_FILTERSPEC* pointer, eg:

std::unique_ptr<COMDLG_FILTERSPEC[]> CreateFILTERSPEC(const std::list<std::pair<std::wstring, std::wstring>> &_filters) {

    auto filterSpecs = std::make_unique<COMDLG_FILTERSPEC[]>(_filters.size());
    COMDLG_FILTERSPEC* filterSpec = filterSpecs.get();

    for (const auto &filter : _filters) {
        filterSpec->pszName = filter.first.c_str();
        filterSpec->pszSpec = filter.second.c_str();
        ++filterSpec;
    }

    return filterSpecs;
}

auto filterSpecs = CreateFILTERSPEC(filters);

// use filterSpecs.get() as needed ...

// the COMDLG_FILTERSPEC memory is freed automatically when
// filterSpecs goes out of scope...

Or, return a std::vector instead, eg:

std::vector<COMDLG_FILTERSPEC> CreateFILTERSPEC(const std::list<std::pair<std::wstring, std::wstring>> &_filters) {

    std::vector<COMDLG_FILTERSPEC> filterSpecs(_filters.size());
    COMDLG_FILTERSPEC* filterSpec = filterSpecs.data();

    for (const auto &filter : _filters) {
        filterSpec->pszName = filter.first.c_str();
        filterSpec->pszSpec = filter.second.c_str();
        ++filterSpec;
    }

    return filterSpecs;
}

auto filterSpecs = CreateFILTERSPEC(filters);

// use filterSpecs.data() as needed ...

// the COMDLG_FILTERSPEC memory is freed automatically when
// filterSpecs goes out of scope...