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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Dave Townsend [:mossop]]

Attachment: Bug 1555179: Use 0 in place of out of range numbers in version comparisons. r=froydnj

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ef7dff7b38
Use 0 in place of out of range numbers in version comparisons. r=froydnj

Comment 4

[Dave Townsend [:mossop]]

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

Comment 5

[Dave Townsend [:mossop]]

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

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b5ef7dff7b38

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e380696a22dd

Comment 10

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.