C26485 and pointer decay with TCHAR in exception handler

Question

I don't understand this C26485 warning I receive.

My code is an exception handler:

catch (CDBException* e)
{
    TCHAR szError[_MAX_PATH];
    e->GetErrorMessage(szError, _MAX_PATH);
    AfxMessageBox(szError);
}

It is saying:

Expression szError: No array to pointer decay (bounds.3).

Both GetErrorMessage and AfxMessageBox are flagging this.

It doesn't make sense to me. Thanks for your help.

Answer 1

This diagnostic is an unfortunate result of the code analysis taking too narrow a view, ignoring hints that are readily available. C26485 warns against array-to-pointer decay, one of C++' most dangerous features. When passing the name of an array to a function that expects a pointer, the compiler silently converts the array into a pointer to its first element, thereby dropping the size information that's part of the array type.

Clients that call into an interface that accepts individual arguments (pointer and size) to describe an array must make sure that the size actually matches that of the array. This has caused countless CVE's, and there's no reason to believe that the situation is getting any better. Array-to-pointer decay is dangerous and having tooling guard against it is great. In theory.

Here, however, things are different. The declaration (and definition) of GetErrorMessage have SAL Annotations that allow the compiler to verify, that pointer and size do match, at compile time. The signature is as follows:

virtual BOOL GetErrorMessage(_Out_writes_z_(nMaxError) LPTSTR lpszError,
                             _In_ UINT nMaxError,
                             _Out_opt_ PUINT pnHelpContext = NULL) const;

The _Out_writes_z_(s) annotation establishes a compile-time verifiable relationship between the pointer lpszError and its corresponding array's size nMaxError. This is helpful information that should be taken advantage of whenever possible.

First, though, let's try to address the immediate issue, following the recommendation from the documentation:

An explicit cast to the decayed pointer type prevents the warning, but it doesn't prevent buggy code.

The most compact way to turn an array into a pointer to its first element is to literally just do that:

catch (CDBException* e)
{
    TCHAR szError[_MAX_PATH];
    e->GetErrorMessage(&szError[0], _MAX_PATH);
    AfxMessageBox(&szError[0]);
}

This fixes the immediate issue (on both function calls, incidentally, even if for different reasons). No more C26485's are issued, and—as an added bonus—passing an incorrect value as the second argument (e.g. _MAX_PATH + 1) does get the desired C6386 diagnostic ("buffer overrun").

This is crucially important, too, as a way of verifying correctness. If you were to use a more indirect way (say, by using a CString, as suggested here), you'd immediately give up on that compile-time verification. Using a CString is both computationally more expensive, and less secure.

As an alternative to the above, you could also temporarily suppress the C26485 diagnostic on both calls, e.g.

catch (CDBException* e)
{
    TCHAR szError[_MAX_PATH];
    // Decay is safe due to the _Out_writes_z_ annotation
    #pragma warning(suppress : 26485)
    e->GetErrorMessage(szError, _MAX_PATH);
    // Decay is safe; the previous call guarantees zero-termination
    #pragma warning(suppress : 26485)
    AfxMessageBox(szError);
}

Which of those implementations you choose is ultimately a matter of personal preference. Either one addresses the issue, with the latter being maybe a bit more preserving as far as code analysis goes.

A word on why the final call to AfxMessageBox is safe: It expects a zero-terminated string, and thus doesn't need an explicit size argument. The _Out_writes_z_(s) annotation on the GetErrorMessage signature makes the promise to always zero-terminate the output string on return. This, too, is verified at compile time, on both sides of the contract: Callers can rely on receiving a zero-terminated string, and the compiler makes sure that the implementation has no return path that violates this post-condition.

Answer 2

As suggested by Andrew, I should post working code here.

Solution without using TCHAR

catch (CDBException* e)
{
   e->ReportError();
   e->Delete();
}

.

catch (CException* e)
{
  CString strError;
  e->GetErrorMessage(strError.GetBufferSetLength(_MAX_PATH), _MAX_PATH);
  strError.ReleaseBuffer();
  AfxMessageBox(strError);
  e->Delete();
}

For the second example, selon IInsepctable this silences only the code analysis.

And to remember, usually you must delete the CException pointer, which you have forgotten here?