228210 - XPCOM glue is not backwards compatible -> NS_GetFrozenFunction trashes memory

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Darin Fisher]

take some code linked with the glue from 1.4 and try running it with 1.6.  i
suspect it will crash.  the problem is that the size of XPCOMFunctions (see
nsXPCOMPrivate.h) increased post 1.4 to include getDebug and getTraceRefcnt
functions.  XPCOMFunctions includes a field called "size" that is supposed to be
initialized with sizeof(XPCOMFunctions) by the embedder.  however,
NS_GetFrozenFunctions does not check this field :-(

the result is trashing of memory if code compiled against the 1.4 glue is run
against a 1.6 build.

Comment 1

[Darin Fisher]

i don't know if any embedders will actually care, but this does break our
embedding story.  actually, it's been broken since July :-/

Comment 2

[Darin Fisher]

Attachment: v1 patch

here's a patch that fixes this.  i actually stumbled upon this bug because i
was looking at extending xpcomFunctions to include the functions in
nsStringAPI.  so, the patch here isn't the most minimal patch.	i've simplified
the code a bit with a preprocessor macro.

Comment 3

[Brendan Eich [:brendan]]

Yikes -- when did compatibility break, after 1.5 or before?

/be

Comment 4

[Benjamin Smedberg]

Comment on attachment 137268 [details] [diff] [review]
v1 patch

>+    // these functions were added post 1.4 (need to check size of |functions|)
>+    if (functions->size >= sizeof(XPCOMFunctions)) {

Why not
functions->size > offsetof(XPCOMFunctions,getTraceRefcnt)

That way when XPCOMFunctions grows again (as it will for your string embedding
stuff ;)  you work with all versions of the glue.

Comment 5

[Benjamin Smedberg]

Comment on attachment 137268 [details] [diff] [review]
v1 patch

While we're at it, should we do a "greater than" version check like so:

if (functions->size > sizeof(XPCOMFunctions))
  return NS_ERROR_FAILURE;

Comment 6

[Darin Fisher]

brendan: i don't know off the top of my head.  maybe that is 1.5??


> Why not
> functions->size > offsetof(XPCOMFunctions,getTraceRefcnt)
> 
> That way when XPCOMFunctions grows again (as it will for your string embedding
> stuff ;)  you work with all versions of the glue.

yeah, i have that in my tree for the nsStringAPI fu.  i figured it was best to
keep this patch simple, but whatever.


> While we're at it, should we do a "greater than" version check like so:
>
> if (functions->size > sizeof(XPCOMFunctions))
>   return NS_ERROR_FAILURE;

the glue currently calloc's the XPCOMFunctions structure, so provided it null
checks before calling functions, all should be ok.  of course, it doesn't null
check :-(

if you add that check, then code compiled against 1.6 will not work in 1.4.  is
that really what we want?

Comment 7

[Darin Fisher]

actually, what dougt said over AIM makes sense.  new code should not call the
new functions if it wants to work in an old build.  we should protect against
crashes, but nonetheless i think that's a less critical issue.

Comment 8

[Darin Fisher]

fixed-on-trunk for 1.6 final.

Comment 9

[Darin Fisher]

restoring blocking1.6+ flags that brendan set.