View Issue Details

IDProjectCategoryView StatusLast Update
00028203 - Current Dev ListMaintenancepublic2018-10-24 07:42
ReporterK7ZCZAssigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Product Version6.4.0.873 
Target VersionFixed in Version 
Summary0002820: All: many calls to wcstombs_s() are incorrect, all are unnecessary
Description
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));
    }
#else
    StringCchCopyN(szCharCmd, ARRAYSIZE(szCharCmd), lpszCmd, strlen(lpszCmd));
#endif


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.
ModuleAll
Sub-ModuleVarious
TestingNot Started

Relationships

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

Activities

K7ZCZ

2018-10-23 16:40

administrator   ~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.

K7ZCZ

2018-10-24 07:42

administrator   ~0006344

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

https://hrdsoftware.visualstudio.com/HRD/_versionControl/changeset/4393
https://hrdsoftware.visualstudio.com/HRD/_versionControl/changeset/4394

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