Fix AssemblyName(string) constructor's version parsing (#13373)

Fix AssemblyName(string) constructor's version parsing

Functional fix for https://github.com/dotnet/corefx/issues/22663
- Allow fewer version components
- Match .NET Framework behavior in several cases. Major and minor version must be specified for the version to be used.
- Used zero for unspecified build/revision. This is different from .NET Framework but the loader also behaves differently. Details are in code comments.

5 files changed, 151 insertions, 157 deletions

diff --git a/src/binder/assemblybinder.cpp b/src/binder/assemblybinder.cpp
index 73ea025ae8..7c0aa8077e 100644
--- a/src/binder/assemblybinder.cpp
+++ b/src/binder/assemblybinder.cpp
@@ -89,38 +89,66 @@ namespace BINDER_SPACE
             AssemblyVersion *pRequestedVersion = pRequestedName->GetVersion();
             AssemblyVersion *pFoundVersion = pFoundName->GetVersion();
 
-            //
-            // If the AssemblyRef has no version, we can treat it as requesting the most accommodating version (0.0.0.0). In
-            // that case, skip version checking and allow the bind.
-            //
-            if (!pRequestedName->HaveAssemblyVersion())
+            do
             {
-                return hr;
-            }
+                if (!pRequestedVersion->HasMajor())
+                {
+                    // An unspecified requested version component matches any value for the same component in the found version,
+                    // regardless of lesser-order version components
+                    break;
+                }
+                if (!pFoundVersion->HasMajor() || pRequestedVersion->GetMajor() > pFoundVersion->GetMajor())
+                {
+                    // - A specific requested version component does not match an unspecified value for the same component in
+                    //   the found version, regardless of lesser-order version components
+                    // - Or, the requested version is greater than the found version
+                    hr = FUSION_E_APP_DOMAIN_LOCKED;
+                    break;
+                }
+                if (pRequestedVersion->GetMajor() < pFoundVersion->GetMajor())
+                {
+                    // The requested version is less than the found version
+                    break;
+                }
 
-            //
-            // This if condition is paired with the one above that checks for pRequestedName
-            // not having an assembly version.  If we didn't exit in the above if condition,
-            // and satisfy this one's requirements, we're in a situation where the assembly
-            // Ref has a version, but the Def doesn't, which cannot succeed a bind
-            //
-            _ASSERTE(pRequestedName->HaveAssemblyVersion());
-            if (!pFoundName->HaveAssemblyVersion())
-            {
-                hr = FUSION_E_APP_DOMAIN_LOCKED;
-            }
-            else if (pRequestedVersion->IsEqualFeatureVersion(pFoundVersion))
-            {
-                // Now service version matters
-                if (pRequestedVersion->IsLargerServiceVersion(pFoundVersion))
+                if (!pRequestedVersion->HasMinor())
+                {
+                    break;
+                }
+                if (!pFoundVersion->HasMinor() || pRequestedVersion->GetMinor() > pFoundVersion->GetMinor())
                 {
                     hr = FUSION_E_APP_DOMAIN_LOCKED;
+                    break;
                 }
-            }
-            else if (pRequestedVersion->IsLargerFeatureVersion(pFoundVersion))
-            {
-                hr = FUSION_E_APP_DOMAIN_LOCKED;
-            }
+                if (pRequestedVersion->GetMinor() < pFoundVersion->GetMinor())
+                {
+                    break;
+                }
+
+                if (!pRequestedVersion->HasBuild())
+                {
+                    break;
+                }
+                if (!pFoundVersion->HasBuild() || pRequestedVersion->GetBuild() > pFoundVersion->GetBuild())
+                {
+                    hr = FUSION_E_APP_DOMAIN_LOCKED;
+                    break;
+                }
+                if (pRequestedVersion->GetBuild() < pFoundVersion->GetBuild())
+                {
+                    break;
+                }
+
+                if (!pRequestedVersion->HasRevision())
+                {
+                    break;
+                }
+                if (!pFoundVersion->HasRevision() || pRequestedVersion->GetRevision() > pFoundVersion->GetRevision())
+                {
+                    hr = FUSION_E_APP_DOMAIN_LOCKED;
+                    break;
+                }
+            } while (false);
 
             if (pApplicationContext->IsTpaListProvided() && hr == FUSION_E_APP_DOMAIN_LOCKED)
             {
diff --git a/src/binder/inc/assemblyname.inl b/src/binder/inc/assemblyname.inl
index 3acd39ce0a..796b33661d 100644
--- a/src/binder/inc/assemblyname.inl
+++ b/src/binder/inc/assemblyname.inl
@@ -133,7 +133,7 @@ void AssemblyName::SetHave(DWORD dwIdentityFlags)
 
 BOOL AssemblyName::HaveAssemblyVersion()
 {
-    return (m_version.GetMajor() != static_cast<DWORD>(-1));
+    return m_version.HasMajor();
 }
 
 BOOL AssemblyName::HaveNeutralCulture()
diff --git a/src/binder/inc/assemblyversion.hpp b/src/binder/inc/assemblyversion.hpp
index 20ef4ee48c..a6be617a30 100644
--- a/src/binder/inc/assemblyversion.hpp
+++ b/src/binder/inc/assemblyversion.hpp
@@ -22,10 +22,19 @@ namespace BINDER_SPACE
 {
     class AssemblyVersion
     {
+    private:
+        static const DWORD Unspecified = (DWORD)-1;
+        static const USHORT UnspecifiedShort = (USHORT)-1;
+
     public:
         inline AssemblyVersion();
         inline ~AssemblyVersion();
 
+        inline BOOL HasMajor();
+        inline BOOL HasMinor();
+        inline BOOL HasBuild();
+        inline BOOL HasRevision();
+
         inline DWORD GetMajor();
         inline DWORD GetMinor();
         inline DWORD GetBuild();
@@ -35,19 +44,10 @@ namespace BINDER_SPACE
                                       /* in */ DWORD dwMinor);
         inline void SetServiceVersion(/* in */ DWORD dwBuild,
                                       /* in */ DWORD dwRevision);
-        inline BOOL SetServiceVersion(/* in */ LPCWSTR pwzVersionStr);
-        inline BOOL SetVersion(/* in */ LPCWSTR pwzVersionStr);
         inline void SetVersion(AssemblyVersion *pAssemblyVersion);
 
-        inline BOOL IsLargerFeatureVersion(/* in */ AssemblyVersion *pAssemblyVersion);
-        inline BOOL IsEqualFeatureVersion(/* in */ AssemblyVersion *pAssemblyVersion);
-        inline BOOL IsSmallerFeatureVersion(/* in */ AssemblyVersion *pAssemblyVersion);
-        inline BOOL IsEqualServiceVersion(/* in */ AssemblyVersion *pAssemblyVersion);
-        inline BOOL IsLargerServiceVersion(/* in */ AssemblyVersion *pAssemblyVersion);
         inline BOOL Equals(AssemblyVersion *pAssemblyVersion);
-        inline BOOL IsSmallerOrEqual(AssemblyVersion *pAssemblyVersion);
-        inline BOOL IsLargerOrEqual(AssemblyVersion *pAssemblyVersion);
-    protected:
+    private:
         DWORD m_dwMajor;
         DWORD m_dwMinor;
         DWORD m_dwBuild;
diff --git a/src/binder/inc/assemblyversion.inl b/src/binder/inc/assemblyversion.inl
index 795f8c1765..d093808a1a 100644
--- a/src/binder/inc/assemblyversion.inl
+++ b/src/binder/inc/assemblyversion.inl
@@ -25,6 +25,26 @@ AssemblyVersion::~AssemblyVersion()
     // Noting to do here
 }
 
+BOOL AssemblyVersion::HasMajor()
+{
+    return m_dwMajor != Unspecified;
+}
+
+BOOL AssemblyVersion::HasMinor()
+{
+    return m_dwMinor != Unspecified;
+}
+
+BOOL AssemblyVersion::HasBuild()
+{
+    return m_dwBuild != Unspecified;
+}
+
+BOOL AssemblyVersion::HasRevision()
+{
+    return m_dwRevision != Unspecified;
+}
+
 DWORD AssemblyVersion::GetMajor()
 {
     return m_dwMajor;
@@ -48,22 +68,21 @@ DWORD AssemblyVersion::GetRevision()
 void AssemblyVersion::SetFeatureVersion(DWORD dwMajor,
                                         DWORD dwMinor)
 {
-    m_dwMajor = dwMajor;
-    m_dwMinor = dwMinor;
+    // BaseAssemblySpec and AssemblyName properties store uint16 components for the version. Version and AssemblyVersion store
+    // int32 or uint32. When the former are initialized from the latter, the components are truncated to uint16 size. When the
+    // latter are initialized from the former, they are zero-extended to int32 size. For uint16 components, the max value is
+    // used to indicate an unspecified component. For int32 components, -1 is used. Since we're treating the version as an
+    // assembly version here, map the uint16 unspecified value to the int32 size.
+    m_dwMajor = dwMajor == UnspecifiedShort ? Unspecified : dwMajor;
+    m_dwMinor = dwMinor == UnspecifiedShort ? Unspecified : dwMinor;
 }
 
 void AssemblyVersion::SetServiceVersion(DWORD dwBuild,
                                         DWORD dwRevision)
 {
-    m_dwBuild = dwBuild;
-    m_dwRevision = dwRevision;
-}
-
-BOOL AssemblyVersion::SetVersion(LPCWSTR pwzVersionStr)
-{
-    SmallStackSString versionString(pwzVersionStr);
-
-    return TextualIdentityParser::ParseVersion(versionString, this);
+    // See comment in SetFeatureVersion, the same applies here
+    m_dwBuild = dwBuild == UnspecifiedShort ? Unspecified : dwBuild;
+    m_dwRevision = dwRevision == UnspecifiedShort ? Unspecified : dwRevision;
 }
 
 void AssemblyVersion::SetVersion(AssemblyVersion *pAssemblyVersion)
@@ -74,83 +93,6 @@ void AssemblyVersion::SetVersion(AssemblyVersion *pAssemblyVersion)
     m_dwRevision = pAssemblyVersion->GetRevision();
 }
 
-BOOL AssemblyVersion::IsLargerFeatureVersion(AssemblyVersion *pAssemblyVersion)
-{
-    BOOL result = FALSE;
-
-    if (GetMajor() > pAssemblyVersion->GetMajor())
-    {
-        result = TRUE;
-    }
-    else if ((GetMajor() == pAssemblyVersion->GetMajor()) &&
-             (GetMinor() > pAssemblyVersion->GetMinor()))
-    {
-        result = TRUE;
-    }
-
-    return result;
-}
-
-BOOL AssemblyVersion::IsEqualFeatureVersion(AssemblyVersion *pAssemblyVersion)
-{
-    BOOL result = FALSE;
-
-    if ((GetMajor() == pAssemblyVersion->GetMajor()) &&
-        (GetMinor() == pAssemblyVersion->GetMinor()))
-    {
-        result = TRUE;
-    }
-
-    return result;
-}
-
-BOOL AssemblyVersion::IsSmallerFeatureVersion(AssemblyVersion *pAssemblyVersion)
-{
-    BOOL result = FALSE;
-
-    if (GetMajor() < pAssemblyVersion->GetMajor())
-    {
-        result = TRUE;
-    }
-    else if ((GetMajor() == pAssemblyVersion->GetMajor()) &&
-             (GetMinor() < pAssemblyVersion->GetMinor()))
-    {
-        result = TRUE;
-    }
-
-    return result;
-}
-
-BOOL AssemblyVersion::IsEqualServiceVersion(AssemblyVersion *pAssemblyVersion)
-{
-    BOOL result = FALSE;
-
-    if ((GetBuild() == pAssemblyVersion->GetBuild()) &&
-        (GetRevision() == pAssemblyVersion->GetRevision()))
-    {
-        result = TRUE;
-    }
-
-    return result;
-}
-
-BOOL AssemblyVersion::IsLargerServiceVersion(AssemblyVersion *pAssemblyVersion)
-{
-    BOOL result = FALSE;
-
-    if (GetBuild() > pAssemblyVersion->GetBuild())
-    {
-        result = TRUE;
-    }
-    else if ((GetBuild() == pAssemblyVersion->GetBuild()) &&
-             (GetRevision() > pAssemblyVersion->GetRevision()))
-    {
-        result = TRUE;
-    }
-
-    return result;
-}
-
 BOOL AssemblyVersion::Equals(AssemblyVersion *pAssemblyVersion)
 {
     BOOL result = FALSE;
@@ -164,20 +106,4 @@ BOOL AssemblyVersion::Equals(AssemblyVersion *pAssemblyVersion)
     return result;
 }
 
-BOOL AssemblyVersion::IsSmallerOrEqual(AssemblyVersion *pAssemblyVersion)
-{
-    return (Equals(pAssemblyVersion) ||
-            IsSmallerFeatureVersion(pAssemblyVersion) ||
-            (IsEqualFeatureVersion(pAssemblyVersion) &&
-             !IsLargerServiceVersion(pAssemblyVersion)));
-}
-
-BOOL AssemblyVersion::IsLargerOrEqual(AssemblyVersion *pAssemblyVersion)
-{
-    return (Equals(pAssemblyVersion) ||
-            IsLargerFeatureVersion(pAssemblyVersion) ||
-            (IsEqualFeatureVersion(pAssemblyVersion) &&
-             IsLargerServiceVersion(pAssemblyVersion)));
-}
-
 #endif
diff --git a/src/binder/textualidentityparser.cpp b/src/binder/textualidentityparser.cpp
index eb6e371afe..2913847dd2 100644
--- a/src/binder/textualidentityparser.cpp
+++ b/src/binder/textualidentityparser.cpp
@@ -65,7 +65,7 @@ namespace BINDER_SPACE
         const int iPublicKeyMaxLength = 2048;
 
         const int iVersionMax = 65535;
-        const int iRequiredVersionParts = 4;
+        const int iVersionParts = 4;
 
         inline void UnicodeHexToBin(LPCWSTR pSrc, UINT cSrc, LPBYTE pDest)
         {
@@ -288,10 +288,10 @@ namespace BINDER_SPACE
             {
                 tmpString.Clear();
                 tmpString.Printf(W("%d.%d.%d.%d"),
-                                 pAssemblyIdentity->m_version.GetMajor(),
-                                 pAssemblyIdentity->m_version.GetMinor(),
-                                 pAssemblyIdentity->m_version.GetBuild(),
-                                 pAssemblyIdentity->m_version.GetRevision());
+                                 (DWORD)(USHORT)pAssemblyIdentity->m_version.GetMajor(),
+                                 (DWORD)(USHORT)pAssemblyIdentity->m_version.GetMinor(),
+                                 (DWORD)(USHORT)pAssemblyIdentity->m_version.GetBuild(),
+                                 (DWORD)(USHORT)pAssemblyIdentity->m_version.GetRevision());
 
                 textualIdentity.Append(W(", Version="));
                 textualIdentity.Append(tmpString);
@@ -378,8 +378,10 @@ namespace BINDER_SPACE
     {
         BOOL fIsValid = FALSE;
         DWORD dwFoundNumbers = 0;
-        DWORD dwCurrentNumber = 0;
-        DWORD dwNumbers[iRequiredVersionParts];
+        bool foundUnspecifiedComponent = false;
+        const DWORD UnspecifiedNumber = (DWORD)-1;
+        DWORD dwCurrentNumber = UnspecifiedNumber;
+        DWORD dwNumbers[iVersionParts] = {UnspecifiedNumber, UnspecifiedNumber, UnspecifiedNumber, UnspecifiedNumber};
 
         BINDER_LOG_ENTER(W("TextualIdentityParser::ParseVersion"));
 
@@ -391,17 +393,46 @@ namespace BINDER_SPACE
             {
                 WCHAR wcCurrentChar = cursor[0];
 
-                if (dwFoundNumbers >= static_cast<DWORD>(iRequiredVersionParts))
+                if (dwFoundNumbers >= static_cast<DWORD>(iVersionParts))
                 {
                     goto Exit;
                 }
-                else if (wcCurrentChar == W('.') || wcCurrentChar == 0x00)
+                else if (wcCurrentChar == W('.') || wcCurrentChar == W('\0'))
                 {
-                    dwNumbers[dwFoundNumbers++] = dwCurrentNumber;
-                    dwCurrentNumber = 0;
+                    if (dwCurrentNumber == UnspecifiedNumber)
+                    {
+                        // Difference from .NET Framework, compat with Version(string) constructor: A missing version component
+                        // is considered invalid.
+                        //
+                        // Examples:
+                        //   "MyAssembly, Version=."
+                        //   "MyAssembly, Version=1."
+                        //   "MyAssembly, Version=.1"
+                        //   "MyAssembly, Version=1..1"
+                        goto Exit;
+                    }
+
+                    // Compat with .NET Framework: A value of iVersionMax is considered unspecified. Once an unspecified
+                    // component is found, validate the remaining components but consider them as unspecified as well.
+                    if (dwCurrentNumber == iVersionMax)
+                    {
+                        foundUnspecifiedComponent = true;
+                        dwCurrentNumber = UnspecifiedNumber;
+                    }
+                    else if (!foundUnspecifiedComponent)
+                    {
+                        dwNumbers[dwFoundNumbers] = dwCurrentNumber;
+                        dwCurrentNumber = UnspecifiedNumber;
+                    }
+
+                    dwFoundNumbers++;
                 }
                 else if ((wcCurrentChar >= W('0')) && (wcCurrentChar <= W('9')))
                 {
+                    if (dwCurrentNumber == UnspecifiedNumber)
+                    {
+                        dwCurrentNumber = 0;
+                    }
                     dwCurrentNumber = (dwCurrentNumber * 10) + (wcCurrentChar - W('0'));
                     
                     if (dwCurrentNumber > static_cast<DWORD>(iVersionMax))
@@ -417,12 +448,21 @@ namespace BINDER_SPACE
                 cursor++;
             }
 
-            if (dwFoundNumbers == static_cast<DWORD>(iRequiredVersionParts))
+            // Difference from .NET Framework: If the major or minor version are unspecified, the version is considered invalid.
+            //
+            // Examples:
+            //   "MyAssembly, Version="
+            //   "MyAssembly, Version=1"
+            //   "MyAssembly, Version=65535.1"
+            //   "MyAssembly, Version=1.65535"
+            if (dwFoundNumbers < 2 || dwNumbers[0] == UnspecifiedNumber || dwNumbers[1] == UnspecifiedNumber)
             {
-                pAssemblyVersion->SetFeatureVersion(dwNumbers[0], dwNumbers[1]);
-                pAssemblyVersion->SetServiceVersion(dwNumbers[2], dwNumbers[3]);
-                fIsValid = TRUE;
+                goto Exit;
             }
+
+            pAssemblyVersion->SetFeatureVersion(dwNumbers[0], dwNumbers[1]);
+            pAssemblyVersion->SetServiceVersion(dwNumbers[2], dwNumbers[3]);
+            fIsValid = TRUE;
         }
 
     Exit: