View Issue Details

IDProjectCategoryView StatusLast Update
00028202 - Next Dev List (Holding Area)Maintenancepublic2020-07-02 02:12
ReporterK7ZCZAssigned To 
Status newResolutionopen 
Summary0002820: All: many calls to wcstombs_s() are incorrect, all are unnecessary
wcstombs_s() converts a wide ("Unicode") string to a multi-byte ("ANSI") string. Searching the suite for this API reveals about 175 call sites. Most of those calls have problems. For example,

// protoype for reference
errno_t wcstombs_s(size_t *pReturnValue, char *mbstr, size_t sizeInBytes, const wchar_t *wcstr, size_t count);  

// actual app code
wcstombs_s(&size, szPathName, MAX_PATH, dlg.GetPathName(), MAX_PATH);

this call passes the same value in (MAX_PATH) for the source and target length. If the string were really as long as MAX_PATH, the function would fail because the count parameter does include the null terminator, while the sizeInBytes parameter does. The security features in this function cause it to throw a fatal exception if the length mismatch occurs. That has the symptom of terminating the application without showing the user any UI.

Another pattern is the use of an absurdly large stack-allocated buffer, like this example from CConnection::KenwoodReadString() in Rig Control:

    CHAR szCharCmd[0x10000];
#ifdef _UNICODE
        size_t size;
        wcstombs_s(&size, szCharCmd, ARRAYSIZE(szCharCmd), lpszCmd, wcslen(lpszCmd));
    StringCchCopyN(szCharCmd, ARRAYSIZE(szCharCmd), lpszCmd, strlen(lpszCmd));

MFC's CString hasbuilt-in handling for this conversion, as does the application's COwnString class. If the built-in handling is inadequate, it's much smarter to write reusable code and add it to COwnString (which is already used throughout the application) than it is to re-invent the wheel, mostly incorrectly, in 175 different places.

This issue is more than a coding style problem. Since the bad call sites have the potential to take down the application in a way that is very troubling for customers and isn't particularly easy to diagnose, we should come up with more sensible replacement.
TagsNo tags attached.
TestingNot Started


related to 0002923 closedK7ZCZ Ham Radio Deluxe HRDStation: unchecked call to mbstowcs_s() causes crashes 



2018-10-23 16:40

developer   ~0006341

The same problem exists with calls to mbstowcs_s(). This example is from HRDStats.cpp:

    WCHAR wszName[512];
    size_t size;
    mbstowcs_s(&size, wszName, 512, lpszName, 512);
    wszName[size] = '\0';

If a string of 512 characters in length arrives, the function will throw a fatal exception because size will be set to 512, wszName[512] doesn't exist, and the value will be trapped.

As with the wcstombs_s() pattern, this bogus code idiom is copied-and-pasted throughout the code. In some instances, it can be replaced with a CString call that is correct and encapsulates the implementation. In some instances, care must be given to computing the length of the input string -- if the input string is too long, the resulting converted string will probably also be too long. In almost all cases, the code doesn't check the length of any string and is not robust to buffer-overflow input.


2018-10-24 07:42

developer   ~0006344

incrementally addressed with these checkins; there is much more work to do:

Issue History

Date Modified Username Field Change
2018-07-28 11:16 K7ZCZ New Issue
2018-07-28 11:32 K7ZCZ File Added: Mantis2820.xlsx
2018-07-28 11:32 K7ZCZ File Deleted: Mantis2820.xlsx
2018-10-23 16:40 K7ZCZ Note Added: 0006341
2018-10-23 16:41 K7ZCZ Relationship added related to 0002923
2018-10-24 07:42 K7ZCZ Note Added: 0006344
2020-07-02 02:12 WA9PIE Project 3 - Current Dev List => 2 - Next Dev List (Holding Area)