Open StephanTLavavej opened 1 month ago
__crtLCMapStringA
is used by _Strxfrm
, _Toupper
, and _Tolower
.
_Toupper
and _Tolower
each converts a single character. It should be extremely unlikely (if not impossible) for them to meet an allocation failure._Strxfrm
calls __crtLCMapStringA
twice. The first call is used to compute the required size without filling the output buffer, which means __crtLCMapStringA
is expected to return the required size regardless of whether it can write to the buffer.is expected to return the required size regardless of whether it can write to the buffer.
Ah, but we pass 0
for cchDest
when we're doing that. Thus, if (0 != cchDest)
is inactive in the SORTKEY block, which otherwise does nothing:
We then fall through to the end of the function, where we use the "successful" return retval;
- that's the only one I don't think is bogus.
Quote from the offline documentation I still use when the offline is bogus (as I thing currently happens)
Return Values Returns the number of characters or bytes in the translated string or sort key, including a terminating null character, if successful. If the function succeeds and the value of cchDest is 0, the return value is the size of the buffer required to hold the translated string or sort key, including a terminating null character.
This function returns 0 if it does not succeed. To get extended error information, the application can call GetLastError. GetLastError can return one of the following error codes:
- ERROR_INSUFFICIENT_BUFFER
- ERROR_INVALID_FLAGS
- ERROR_INVALID_PARAMETER
While fixing a CodeQL warning in
__crtLCMapStringA()
, I'm seeing code that makes no sense:https://github.com/microsoft/STL/blob/1e312b38db8df1dfbea17adc344454feb8d00dd9/stl/src/StlLCMapStringA.cpp#L28-L122
Specifically, every single
return retval;
(except for the last one, which is the success path) looks like it's a failure case where we should actually be returning 0 (failure).Early on, if we can't allocate an
inwbuffer
, we return 0, which makes total sense: https://github.com/microsoft/STL/blob/1e312b38db8df1dfbea17adc344454feb8d00dd9/stl/src/StlLCMapStringA.cpp#L62-L65But later, if we can't allocate an
outwbuffer
, wereturn retval;
, which makes no sense whatsoever: https://github.com/microsoft/STL/blob/1e312b38db8df1dfbea17adc344454feb8d00dd9/stl/src/StlLCMapStringA.cpp#L99-L102Similarly for the other cases.
Notes:
return 0;
without a thorough analysis and evidence that this won't damage behavior in some way._CRTIMP2
), but it's used only within the STL and is not user-visible. I believe it's never been user-visible in the VS 2015+ era (need to confirm this). If it was never user-visible, then we need to maintain its signature for dllexport validation but we need not preserve its behavior.LCMapStringEx
forLCMAP_SORTKEY
(this is what I'm looking at for CodeQL). Onlyxstrxfrm.cpp
has 2 calls withLCMAP_SORTKEY
, so if we ever gain the understanding/courage to mess with this, we should consider splitting up this function's behavior.