Issue 2301 – Some Unicode texts, notably custom command names, were saved incorrectly in an INI file

https://winscp.net/tracker/2301

Source commit: d4e2d35de89e549991b2d661c86bce8f52b27f3f

source/core/Common.cpp

@@ -1334,7 +1334,7 @@ UnicodeString __fastcall DisplayableStr(const RawByteString & Str)
   int Index = 1;
   while ((Index <= Str.Length()) && Displayable)
   {
-    if (((Str[Index] < '\x20') || (static_cast<unsigned char>(Str[Index]) >= static_cast<unsigned char>('\x80'))) &&
+    if (((Str[Index] < '\x20') || IsWideChar(Str[Index])) &&
         (Str[Index] != '\n') && (Str[Index] != '\r') && (Str[Index] != '\t') && (Str[Index] != '\b'))
     {
       Displayable = false;

source/core/Common.h

@@ -110,6 +110,7 @@ int __fastcall CompareNumber(__int64 Value1, __int64 Value2);
 bool ContainsTextSemiCaseSensitive(const UnicodeString & Text, const UnicodeString & SubText);
 bool __fastcall IsReservedName(UnicodeString FileName);
 UnicodeString __fastcall ApiPath(UnicodeString Path);
+bool IsWideChar(wchar_t Ch) { return static_cast<unsigned char>(Ch) >= static_cast<unsigned char>('\x80'); };
 UnicodeString __fastcall DisplayableStr(const RawByteString & Str);
 UnicodeString __fastcall ByteToHex(unsigned char B, bool UpperCase = true);
 UnicodeString __fastcall BytesToHex(const unsigned char * B, size_t Length, bool UpperCase = true, wchar_t Separator = L'\0');

source/core/HierarchicalStorage.cpp

@@ -21,21 +21,8 @@
 #define WRITE_REGISTRY(Method) \
   try { FRegistry->Method(Name, Value); } catch(...) { }
 //---------------------------------------------------------------------------
-UnicodeString __fastcall MungeStr(const UnicodeString & Str, bool ForceAnsi, bool Value)
+UnicodeString PuttyEscape(const RawByteString & Source, bool Value)
 {
-  RawByteString Source;
-  if (ForceAnsi)
-  {
-    Source = RawByteString(PuttyStr(Str));
-  }
-  else
-  {
-    Source = RawByteString(UTF8String(Str));
-    if (Source.Length() > Str.Length())
-    {
-      Source.Insert(Bom, 1);
-    }
-  }
   strbuf * sb = strbuf_new();
   escape_registry_key(Source.c_str(), sb);
   RawByteString Dest(sb->s);
@@ -48,6 +35,31 @@ UnicodeString __fastcall MungeStr(const UnicodeString & Str, bool ForceAnsi, boo
   return UnicodeString(Dest.c_str(), Dest.Length());
 }
 //---------------------------------------------------------------------------
+bool ToUnicode(const UnicodeString & Str, RawByteString & Utf)
+{
+  Utf = RawByteString(UTF8String(Str));
+  bool Result = (Utf.Length() > Str.Length());
+  if (Result)
+  {
+    Utf.Insert(Bom, 1);
+  }
+  return Result;
+}
+//---------------------------------------------------------------------------
+UnicodeString __fastcall MungeStr(const UnicodeString & Str, bool ForceAnsi, bool Value)
+{
+  RawByteString Source;
+  if (ForceAnsi)
+  {
+    Source = RawByteString(PuttyStr(Str));
+  }
+  else
+  {
+    ToUnicode(Str, Source);
+  }
+  return PuttyEscape(Source, Value);
+}
+//---------------------------------------------------------------------------
 UnicodeString __fastcall UnMungeStr(const UnicodeString & Str)
 {
   // Str should contain ASCII characters only
@@ -79,32 +91,53 @@ UnicodeString __fastcall PuttyMungeStr(const UnicodeString & Str)
   return MungeStr(Str, true, false);
 }
 //---------------------------------------------------------------------------
-UnicodeString __fastcall MungeIniName(const UnicodeString & Str)
+UnicodeString __fastcall MungeIniName(const UnicodeString & Str, bool ForceAnsi)
 {
-  int P = Str.Pos(L"=");
-  // make this fast for now
-  if (P > 0)
+  bool NeedEscaping = false;
+  for (int Index = 1; !NeedEscaping && (Index <= Str.Length()); Index++)
+  {
+    wchar_t Ch = Str[Index];
+    NeedEscaping = IsWideChar(Ch) || (Ch == L'=');
+  }
+  UnicodeString Result;
+  if (!ForceAnsi && NeedEscaping)
   {
-    return ReplaceStr(Str, L"=", L"%3D");
+    RawByteString Utf;
+    ToUnicode(Str, Utf);
+    Result = PuttyEscape(Utf, false);
   }
   else
   {
-    return Str;
+    Result = Str;
   }
+  Result = ReplaceStr(Result, L"=", L"%3D");
+  return Result;
 }
 //---------------------------------------------------------------------------
+UnicodeString EscapedBom(TraceInitStr(PuttyEscape(Bom, false)));
+//---------------------------------------------------------------------------
 UnicodeString __fastcall UnMungeIniName(const UnicodeString & Str)
 {
-  int P = Str.Pos(L"%3D");
-  // make this fast for now
-  if (P > 0)
+  UnicodeString Result;
+  if (StartsStr(EscapedBom, Str))
   {
-    return ReplaceStr(Str, L"%3D", L"=");
+    Result = UnMungeStr(Str);
   }
   else
   {
-    return Str;
+    // Backward compatibility only, with versions that did not Unicode-encoded strings with =.
+    // Can be dropped eventually.
+    int P = Str.Pos(L"%3D");
+    if (P > 0)
+    {
+      Result = ReplaceStr(Str, L"%3D", L"=");
+    }
+    else
+    {
+      Result = Str;
+    }
   }
+  return Result;
 }
 //---------------------------------------------------------------------------
 template<typename T>
@@ -235,6 +268,17 @@ UnicodeString __fastcall THierarchicalStorage::MungeKeyName(const UnicodeString
   return Result;
 }
 //---------------------------------------------------------------------------
+UnicodeString THierarchicalStorage::MungeIniName(const UnicodeString & Str)
+{
+  UnicodeString Result = ::MungeIniName(Str, ForceAnsi);
+  // This does not handle Ansi-encoded value with equal sign, but that's an edge case
+  if ((Result != Str) && !ForceAnsi && CanRead() && DoValueExists(Str, true))
+  {
+    Result = ::MungeIniName(Str, true);
+  }
+  return Result;
+}
+//---------------------------------------------------------------------------
 UnicodeString __fastcall THierarchicalStorage::DoReadRootAccessString()
 {
   UnicodeString Result;
@@ -507,7 +551,7 @@ bool __fastcall THierarchicalStorage::ValueExists(const UnicodeString & Value)
 {
   if (CanRead())
   {
-    return DoValueExists(Value);
+    return DoValueExists(Value, ForceAnsi);
   }
   else
   {
@@ -1029,8 +1073,9 @@ bool __fastcall TRegistryStorage::DoKeyExists(const UnicodeString & SubKey, bool
   return Result;
 }
 //---------------------------------------------------------------------------
-bool __fastcall TRegistryStorage::DoValueExists(const UnicodeString & Value)
+bool __fastcall TRegistryStorage::DoValueExists(const UnicodeString & Value, bool DebugUsedArg(AForceAnsi))
 {
+  // TODO: use AForceAnsi
   bool Result = FRegistry->ValueExists(Value);
   return Result;
 }
@@ -1325,7 +1370,9 @@ void __fastcall TCustomIniFileStorage::DoGetValueNames(TStrings * Strings)
   FIniFile->ReadSection(CurrentSection, Strings);
   for (int Index = 0; Index < Strings->Count; Index++)
   {
-    Strings->Strings[Index] = UnMungeIniName(Strings->Strings[Index]);
+    UnicodeString S = Strings->Strings[Index];
+    S = UnMungeIniName(S);
+    Strings->Strings[Index] = S;
   }
 }
 //---------------------------------------------------------------------------
@@ -1336,16 +1383,16 @@ bool __fastcall TCustomIniFileStorage::DoKeyExists(const UnicodeString & SubKey,
     DoKeyExistsInternal(MungeStr(SubKey, AForceAnsi, false));
 }
 //---------------------------------------------------------------------------
-bool __fastcall TCustomIniFileStorage::DoValueExistsInternal(const UnicodeString & Value)
+bool __fastcall TCustomIniFileStorage::DoValueExistsInternal(const UnicodeString & Value, bool AForceAnsi)
 {
-  return FIniFile->ValueExists(CurrentSection, MungeIniName(Value));
+  return FIniFile->ValueExists(CurrentSection, ::MungeIniName(Value, AForceAnsi));
 }
 //---------------------------------------------------------------------------
-bool __fastcall TCustomIniFileStorage::DoValueExists(const UnicodeString & Value)
+bool __fastcall TCustomIniFileStorage::DoValueExists(const UnicodeString & Value, bool AForceAnsi)
 {
   return
-    (HandleByMasterStorage() && FMasterStorage->ValueExists(Value)) ||
-    DoValueExistsInternal(Value);
+    (HandleByMasterStorage() && FMasterStorage->DoValueExists(Value, AForceAnsi)) ||
+    DoValueExistsInternal(Value, AForceAnsi);
 }
 //---------------------------------------------------------------------------
 bool __fastcall TCustomIniFileStorage::DoDeleteValue(const UnicodeString & Name)
@@ -1369,7 +1416,7 @@ bool __fastcall TCustomIniFileStorage::HandleByMasterStorage()
 //---------------------------------------------------------------------------
 bool __fastcall TCustomIniFileStorage::HandleReadByMasterStorage(const UnicodeString & Name)
 {
-  return HandleByMasterStorage() && !DoValueExistsInternal(Name);
+  return HandleByMasterStorage() && !DoValueExistsInternal(Name, ForceAnsi);
 }
 //---------------------------------------------------------------------------
 size_t __fastcall TCustomIniFileStorage::DoBinaryDataSize(const UnicodeString & Name)

source/core/HierarchicalStorage.h

@@ -101,13 +101,14 @@ protected:
   UnicodeString __fastcall GetCurrentSubKeyMunged();
   virtual void __fastcall SetAccessMode(TStorageAccessMode value);
   virtual bool __fastcall DoKeyExists(const UnicodeString & SubKey, bool ForceAnsi) = 0;
-  virtual bool __fastcall DoValueExists(const UnicodeString & Value) = 0;
+  virtual bool __fastcall DoValueExists(const UnicodeString & Value, bool ForceAnsi) = 0;
   static UnicodeString __fastcall IncludeTrailingBackslash(const UnicodeString & S);
   static UnicodeString __fastcall ExcludeTrailingBackslash(const UnicodeString & S);
   virtual bool __fastcall DoOpenSubKey(const UnicodeString & SubKey, bool CanCreate) = 0;
   virtual void __fastcall DoCloseSubKey() = 0;
   bool MungingKeyName(const UnicodeString & Key);
   UnicodeString __fastcall MungeKeyName(const UnicodeString & Key);
+  UnicodeString MungeIniName(const UnicodeString & Str);
   virtual UnicodeString __fastcall GetSource() = 0;
   virtual bool __fastcall GetTemporary();
   virtual bool __fastcall DoDeleteSubKey(const UnicodeString & SubKey) = 0;
@@ -166,7 +167,7 @@ public:
 protected:
   virtual void __fastcall SetAccessMode(TStorageAccessMode value);
   virtual bool __fastcall DoKeyExists(const UnicodeString & SubKey, bool ForceAnsi);
-  virtual bool __fastcall DoValueExists(const UnicodeString & Value);
+  virtual bool __fastcall DoValueExists(const UnicodeString & Value, bool ForceAnsi);
   virtual bool __fastcall DoOpenSubKey(const UnicodeString & SubKey, bool CanCreate);
   virtual void __fastcall DoCloseSubKey();
   virtual UnicodeString __fastcall GetSource();
@@ -211,7 +212,7 @@ private:
   UnicodeString __fastcall GetCurrentSection();
   inline bool __fastcall HandleByMasterStorage();
   inline bool __fastcall HandleReadByMasterStorage(const UnicodeString & Name);
-  inline bool __fastcall DoValueExistsInternal(const UnicodeString & Value);
+  inline bool __fastcall DoValueExistsInternal(const UnicodeString & Value, bool ForceAnsi);
   void __fastcall DoWriteStringRawInternal(const UnicodeString & Name, const UnicodeString & Value);
   int __fastcall DoReadIntegerWithMapping(const UnicodeString & Name, int Default, const TIntMapping * Mapping);
 
@@ -225,7 +226,7 @@ protected:
   __property UnicodeString CurrentSection = { read = GetCurrentSection };
   virtual void __fastcall SetAccessMode(TStorageAccessMode value);
   virtual bool __fastcall DoKeyExists(const UnicodeString & SubKey, bool ForceAnsi);
-  virtual bool __fastcall DoValueExists(const UnicodeString & Value);
+  virtual bool __fastcall DoValueExists(const UnicodeString & Value, bool ForceAnsi);
   virtual bool __fastcall DoOpenSubKey(const UnicodeString & SubKey, bool CanCreate);
   virtual void __fastcall DoCloseSubKey();
   virtual UnicodeString __fastcall GetSource();