706249 - "ASSERTION: We've overflowed the mSpec buffer" in nsStandardURL::BuildNormalizedSpec

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: We've overflowed the mSpec buffer!: 'mSpec.Length() <= approxLen', file netwerk/base/src/nsStandardURL.cpp, line 697

The last major change to this function was in bug 125608.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Caused by the null password entry.  Could fix by omitting it if empty, but that would be a change in behavior and it's not clear if that would even be correct (i.e. a null password is a password), so enforce adding one byte for a password (for the ':') even if the field is empty.  Patch follows.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch with tests

Tested; asserts without code fix, no assert with it

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Worst-case analysis of this bug is it writes a '\0' to the byte following the allocation in some (not all) cases where the password is given but empty.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 577837 [details] [diff] [review]
Patch with tests

r=me

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

inbound via https://hg.mozilla.org/integration/mozilla-inbound/rev/8304db7e46bb

The original bug never existed in 1.9.x -> unaffected

Once it's green and merged to m-c I'll ask for approvals for Aurora and Beta

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Merged to m-c

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 577837 [details] [diff] [review]
Patch with tests

Now in m-c; we should get it into aurora and beta soon

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 577837 [details] [diff] [review]
Patch with tests

[Triage Comment]
Please land this sg:crit bug on aurora/beta asap so that we can bake this on beta for ~2 weeks.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/12c96ed8154d
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/9f3a16bf8afc

Tracking for FF9 was confused - it is affected, and it was also approved for beta, so marking fixed.  Didn't touch tracking (JST?)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with the following tinderbox debug builds:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a1) Gecko/20111214 Firefox/11.0a1

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111214 Firefox/10.0a2

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0) Gecko/20111212 Firefox/9.0

Mozilla/5.0 (X11; Linux i686; rv:9.0) Gecko/20111208 Firefox/9.0

I was not able to check on Windows because all the tinderbox debug builds seem to be broken and I can't start those.