Bug 1555179: Use 0 in place of out of range numbers in version comparisons. r=froydnj a=RyanVM
authorDave Townsend <dtownsend@oxymoronical.com>
Tue, 28 May 2019 16:37:45 -0700
changeset 533460 e380696a22dd3cb1e71546c5c805c33128dd1238
parent 533459 9e67e1c4e97815f2778cd3fa504925074b21968e
child 533461 89da9d90490a421de0dc706a17000fbfbb580455
push id11331
push usernerli@mozilla.com
push dateWed, 29 May 2019 14:58:24 +0000
treeherdermozilla-beta@104b25c67fb0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, RyanVM
bugs1555179
milestone68.0
Bug 1555179: Use 0 in place of out of range numbers in version comparisons. r=froydnj a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D32926
xpcom/base/nsVersionComparator.cpp
--- a/xpcom/base/nsVersionComparator.cpp
+++ b/xpcom/base/nsVersionComparator.cpp
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsVersionComparator.h"
 
 #include <stdlib.h>
 #include <string.h>
 #include <stdint.h>
+#include <errno.h>
 #if defined(XP_WIN) && !defined(UPDATER_NO_STRING_GLUE_STL)
 #  include <wchar.h>
 #  include "nsString.h"
 #endif
 
 struct VersionPart {
   int32_t numA;
 
@@ -60,17 +61,29 @@ static char* ParseVP(char* aPart, Versio
   if (dot) {
     *dot = '\0';
   }
 
   if (aPart[0] == '*' && aPart[1] == '\0') {
     aResult.numA = INT32_MAX;
     aResult.strB = "";
   } else {
+    errno = 0;
     aResult.numA = strtol(aPart, const_cast<char**>(&aResult.strB), 10);
+
+    // Different platforms seem to disagree on what to return when the value
+    // is out of range so we ensure that it is always what we want it to be.
+    // We choose 0 firstly because that is the default when the number doesn't
+    // exist at all and also because it would be easier to recover from should
+    // you somehow end up in a situation where an old version is invalid. It is
+    // much easier to create a version either larger or smaller than 0, much
+    // harder to do the same with INT_MAX.
+    if (errno != 0) {
+      aResult.numA = 0;
+    }
   }
 
   if (!*aResult.strB) {
     aResult.strB = nullptr;
     aResult.strBlen = 0;
   } else {
     if (aResult.strB[0] == '+') {
       static const char kPre[] = "pre";
@@ -80,17 +93,24 @@ static char* ParseVP(char* aPart, Versio
       aResult.strBlen = sizeof(kPre) - 1;
     } else {
       const char* numstart = strpbrk(aResult.strB, "0123456789+-");
       if (!numstart) {
         aResult.strBlen = strlen(aResult.strB);
       } else {
         aResult.strBlen = numstart - aResult.strB;
 
+        errno = 0;
         aResult.numC = strtol(numstart, &aResult.extraD, 10);
+
+        // See above for the rationale for using 0 here.
+        if (errno != 0) {
+          aResult.numC = 0;
+        }
+
         if (!*aResult.extraD) {
           aResult.extraD = nullptr;
         }
       }
     }
   }
 
   if (dot) {
@@ -129,17 +149,23 @@ static wchar_t* ParseVP(wchar_t* aPart, 
   }
 
   if (aPart[0] == '*' && aPart[1] == '\0') {
     static wchar_t kEmpty[] = L"";
 
     aResult.numA = INT32_MAX;
     aResult.strB = kEmpty;
   } else {
+    errno = 0;
     aResult.numA = wcstol(aPart, const_cast<wchar_t**>(&aResult.strB), 10);
+
+    // See above for the rationale for using 0 here.
+    if (errno != 0) {
+      aResult.numA = 0;
+    }
   }
 
   if (!*aResult.strB) {
     aResult.strB = nullptr;
     aResult.strBlen = 0;
   } else {
     if (aResult.strB[0] == '+') {
       static wchar_t kPre[] = L"pre";
@@ -149,17 +175,24 @@ static wchar_t* ParseVP(wchar_t* aPart, 
       aResult.strBlen = sizeof(kPre) - 1;
     } else {
       const wchar_t* numstart = wcspbrk(aResult.strB, L"0123456789+-");
       if (!numstart) {
         aResult.strBlen = wcslen(aResult.strB);
       } else {
         aResult.strBlen = numstart - aResult.strB;
 
+        errno = 0;
         aResult.numC = wcstol(numstart, &aResult.extraD, 10);
+
+        // See above for the rationale for using 0 here.
+        if (errno != 0) {
+          aResult.numC = 0;
+        }
+
         if (!*aResult.extraD) {
           aResult.extraD = nullptr;
         }
       }
     }
   }
 
   if (dot) {