1246061 - (CVE-2016-2808) null-byte written out of bounds using .watch() due to generation count overflow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: CESG Vulnerability Report 2 (pdf)

The CESG (https://www.cesg.gov.uk/) sent a vulnerability report to the security alias concerning a bug in the JS engine regarding Object.watch()

CESG REF: 54224850 / VULNERABILITY ID: 426732

Details in the attached PDF

   "There is a way to trick SpiderMonkey into using an invalid hash entry
   by overflowing an unsigned 32-bit integer. This makes it possible to use
   this vulnerability to write a null byte at a relative offset using an
   invalid pointer. This is achieved using Watchers. [...]"

Their PoC runs in a worker and increments the generation count until it wraps around to the same value.

Comment 1

[Jeff Walden [:Waldo]]

Aargh.  If this is the check I think it was, I always thought generation-comparison looked a little smarmy, and that wraparound appeared possible if you were willing to wait long enough.  I semi-assumed we were not caring on the basis of it indeed taking too long to run: "Triggering this null write via the following proof of concept takes about twenty five minutes."  :-(  But maybe 25mins isn't that long if you (say) waited to spring the attack til late at night.  I should have said something here at some point.

I suspect we can just make the generation field (and various other generation fields in other places) uint64_t to fix this.  Should be fun to track down all the various places generation numbers are treated as uint32_t, to avoid a stray truncation making a potential fix ineffective.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Possible patch

Sigh, I've been punting this bug too long.  :-(  Here's a probable patch.  I *think* I got all the cases where a generation is manually typed, but I could have missed some still.

Semi-unrelatedly except in concept:

js::ExpandoAndGeneration::generation, used for DOM proxy invalidation, is also a uint32_t similarly vulnerable to wraparound.  It's 32-bit wraparound, not 24-bit wraparound as for HashTable::gen.  That gives 256x for wraparound to occur, taking delay from 24mins to ~6000mins if we assume equally fast steps between increments.  But we *don't*, in that case, have to run all in a single time slice, possibly, so we might still have a problem.  We should probably decide the answer to that at the same time we deal with this issue, as it's something someone auditing our code after this patch lands would find, and might try to attack.

Comment 3

[Jeff Walden [:Waldo]]

bz, comments on the end of comment 2?  I'm inclined to just bump it to 64-bit and be done with worrying, to be honest -- even if wraparound could never happen because of JIT code being discarded by GC, because of event loop turnings meaning *something* makes the problem go away, blah blah blah.  I see no reason to try to be cute here.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Proof-of-concept strongly-typing HashTable::gen, atop the previous patch

Looks like my previous patch fixed everything.  This patch atop it makes HashTable::{gen,shift} into a class with mutators/accessors, and it makes HashTable::generator() and all callers deal with an opaque Generator class that *can't* implicitly convert to anything else, and things compile.  Woo!

We could land this (or something similar) if we wanted, but generation() both internally and externally (to the JS engine) is only rarely assigned to anything, so it's probably overkill.  Let's run with just the first patch.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8723294 [details] [diff] [review]
Possible patch

This increases the opt-build size of js::HashTable by four bytes on 32-bit systems.  On 64-bit systems, it increases the size of js::HashTable by four bytes as well, but it does so from 20 bytes of members to 24 -- so practically we were halfway into a word anyway.  I see no way to avoid either size increase.  (In any case hash table contents size should swamp the hash table's own size, if the user is doing it right.)

Note that generation, as the subsequent attachment demonstrates, is functionally opaque.  We could make it *actually* opaque, but it didn't seem quite worth it.  Feel free to say something if you disagree.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8723294 [details] [diff] [review]
Possible patch

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

Makes sense, although I wonder if it'd be a bit safer to have generation return a struct or class wrapping the uint64 instead.  That way you couldn't have accidental silent truncation of uint64 to uint32.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> bz, comments on the end of comment 2?  I'm inclined to just bump it to 64-bit and be done with worrying

I agree.  Fwiw, I did talk it over with efaust, and I'm 99% sure wraparound would not be a security issue, just a web-facing behavior correctness one.  But still, no reason to have to worry about it.  Bumping that counter to 64-bit seems fine.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #6)
> Makes sense, although I wonder if it'd be a bit safer to have generation
> return a struct or class wrapping the uint64 instead.  That way you couldn't
> have accidental silent truncation of uint64 to uint32.

See comment 4 and comment 5?  I would probably do this without even thinking about it (and make js::Generation into Opaque<uint64_t> for extra niceness), but HashTable::gen is part of a bit field, so it can't be directly made into anything else.

So: we both sort of agree this would be nice.  Do we nonetheless want to do it?  The extra patch here is a bit fugly certainly, so I'm on the fence here.  Do you want to make your suggestion more emphatically or just leave things as good enough?  (Particularly as we're not going to be outgrowing uint64_t:56 basically ever.)

No rush on an answer, I want to pull together a patch for comment 7 before landing anything anyway.

Comment 9

[Luke Wagner [:luke]]

Oops, sorry for missing that already.  Could the member variable stay a uint64_t bitfield and only use the Opaque<uint64_t> for the return type of generation()?

Comment 10

[Jeff Walden [:Waldo]]

That could be done.  I'd still be a little leery about the bit field users, but maybe they're few enough to live with.

It's perhaps worth noting generation is publicly used 1) by watchpoint code, and 2) by UbiNode code only for assertions.  The rest is internal hash table gunk.  So if we can kill off watches, we can remove a bunch of this nonsense from public misuse entirely.

Comment 11

[Luke Wagner [:luke]]

The bitfield is private (well not in HashTable, but by encapsulation of HashTable inside HashMap/Set) so I think this would still be a big improvement.

Comment 12

[Luke Wagner [:luke]]

But yeah, if we want to totally remove generation from the public interface, that sounds even better!

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Implement mozilla::Opaque<T>, a comparable class that otherwise-opaquely wraps a T

To be used in the next patch.  (Or read previous comments where I allude to the addition of this class.)

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Strongly-type consumers of HashTable::gen, using mozilla::Opaque<uint64_t>

Okay, yeah -- HashTable::gen is only incremented, never really subjected to implicit conversions otherwise -- so adding opacity at the HashTable boundary is totally good enough, you're right.

Comment 15

[Jeff Walden [:Waldo]]

The generation() removals in the second patch there are because, if we want to get rid of this, it seems best to keep it out of APIs where it's not used to discourage creeping badness, basically.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8725803 [details] [diff] [review]
Implement mozilla::Opaque<T>, a comparable class that otherwise-opaquely wraps a T

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

r=me with documentation nits addressed.

::: mfbt/Opaque.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * An opaque type supporting only comparison operations that wraps the specified
> + * integral type.

Maybe just "An opaque integral type supporting only comparison operators."?

A block comment describing reason(s) you might want to use Opaque<T> instead of T would be splendid.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> A block comment describing reason(s) you might want to use Opaque<T> instead
> of T would be splendid.

And of course I should have ended this with "...as the case for it being here is...opaque."

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8725805 [details] [diff] [review]
Strongly-type consumers of HashTable::gen, using mozilla::Opaque<uint64_t>

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

Great!

Comment 19

[Jeff Walden [:Waldo]]

jandem, expanding js::ExpandAndGeneration::generation (currently uint32_t) to uint64_t, as mooted in the end of comment 2 and discussed in comment 3 and comment 7, semi-runs up against MacroAssembler deficiencies in its two uses.

The first use:

        ExpandoAndGeneration* expandoAndGeneration = (ExpandoAndGeneration*)expandoVal.toPrivate();
        masm.movePtr(ImmPtr(expandoAndGeneration), tempVal.scratchReg());

        masm.branch32(Assembler::NotEqual,
                      Address(tempVal.scratchReg(),
                              ExpandoAndGeneration::offsetOfGeneration()),
                      Imm32(expandoAndGeneration->generation),
                      &failDOMProxyCheck);

This is addressable (...heh) by adding branch64.  Except at least x86-64 doesn't have cmp r64,imm64, so I'm not sure that would be the "right" MacroAssembler addition to make.

The second use:

        masm.loadPtr(*expandoAndGenerationAddr, tempVal.scratchReg());
        masm.branchPrivatePtr(Assembler::NotEqual, expandoAddr, tempVal.scratchReg(),
                              &failDOMProxyCheck);

        masm.load32(*generationAddr, scratch);
        masm.branch32(Assembler::NotEqual,
                      Address(tempVal.scratchReg(), offsetof(ExpandoAndGeneration, generation)),
                      scratch, &failDOMProxyCheck);

This could be solved by adding load64 and (again) branch64.  On 64-bit this is perfectly optimal.  But on 32-bit, you then have to take an extra register, really for no purpose because you could compare the low bits (and maybe branch), then compare the high bits (and maybe branch).

So, I'm not sure what would be the best MacroAssembler interface additions to address these needs.  Please advise!

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> This is addressable (...heh) by adding branch64.  Except at least x86-64
> doesn't have cmp r64,imm64, so I'm not sure that would be the "right"
> MacroAssembler addition to make.

For this one, we could add:

  branch64(Condition, const Address&, Imm64, Label*);

On 32-bit platforms, this would end up doing two (branch32) comparisons. On 64-bit platforms, it can forward to branchPtr or something.

> This could be solved by adding load64 and (again) branch64.  On 64-bit this
> is perfectly optimal.  But on 32-bit, you then have to take an extra
> register, really for no purpose because you could compare the low bits (and
> maybe branch), then compare the high bits (and maybe branch).

Hm. We could have:

  branch64(Condition, const Address&, const Address&, Register scratch, Label*);

On 64-bit we'd just load from address1 into scratch and branch. On 32-bit we could do this twice for the low bits and high bits, as you described. Not very pretty, but it gets the job done without requiring extra scratch registers. It should probably assert the scratch register != the base register of one of the addresses.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Implement MacroAssembler::branch64(Condition cond, const Address& lhs, Imm64 val, Label* label)

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Implement MacroAssembler::branch64(Condition cond, const Address& lhs, const Address& rhs, Register scratch, Label* label)

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Make ExpandoAndGeneration::generation a uint64_t

Comment 24

[Jeff Walden [:Waldo]]

The patch making ExpandoAndGeneration::generation a uint64_t is tested *only* to the extent that in a 64-bit build, I ran dom/bindings/test Mochitests and they passed.  I'd appreciate further edumacation as to how to verificate this patch.  :-)

Comment 25

[Boris Zbarsky [:bzbarsky]]

We may or may not have decent test coverage for it.  Worth checking whether any tests got added in the bug that added the ExpandoAndGeneration thing.  If they were, you're probably OK.  If they were not, we can try to come up with some...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8728313 [details] [diff] [review]
Make ExpandoAndGeneration::generation a uint64_t

Please leave offsetOfGeneration()'s name as it was, now that you've found all the callers.

r=me

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8728312 [details] [diff] [review]
Implement MacroAssembler::branch64(Condition cond, const Address& lhs, const Address& rhs, Register scratch, Label* label)

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

::: js/src/jit/MacroAssembler.h
@@ +816,5 @@
>  
>      inline void branch64(Condition cond, const Address& lhs, Imm64 val, Label* label) PER_ARCH;
>  
> +    // Compare the value at lhs with the value at rhs.  The scratch register
> +    // *may* be the base of lhs.

This comment doesn't match the |lhs.base != scratch| asserts. But right, we should be careful.

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Fully-concatenated patch, contains no tests

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 8733566 [details] [diff] [review]
Fully-concatenated patch, contains no tests

Ugh, I thought I'd made a request already, and I was going to land today.  :-(

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?

See comment 0.  This battle is lost.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I elided the comment on js::Generation, and I left the existing comments by the generation() methods in HashTable code.  Those changes can be a separate patch to land later.  With those mini-adjustments, I don't think there's anything more that can be done to camouflage the issue.  (Note that because this combines the HashTable generation-changing code and the ExpandoAndGeneration generation-changing code, along with low-level JIT primitives, this is semi-camouflaged already by dint of information overload to the reader.  Sort of.)

* Which older supported branches are affected by this flaw?  If not all supported branches, which bug introduced the flaw?

All of them.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Shouldn't be bad.  Will do after this sticks.

* How likely is this patch to cause regressions; how much testing does it need?

The important parts of this are pretty small, but it's a moderate amount of change.  I'm somewhat inclined to say either landing now, or maybe somewhat later (the slightly more paranoid option), is the right balance of testing and exposure.  Since I don't have approval to land now, I can wait a bit here.  I think I'd hesitate at landing more than a week after this, tho, if at all possible.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8733566 [details] [diff] [review]
Fully-concatenated patch, contains no tests

sec-approval+.

We'll want this on branches.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/967dcb05f347

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks good. Waldo, can you request uplift?

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Aurora patch

Approval Request Comment
[Feature/regressing bug #]: ancient, at least Mozilla 1.8 (whenever js/src/jsdhash.h was introduced likely)
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: landed in m-i a couple days ago; sadly, 32-bit integer overflow means this isn't automatically-testable
[Risks and why]: not too many, the backport was painless
[String/UUID change made/needed]: N/A

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Beta patch

Approval Request Comment
[Feature/regressing bug #]: ancient
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: landed in m-i; not auto-testable because of 32-bit wraparound necessity
[Risks and why]: beta backport had slightly more backporting pain -- mostly related to MacroAssembler backporting across some refactoring we've done on trunk; but such issues should be sussed out by a compilation failure pretty easily -- so still fairly low risk, but higher than with other backportings
[String/UUID change made/needed]: N/A

Comment 35

[Jeff Walden [:Waldo]]

Attachment: Beta patch

Oops, wrong patch last time -- see previous approval-request comment.

Comment 36

[Jeff Walden [:Waldo]]

Attachment: ESR45 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: trunk is 48, right?
Risk to taking this patch (and alternatives if risky): mostly that I messed up backporting of MacroAssembler changes, but such mistakes should appear as compilation failures -- so not horribly risky; no alternatives
String or UUID changes made by this patch: N/A

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8734908 [details] [diff] [review]
Beta patch

May be a bit risky but I think we should take this for beta 6.

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8734906 [details] [diff] [review]
Aurora patch

sec-high, taking this for aurora

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ff2fe5a07d9

Comment 40

[Jeff Walden [:Waldo]]

Attachment: ESR38 patch

Backporting to ESR38 (shoot me now) is a little different from backporting to 45+.

First, our hash table code has different orderings/sizings of the various HashTable fields around the generation number.  Different sizes of bit fields, other things.  I fiddled stuff into the state it's in on trunk, because I'm not sure it's a good idea to give up efficient packing, and because being *more* like trunk seems likely safer than having a smaller change, that's *less* similar to what we'll test and run with elsewhere.

(There's one additional change, for this stuff: js/src/shell/jsheaptools.cpp needed a modification, because it does some manual generation-comparing.  If I were on trunk I'd make that use relookupForAdd [?, not sure exactly what it is that exists for that, but I'm ~99% sure there's something] to avoid the manual generation-comparing.  But for a backport to an ESR, a smaller change seems best.)

Second, Imm64 doesn't exist in ESR38.  Maybe it's backportable, maybe not.  I dunno.  Given the ExpandoAndGeneration bits of this are a hedge against risk, without certain danger if we don't make the changes, I think we should not bother making ExpandoAndGeneration::generation a uint64_t.  Backporting a JIT feature to address only-theoretical concerns seems unwise to me.

So: r?luke for the modified ESR38 hash table code.  And r?jandem to agree on not backporting ExpandoAndGeneration modifications (or rewriting them to not use Imm64, or backporting Imm64, or some other belabored trick).

Comment 41

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

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

Comment 42

[Jan de Mooij [:jandem]]

Comment on attachment 8734958 [details] [diff] [review]
ESR38 patch

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #40)
> Given the ExpandoAndGeneration bits of this are a hedge against
> risk, without certain danger if we don't make the changes, I think we should
> not bother making ExpandoAndGeneration::generation a uint64_t.  Backporting
> a JIT feature to address only-theoretical concerns seems unwise to me.

Makes sense to me. I'd also be fine with disabling these stubs completely on ESR 38 btw, but that doesn't seem necessary here.

Comment 43

[Sylvestre Ledru [:Sylvestre]]

The patch is pretty big and I am bit uncomfortable to take in esr
> [Risks and why]: not too many, the backport was painless
Could you detail what it means?

Comment 44

[Jeff Walden [:Waldo]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #43)
> The patch is pretty big and I am bit uncomfortable to take in esr
> > [Risks and why]: not too many, the backport was painless
> Could you detail what it means?

There are two separate issues fixed by this patch.

===

First: js::HashSet::generation() and js::HashMap::generation() pre-patch are 32-bit integer counters, recording the "generation" of the structure.  If at times T1 and T2 the counter has the same generation value, then the structure hasn't changed -- pointers to keys/values are all valid.  Every increment denotes a new generation.

But it's easy to make generation-incrementing overflow to 0 if generations are 32 bits.  That makes "same value" comparisons unusable.  Malicious code can make this happen with the non-standard Object.prototype.watch method.

This is fixed by the changes to GCHashTable.h, HashTable.h, jsapi.h, jscntxt.h, jswatchpoint.cpp, UbiNodeCensus.cpp, Opaque.h, and moz.build.

Most of these files' changes *remove* exposure of generation, reducing attack surface, unquestionably safe.  The rest make generation an opaque 64-bit integer that can't overflow this millennium.  ;-)  The only interesting changes are to HashTable.h, and they're straightforward -- basically one way to do it.

I don't think this part of the patch has any particular risk.

===

Second: we have a separate "generation" concept used for a JIT optimization, where that generation's another 32-bit integer that might also overflow.

(Theoretically -- we don't know malicious code can trigger this, due to the slow-script dialog [assuming the user doesn't hit Continue a hundred times].  But it's not certain, and the slow-script dialog *is not 100% reliable* last I checked.  Certainly it's not meant as security mechanism.  So we'd really rather fix this than take the risk.)

The changes for this half of the patch involve two classes of change.

First, we require two low-level JIT mechanism for 64-bit integer comparison.  On 64-bit these are easy; on 32-bit they decompose into two 32-bit operations.  All of this reuses existing functionality to do 32-bit or pointer-sized ops.  It changes a bunch of files, but it's quite simple.  And if we were to make a mistake here, a comparison would almost certainly *fail* -- which would trigger safe (but less-fast) behavior.

Second, we have to change a couple places (IonCaches.cpp, SharedIC.cpp) *performing* the aforementioned comparisons, to do 64-bit instead of 32-bit comparisons.  This is again pretty simple -- and again, a mistake almost certainly means safe-but-slower behavior.

So: two sub-parts, each pretty simple (tho one is a larger *number* of simple changes), and a mistake probably means being slow, not unsafe.

===

The first issue *must* be backported, because it's definitely exploitable.  This part of the patch is very similar to the trunk-work with an easy backport.  We have no reason not to take this, and good reason (exploitable!) to take it.

Re the second issue, I meant what I said in the approval request.  The risk of this change is low.  But *if* the backport were complicated, the larger number of changes would mean resulting risk -- too much for a quasi-speculative issue.  It wasn't a complicated backport, so we should take this.

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8734958 [details] [diff] [review]
ESR38 patch

ok, taking it on esr38

Comment 46

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8734909 [details] [diff] [review]
ESR45 patch

Important issue, taking it.
Should be in 45.1.0

Comment 47

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

https://hg.mozilla.org/releases/mozilla-esr45/rev/7c26f8cd7ab3

Comment 48

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

https://hg.mozilla.org/releases/mozilla-esr38/rev/fa4efccde9b7

Comment 49

[Matt Wobensmith [:mwobensmith][:matt:]]

Comment 24 makes an attempt to verify the fix. Comment 25 mentions the possible need for more tests. Without an actual PoC or further tests, I won't be able to have this verified otherwise. If someone provides the above and more information, I'm happy to take a look at it. Marking qe-verify-.

Comment 50

[(no longer active)]

(In reply to Wes Kocher (:KWierso) from comment #48)
> https://hg.mozilla.org/releases/mozilla-esr38/rev/fa4efccde9b7

This landed without mfbt/Opaque.h.  Fixed in https://hg.mozilla.org/releases/mozilla-esr38/rev/486740b9903a.

Comment 51

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

Thanks for that.
Even with that, though, builds were still busted like https://treeherder.mozilla.org/logviewer.html#?job_id=71468&repo=mozilla-esr38

Backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/861f6b83ce1d to fix the builds up.

Comment 52

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/71f611fd27c7d6cb7d6dab9895c2922948042543 *eyes pulsebot's failure to comment in sec-bugs beadily*

Setting aside missing mfbt/Opaque.h, the subsequent failures were due to the changes noted in the first part of comment 40.  ("It all built locally for me...")

On doing archaeology to determine how trunk differed, I discovered these differences were introduced in bug 1136046, with a somewhat more involved patch than the mini-changes I made here that buggily-half-ported that work.  The extent of that work certainly isn't something I have any desire to try to backport, nor any need to do so.  My adjustments were just to try to be consistent, not because necessity.

So I reverted those bits of change, leaving only the uint32_t->uint64_t change and rearranging of two fields to not pack stupidly.  HashTable might be slightly bigger on ESR38 for lack of the packing I half-attempted, but I think we can live with that.  :-)

Comment 53

[(no longer active)]

(In reply to Wes Kocher (:KWierso) from comment #51)
> Thanks for that.
> Even with that, though, builds were still busted like
> https://treeherder.mozilla.org/logviewer.html#?job_id=71468&repo=mozilla-
> esr38
> 
> Backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/861f6b83ce1d
> to fix the builds up.

I doubt there's anything for me to do here. :-)

Comment 54

[Jeff Walden [:Waldo]]

Landed updated/clearer comments, now that we're a ways past this being fixt everywhere:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7a9ba853405d5f05dd4b62cf83ebc54bbc1968

It'd be nice to land a testcase as well, but as a practical matter it's not really feasible to overflow a 32-bit counter here with the amount of work to do each iteration -- not in short enough time to have a test for it.  So let's be proactive and testsuite- this.

Comment 55

[Carsten Book [:Tomcat]]

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