314456 - decodeURIComponent and confirmEx show parts of the memory

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Georges-Etienne Legendre]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

This peace of code shows a part of the memory.

var s=decodeURIComponent("");
Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
.getService(Components.interfaces.nsIPromptService).confirmEx(window,
"test",s,0,null, null, null, null, { });

Reproducible: Always

Steps to Reproduce:
1. Run this script

Actual Results:  
A part of the memory is displayed in this message box.

Expected Results:  
An empty message.

http://forums.mozillazine.org/viewtopic.php?p=1843365#1843365

Comment 1

[Daniel Veditz [:dveditz]]

Confirming, does leak a little memory. Not sure I see the point of making the bug security sensitive after making a public forum post about it...

alert and confirm available to web scripts handle the returned empty string correctly so I don't think this is a security issue (chrome scripts can already do anything).

I haven't debugged this yet, but I wonder if the problem is javascript uses length-counted strings while the raw prompt service requires null-terminated ones, and something in there isn't translating the string types correctly.

Comment 2

[Daniel Veditz [:dveditz]]

Looks like the problem is in Decode() in jsstr.c, and Encode() has the same problem. (That means this also affects decodeURI, encodeURI and encoderURIComponent.) At the end the working string R->chars is realloc'd, normally OK but in the case of an empty string realloc(null, 0+1) gets you fresh memory that's never initialized.

Could special-case "" and bail early, or check for length == 0 at the end.

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: Handle Decode/Encode of empty strings

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Alternative: avoid doing any work at all.

This seems like the perfect time for a use of the emptyString (and we avoid two allocations in the process).

dveditz, what do you think?

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 201436 [details] [diff] [review]
Alternative: avoid doing any work at all.

sure, the bail-early approach is better.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 201436 [details] [diff] [review]
Alternative: avoid doing any work at all.

r=me, can ride along, no risk.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.

Comment 9

[Bob Clary [:bc] (inactive)]

needs to go in the "security" suite, not the js test library.

Comment 10

[Bob Clary [:bc] (inactive)]

Legendre's test will be available as tests/mozilla.org/security/314456-01.html in the qa test farm.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 201436 [details] [diff] [review]
Alternative: avoid doing any work at all.

Dan, Tim, is this something that we want on the 1.7 branch?

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 201436 [details] [diff] [review]
Alternative: avoid doing any work at all.

approved for old branches, a=dveditz for drivers

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.7 branches.

Comment 14

[Bob Clary [:bc] (inactive)]

v ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 windows/linux/mac 20060221, moz 1.7.13 windows 20060221

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 232223 has been marked as a duplicate of this bug. ***