223810 - /js/src/jsscript.c js_alloc_entry may cause damaged heap memory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Wang Xianzhu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031027
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031027

in /js/src/jsscript.c (3.46.4.1):

Line 850:

/* NB: This struct overlays JSHashEntry -- see jshash.h, do not reorganize. */
typedef struct ScriptFilenameEntry {
    JSHashEntry         *next;          /* hash chain linkage */
    JSHashNumber        keyHash;        /* key hash function result */
    const void          *key;           /* ptr to filename, below */
    JSPackedBool        mark;           /* mark flag, for GC */
    char                filename[3];    /* two or more bytes, NUL-terminated */
} ScriptFilenameEntry;

JS_STATIC_DLL_CALLBACK(JSHashEntry *)
js_alloc_entry(void *priv, const void *key)
{
    return (JSHashEntry *)
           malloc(offsetof(ScriptFilenameEntry, filename) + strlen(key) + 1);
}
...

Here key is assumed to be a null-terminated string whose length is 2 or greater.
But this assumption is not always true.  If the length of the key string is
smaller than 2, the allocated buffer's size may be smaller than
sizeof(JSHashEntry), and then in JS_HashTableRawAdd, the assignments to its
fields will overflow.  For a debug version of browser, when the buffer is freed,
 MSVC runtime assertion failure and program crash will occur.

I modified the malloc line into:
           malloc(offsetof(ScriptFilenameEntry, filename) + strlen(key) + 3);
and the error does not occur any more.


Reproducible: Always

Steps to Reproduce:
1. build debug version of mozilla 1.5 with activex and activex-scripting enabled
2. after successful building, copy activex plugin files into proper directories
    ( npmozax.dll into plugins, nsAxSecurityPolicy.js and npmozax.xpt into
components, activex.js into defaults/pref)
3. run mozilla, and open the activex plugin test file calendar.html (in
/embedding/browser/activex/tests)

Actual Results:  
MSVC runtime assertion failed: DBGHEAP.c line 1066: DAMAGE: after .... block at
....".   If ignore, sometimes the program continues, sometimes crashes.


Stack trace when js_alloc_entry is called and a smaller buffer will be allocated:
(in js_SaveScriptFilename, the filename is "", originally set by the ActiveX plugin)

js_alloc_entry(void * 0x00000000, const void * 0x03cc2b64) line 863
JS_HashTableRawAdd(JSHashTable * 0x00a610e8, JSHashEntry * * 0x02a90810,
unsigned long 0x00000000, const void * 0x03cc2b64, void * 0x00000000) line 242 +
20 bytes
js_SaveScriptFilename(JSContext * 0x030f1098, const char * 0x03cc2b64) line 943
+ 23 bytes
js_NewScriptFromCG(JSContext * 0x030f1098, JSCodeGenerator * 0x0012c210,
JSFunction * 0x00000000) line 1045 + 13 bytes
CompileTokenStream(JSContext * 0x030f1098, JSObject * 0x030adf90, JSTokenStream
* 0x031c5df8, void * 0x030f1118, int * 0x00000000) line 2961 + 18 bytes
JS_CompileUCScriptForPrincipals(JSContext * 0x030f1098, JSObject * 0x030adf90,
JSPrincipals * 0x032f7120, const unsigned short * 0x03425b40, unsigned int
0x00000000, const char * 0x03cc2b64, unsigned int 0x00000001) line 3036 + 23 bytes
JS_CompileScriptForPrincipals(JSContext * 0x030f1098, JSObject * 0x030adf90,
JSPrincipals * 0x032f7120, const char * 0x03cc2b65, unsigned int 0x00000000,
const char * 0x03cc2b64, unsigned int 0x00000001) line 3006 + 33 bytes
MozAxAutoPushJSContext::MozAxAutoPushJSContext(JSContext * 0x030f1098, nsIURI *
0x030d2fe8) line 410 + 42 bytes
WillHandleCLSID(const _GUID & {CLSID_Calendar Control 8.0}, PluginInstanceData *
0x033b73b0) line 475 + 24 bytes
CreateControl(const _GUID & {CLSID_Calendar Control 8.0}, PluginInstanceData *
0x033b73b0, PropertyList & {...}, const unsigned short * 0x00000000) line 578 +
13 bytes
NewControl(const char * 0x033cec88, PluginInstanceData * 0x033b73b0, unsigned
short 0x0001, short 0x0008, char * * 0x033c7b98, char * * 0x033466c8) line 925 +
21 bytes
NPP_New(char * 0x033cec88, _NPP * 0x031e8908, unsigned short 0x0001, short
0x0008, char * * 0x033c7b98, char * * 0x033466c8, _NPSavedData * 0x00000000)
line 971 + 31 bytes
ns4xPluginInstance::InitializePlugin(nsIPluginInstancePeer * 0x03426150) line
821 + 105 bytes
ns4xPluginInstance::Initialize(ns4xPluginInstance * const 0x031e88f0,
nsIPluginInstancePeer * 0x03426150) line 616
nsPluginHostImpl::TrySetUpPluginInstance(nsPluginHostImpl * const 0x00b46058,
const char * 0x0340d828, nsIURI * 0x03252fe0, nsIPluginInstanceOwner *
0x0320acf0) line 3896 + 21 bytes
......

Comment 1

[timeless]

brendan: i bet my bug 223187 is this.

Comment 2

[timeless]

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

Comment 3

[timeless]

Attachment: max (current_code, 3)

Comment 4

[Phil Schwartau]

Formally confirming for consideration -

Comment 5

[Brendan Eich [:brendan]]

On what systems does malloc not round up to a multiple of 4, if not 8?

Oh well.  Taking, patch in a minute.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 7

[Brendan Eich [:brendan]]

Attachment: better proposed fix

Didn't like the repeated magic 3 in timeless's patch, or the magic sizeof(long)
in the last patch.  This is money.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 134262 [details] [diff] [review]
better proposed fix

Get there!

Comment 9

[Brendan Eich [:brendan]]

Fixed.

/be