microsoft / microsoft/STL

`StlLCMapStringA.cpp`: Does `__crtLCMapStringA()` have bogus error paths?

Open
#4,984 3 comments 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

question
Dominant language
C++
Stars
11.1k
Forks
1.7k
Avg merge
4d 15h
Merged PRs (30d)
22

Description

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-L65

But later, if we can't allocate an outwbuffer, we return retval;, which makes no sense whatsoever:
https://github.com/microsoft/STL/blob/1e312b38db8df1dfbea17adc344454feb8d00dd9/stl/src/StlLCMapStringA.cpp#L99-L102

Similarly for the other cases.

Notes:

  1. This code is very old and somewhat scary. We shouldn't mess with it until we have a crystal-clear understanding of what's going on, and ideally test cases that exercise various codepaths. Our test coverage here is very minimal.
    • I'm filing this issue as a reminder to thoroughly investigate this in the future - I specifically don't want to see a PR that just mechanically replaces lines with return 0; without a thorough analysis and evidence that this won't damage behavior in some way.
  2. We were confused long ago, and dllexported this (marked as _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.
    • I really don't like this function (beginning with the totally incorrect comment "Tries to use NLS API call LCMapStringA"), especially how it calls LCMapStringEx for LCMAP_SORTKEY (this is what I'm looking at for CodeQL). Only xstrxfrm.cpp has 2 calls with LCMAP_SORTKEY, so if we ever gain the understanding/courage to mess with this, we should consider splitting up this function's behavior.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with stl/src/StlLCMapStringA.cpp, especially __crtLCMapStringA() and the cited allocation and return paths. Trace its callers, including the LCMAP_SORTKEY uses in xstrxfrm.cpp, and determine whether the exported signature or behavior must be preserved. Add focused coverage for the relevant failure paths and document the evidence before changing any returns.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
localization, testing-qa
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.