978562 - auth_test.js unit test incorrectly writes to {TypedArray}.length

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Typed array lengths are not writable properties and trying to write to them in strict mode should throw.  SpiderMonkey currently doesn't throw in such cases, but this will be fixed by bug 695438.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: dom patch

Here's a related case in mozilla-central.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Ni(In reply to Brian Hackett (:bhackett) from comment #1)
> Created attachment 8384268 [details] [diff] [review]
> dom patch
> 
> Here's a related case in mozilla-central.

As the lenght will be used to authenticate the sender of message we need to be sure that this change doesn't brake functionality or unit tests.

Comment 3

[Brian Hackett [Laid off!]]

Well, in both cases the assignment is vacuous, the same as 'x.length = x.length'.  If the array involved is always a typed array then trying to change its length won't do anything.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8384268 [details] [diff] [review]
dom patch

Review of attachment 8384268 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM as the operation there is completely redundant, we also have a test ensuring that the length is correct so we cannot go wrong here.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8384267 [details] [diff] [review]
patch

Review of attachment 8384267 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

While looking at this code I've noticed two things that might need addressing in a follow-up. First of all it seems that we're reusing the array passed to Authenticator.check() to populate the |authInfo.data| field, here:

http://hg.mozilla.org/mozilla-central/file/cb68b921c063/dom/wappush/src/gonk/CpPduHelper.jsm#l188

Shouldn't we clone it instead?

The second thing is that the description of the return parameter is clearly wrong:

http://hg.mozilla.org/mozilla-central/file/cb68b921c063/dom/wappush/src/gonk/CpPduHelper.jsm#l175

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8384268 [details] [diff] [review]
dom patch

Review of attachment 8384268 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wappush/src/gonk/CpPduHelper.jsm
@@ -186,5 @@
>        sec: AUTH_SEC_TYPE[sec],
>        mac: mac.toUpperCase(),
>        data: wbxml
>      };
> -    authInfo.data.length = wbxml.length;

Chuck added this here because we needed to know the Typed array length in gaia. See comment https://bugzilla.mozilla.org/show_bug.cgi?id=914685#c25 please.

Comment 8

[Brian Hackett [Laid off!]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)
> ::: dom/wappush/src/gonk/CpPduHelper.jsm
> @@ -186,5 @@
> >        sec: AUTH_SEC_TYPE[sec],
> >        mac: mac.toUpperCase(),
> >        data: wbxml
> >      };
> > -    authInfo.data.length = wbxml.length;
> 
> Chuck added this here because we needed to know the Typed array length in
> gaia. See comment https://bugzilla.mozilla.org/show_bug.cgi?id=914685#c25
> please.

Originally Chuck's code was adding a |dataLength| property to the object.  That property has since been removed and the assignment is now redundant.

Comment 9

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9584fced1895

Comment 10

[Brian Hackett [Laid off!]]

Can someone push the gaia patch?

Comment 11

[Brian Hackett [Laid off!]]

https://github.com/mozilla-b2g/gaia/commit/a495164d731c2dbe2eeb15eb1cc967e471c0e4f4

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> While looking at this code I've noticed two things that might need
> addressing in a follow-up.

Thank you. Filed bug 979128 as follow-up.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6feac97bcc9f

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9584fced1895

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8384267 [details] [diff] [review]
patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Unit tests on v1.3 are permared because we never uplifted this patch:

  1) [wappush] ProvisioningAuthentication > ProvisioningAuthentication.isDocumentValid() A message with USERPIN security mechanism was authenticated:
     TypeError: setting a property that has only a getter

[Bug caused by] (feature/regressing bug #):
Switching aurora to FF30 hardened the rules for setting typed Array.length, so we can no longer do this in the wappush test.

[User impact] if declined:
Unit tests will be perma-red

[Testing completed]:
Tested locally, and this fixes the failing test.

[Risk to taking this patch] (and alternatives if risky):
Test only change, low risk.

[String changes made]:
none.

Comment 16

[[:fabrice] Fabrice Desré]

How is that red only now since that landed 2 *weeks* ago?

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

We don't need this uplift anymore, Yuren landed this code directly last night:
https://github.com/mozilla-b2g/gaia/commit/5b9c7ffe96c13e065eb32d86cf929f7e4810a478

But to answer your question, setting the `length` property of typed arrays didn't throw an error until bug 695438 landed (3/5/14). So this unit test didn't fail until we started running v1.3 unit tests against a FF that had this fix (which happened 2 days ago when we moved aurora to FF30, thereby causing this tree closure mess).