1358549 - Incorrect js-ctypes ABI specification for FileTimeToSystemTime in MSMigrationUtils.jsm

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

This is bad. On 32-bit Windows, the *callee* pops parameters off the stack, whereas default_abi assumes that the *caller* pops parameters off the stack.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

(It should be using ctypes.winapi_abi instead)

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

Review commit: https://reviewboard.mozilla.org/r/132468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132468/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8860474 [details]
Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

https://reviewboard.mozilla.org/r/132468/#review135468

I mean, r=me because I take your word for it, but I have 2 questions:
1) what are the consequences of getting this wrong, in practice?
2) do the automated tests for this code still run correctly (on Windows) ?

I believe we also need to fix: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/unit/test_Edge_db_migration.js#173-174 and https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/ESEDBReader.jsm#237-238 .

And potentially https://dxr.mozilla.org/mozilla-central/rev/c8198aa6e7677e90cc7f1e2df0a14a5cc2719055/browser/components/migration/tests/unit/test_IE_cookies.js#18-46 ? (How would I know for sure? The IE cookie stuff I've never touched, and the rest might have my blame but I've just been moving it around, I think...)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8860474 [details]
Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

https://reviewboard.mozilla.org/r/132468/#review135470

Oops. Don't forget the extra places where we've copied / cargo-culted this, though...

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to :Gijs from comment #3)
> I mean, r=me because I take your word for it, but I have 2 questions:
> 1) what are the consequences of getting this wrong, in practice?

Possible stack corruption after returning from the native API

> 2) do the automated tests for this code still run correctly (on Windows) ?

Yeah that's no problem.

> 
> I believe we also need to fix:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> tests/unit/test_Edge_db_migration.js#173-174 and
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> ESEDBReader.jsm#237-238 .
> 
> And potentially
> https://dxr.mozilla.org/mozilla-central/rev/
> c8198aa6e7677e90cc7f1e2df0a14a5cc2719055/browser/components/migration/tests/
> unit/test_IE_cookies.js#18-46 ? (How would I know for sure? The IE cookie
> stuff I've never touched, and the rest might have my blame but I've just
> been moving it around, I think...)

There's a bunch of problems, yes. Myself and others will be filing follow-ups.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f87db499254
Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime; r=Gijs

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This goes back to Fx43 AFAICT. Is this something we'll want to consider backporting to supported branches?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8860474 [details]
Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

Approval Request Comment
[Feature/Bug causing the regression]: JS code in profile migration
[User impact if declined]: Potential for crashes and/or data corruption
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Trivial patch
[String changes made/needed]: None

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8860474 [details]
Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a stability issue
User impact if declined: Potential for crashing / data corruption
Fix Landed on Version: 55, 54 requested
Risk to taking this patch (and alternatives if risky): None. Trivial patch
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f87db499254

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8860474 [details]
Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

Fix a potential crash/data corruption. Beta54+. Should be in 54 beta 3.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/076868035c74

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8860474 [details]
Bug 1358549: Fix incorrect ABI specification in js-ctypes declaration of FileTimeToSystemTime;

Cancelling ESR52 request. I have looked at the libffi code and there is no chance of crashing given the way it is implemented.

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now) from comment #8)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Aaron's assessment on manual testing needs and the fact that this fix has automated coverage.