Closed Bug 1555179 Opened 6 months ago Closed 6 months ago

Version comparator returns different results on different platforms in the presence of >32-bit numbers

Categories

(Core :: XPCOM, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox67 blocking fixed
firefox67.0.1 blocking fixed
firefox68 + fixed
firefox69 + fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

We use wcstol to parse numbers out of version strings on Windows and strtol elsewhere. The two claim to return the same results. Both say that when the value parsed is too large for a uint32_t they can return LONG_MAX or LONG_MIN. It seems that the two return different ones.

This patch first switches to support 64-bit numbers and then in the unlikely case that a version does overflow it ensures the value is converted to LLONG_MAX.

On second thoughts, switching to 64-bit seems overkill for a rare use-case. And I think it might be better to make an invalid version to match a missing version (0) since if you accidentally end up with an invalid version to compare against it is easy to make another version that is larger or smaller than it, much harder with the int64 limit.

Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ef7dff7b38
Use 0 in place of out of range numbers in version comparisons. r=froydnj

I split the part for bug 1554029 out and landed this.

Comment on attachment 9068228 [details]
Bug 1555179: Use 0 in place of out of range numbers in version comparisons. r=froydnj

Beta/Release Uplift Approval Request

  • User impact if declined: This is required for fixing bug 1554029.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple error checking addition. The only affected code would be behaving badly already.
  • String changes made/needed: None
Attachment #9068228 - Flags: approval-mozilla-release?
Attachment #9068228 - Flags: approval-mozilla-beta?
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment on attachment 9068228 [details]
Bug 1555179: Use 0 in place of out of range numbers in version comparisons. r=froydnj

Fx67 dot-release blocker. Approved for 68.0b6 and 67.0.1.

Attachment #9068228 - Flags: approval-mozilla-release?
Attachment #9068228 - Flags: approval-mozilla-release+
Attachment #9068228 - Flags: approval-mozilla-beta?
Attachment #9068228 - Flags: approval-mozilla-beta+

This was caught by one of the new automated tests added in bug 1554029 failing, so I don't think there's a need for manual QA validation either.

https://hg.mozilla.org/releases/mozilla-release/rev/4e3b198b1d846e0c30fc079f5e1998ae450b8b12

Flags: qe-verify-
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.