937404 - Possible buffer size miscalculation in JS::NotableStringInfo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The move constructor for NotableStringInfo doesn't initialize the bufferSize member. However, it seems that bufferSize is only ever expected to be live in NotableStringInfo's copy constructor, which itself doesn't seem to be used.

This seems to have been the case since the code was added; Waldo is trying out a patch that deletes the copy constructor on Nightly, Aurora, Beta, and Release to make sure.

Comment 1

[Jim Blandy :jimb]

Attachment: Delete NotableStringInfo copy constructor, to see if it's used.

Comment 2

[Jeff Walden [:Waldo]]

esr24 (and by extension b2g18/esr17) and mozilla-release don't have NotableStringInfo at all, nor any move-construction stuff, and only if moves are used ubiquitously would this malign constructor not be used.

Both aurora and beta have compiled the JS engine and are churning away on the rest of Gecko still, no certain conclusions yet.

Comment 3

[Jeff Walden [:Waldo]]

Both aurora/beta compiled, so it looks as though the constructor's unused on aurora/beta, so I think we're safe here.

Nonetheless, to be absolutely sure (and to catch any platforms I can't/didn't compile on), I think we should land patches to delete the back constructor on trunk and on all branches.  jimb, you agree?  I can post the aurora/beta backports (not directly applicable but simple backports) for review/approval if you agree.

Comment 4

[Jim Blandy :jimb]

Yes, let's delete it since it's 1) unsafe, and 2) unused.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Trunk patch

It seems to me that if we're killing the copy constructor that uses bad data, and that's the only use of the data, we should kill the bad data entirely as well.  If it's not there at all, *no one* can misuse it.  :-)  Super-minor delta to the previous patch.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Aurora version of previous patch

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Beta patch

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Beta patch, qref'd

Comment 9

[Jeff Walden [:Waldo]]

I'm told over IRC that the trunk patch will need s/JS_ValueToString/JS::ToString/ to work, just to note.  Trying to round up all the patches here for pushing tonight, uploaded to bug, but may not have time -- we'll see, if I do I'll obsolete everything here and leave just the obvious targeted patches up.

Comment 10

[Jeff Walden [:Waldo]]

Er.  Wrong bug for comment 9, sorry.  :-(

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35f1c196e7d3

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8334127 [details] [diff] [review]
Aurora version of previous patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 893222
User impact if declined: in theory none -- fairly sure we're removing dead code -- but if this code actually *isn't* dead somehow, it's potentially sec-critical; removing the code conclusively proves safety both into the future, and retroactively
Testing completed (on m-c, etc.): this fix is correct if it builds, so no meaningful testing required beyond landing it on m-c (done an hour ago, built fine locally before that, ultimate results will arrive in a few hours)
Risk to taking this patch (and alternatives if risky): basically none, could back out or fix the underlying issue if this isn't actually dead code
String or IDL/UUID changes made by this patch: N/A

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8334131 [details] [diff] [review]
Beta patch, qref'd

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 893222
User impact if declined: in theory none -- fairly sure we're removing dead code -- but if this code actually *isn't* dead somehow, it's potentially sec-critical; removing the code conclusively proves safety both into the future, and retroactively
Testing completed (on m-c, etc.): this fix is correct if it builds, so no meaningful testing required beyond landing it on m-c (done an hour ago, built fine locally before that, ultimate results will arrive in a few hours)
Risk to taking this patch (and alternatives if risky): basically none, could back out or fix the underlying issue if this isn't actually dead code
String or IDL/UUID changes made by this patch: N/A

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35f1c196e7d3 fixed on central

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70b82e1762c2
https://hg.mozilla.org/releases/mozilla-beta/rev/e43e659cd13f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e43e659cd13f

Comment 17

[Jeff Walden [:Waldo]]

Opening -- this was removing unsafe code, but the unsafe code was dead, so no actual danger, as demonstrated by the success in removing it everywhere without issue.

Comment 18

[u279076]

I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.