ICU-21749 Fix stack-use-after-scope bug in uloc

See #1858

diff --git a/icu4c/source/common/uloc.cpp b/icu4c/source/common/uloc.cpp
index 411b2cac936d91adcbe2b06dd22ca0330a539134..c8a3f1ff7313408854857dc55d3a482c1b00c486 100644 (file)
--- a/icu4c/source/common/uloc.cpp
+++ b/icu4c/source/common/uloc.cpp
@@ -478,8 +478,10 @@ static const CanonicalizationMap CANONICALIZE_MAP[] = {
 /* Test if the locale id has BCP47 u extension and does not have '@' */
 #define _hasBCP47Extension(id) (id && uprv_strstr(id, "@") == NULL && getShortestSubtagLength(localeID) == 1)
 /* Converts the BCP47 id to Unicode id. Does nothing to id if conversion fails */
-static int32_t _ConvertBCP47(
-            const char*& finalID, const char* id, char* buffer, int32_t length, UErrorCode* err) {
+static const char* _ConvertBCP47(
+        const char* id, char* buffer, int32_t length,
+        UErrorCode* err, int32_t* pLocaleIdSize) {
+    const char* finalID;
     int32_t localeIDSize = uloc_forLanguageTag(id, buffer, length, NULL, err);
     if (localeIDSize <= 0 || U_FAILURE(*err) || *err == U_STRING_NOT_TERMINATED_WARNING) {
         finalID=id;
@@ -489,7 +491,10 @@ static int32_t _ConvertBCP47(
     } else {
         finalID=buffer;
     }
-    return localeIDSize;
+    if (pLocaleIdSize != nullptr) {
+        *pLocaleIdSize = localeIDSize;
+    }
+    return finalID;
 }
 /* Gets the size of the shortest subtag in the given localeID. */
 static int32_t getShortestSubtagLength(const char *localeID) {
@@ -771,7 +776,8 @@ ulocimp_getKeywordValue(const char* localeID,
       }
 
       if (_hasBCP47Extension(localeID)) {
-          _ConvertBCP47(tmpLocaleID, localeID, tempBuffer, sizeof(tempBuffer), status);
+          tmpLocaleID = _ConvertBCP47(localeID, tempBuffer,
+                                      sizeof(tempBuffer), status, nullptr);
       } else {
           tmpLocaleID=localeID;
       }
@@ -1408,10 +1414,11 @@ uloc_openKeywords(const char* localeID,
     }
 
     if (_hasBCP47Extension(localeID)) {
-        _ConvertBCP47(tmpLocaleID, localeID, tempBuffer, sizeof(tempBuffer), status);
+        tmpLocaleID = _ConvertBCP47(localeID, tempBuffer,
+                                    sizeof(tempBuffer), status, nullptr);
     } else {
         if (localeID==NULL) {
-           localeID=uloc_getDefault();
+            localeID=uloc_getDefault();
         }
         tmpLocaleID=localeID;
     }
@@ -1483,13 +1490,13 @@ _canonicalize(const char* localeID,
 
     int32_t j, fieldCount=0, scriptSize=0, variantSize=0;
     PreflightingLocaleIDBuffer tempBuffer;  // if localeID has a BCP47 extension, tmpLocaleID points to this
+    CharString localeIDWithHyphens;  // if localeID has a BPC47 extension and have _, tmpLocaleID points to this
     const char* origLocaleID;
     const char* tmpLocaleID;
     const char* keywordAssign = NULL;
     const char* separatorIndicator = NULL;
 
     if (_hasBCP47Extension(localeID)) {
-        CharString localeIDWithHyphens;
         const char* localeIDPtr = localeID;
 
         // convert all underbars to hyphens, unless the "BCP47 extension" comes at the beginning of the string
@@ -1504,10 +1511,13 @@ _canonicalize(const char* localeID,
                 localeIDPtr = localeIDWithHyphens.data();
             }
         }
-        
+
         do {
-            tempBuffer.requestedCapacity = _ConvertBCP47(tmpLocaleID, localeIDPtr, tempBuffer.getBuffer(),
-                                                         tempBuffer.getCapacity(), err);
+            // After this call tmpLocaleID may point to localeIDPtr which may
+            // point to either localeID or localeIDWithHyphens.data().
+            tmpLocaleID = _ConvertBCP47(localeIDPtr, tempBuffer.getBuffer(),
+                                        tempBuffer.getCapacity(), err,
+                                        &(tempBuffer.requestedCapacity));
         } while (tempBuffer.needToTryAgain(err));
     } else {
         if (localeID==NULL) {
@@ -1794,7 +1804,7 @@ uloc_getVariant(const char* localeID,
     }
 
     if (_hasBCP47Extension(localeID)) {
-        _ConvertBCP47(tmpLocaleID, localeID, tempBuffer, sizeof(tempBuffer), err);
+        tmpLocaleID =_ConvertBCP47(localeID, tempBuffer, sizeof(tempBuffer), err, nullptr);
     } else {
         if (localeID==NULL) {
            localeID=uloc_getDefault();

diff --git a/icu4c/source/test/cintltst/cloctst.c b/icu4c/source/test/cintltst/cloctst.c
index 121d295d3dbe39442ac2aab17c7e284d2b5d7961..8707babbbee983bd00446aec1a37e0cc601de726 100644 (file)
--- a/icu4c/source/test/cintltst/cloctst.c
+++ b/icu4c/source/test/cintltst/cloctst.c
@@ -243,6 +243,7 @@ void addLocaleTest(TestNode** root)
     TESTCASE(TestKeywordSet);
     TESTCASE(TestKeywordSetError);
     TESTCASE(TestDisplayKeywords);
+    TESTCASE(TestCanonicalization21749StackUseAfterScope);
     TESTCASE(TestDisplayKeywordValues);
     TESTCASE(TestGetBaseName);
 #if !UCONFIG_NO_FILE_IO
@@ -2495,6 +2496,19 @@ static void TestCanonicalizationBuffer(void)
     }
 }
 
+static void TestCanonicalization21749StackUseAfterScope(void)
+{
+    UErrorCode status = U_ZERO_ERROR;
+    char buffer[256];
+    const char* input = "- _";
+    uloc_canonicalize(input, buffer, -1, &status);
+    if (U_SUCCESS(status)) {
+        log_err("FAIL: uloc_canonicalize(%s) => %s, expected U_FAILURE()\n",
+                input, u_errorName(status));
+        return;
+    }
+}
+
 static void TestDisplayKeywords(void)
 {
     int32_t i;

diff --git a/icu4c/source/test/cintltst/cloctst.h b/icu4c/source/test/cintltst/cloctst.h
index e6597e0ce422c9ad23cb89809b26b7ffd9a437c1..9e8e547f7250bcdca3b72a8245f232e79f3dad28 100644 (file)
--- a/icu4c/source/test/cintltst/cloctst.h
+++ b/icu4c/source/test/cintltst/cloctst.h
@@ -88,6 +88,7 @@ static void TestGetAvailableLocalesByType(void);
 
  static void TestCanonicalization(void);
  static void TestCanonicalizationBuffer(void);
+static  void TestCanonicalization21749StackUseAfterScope(void);
 
  static void TestDisplayKeywords(void);