8060 - memory corruption - xpt_link fails when asked to link a large .xpt file

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Mike McCabe]

When I do 'xpt_link outfile *.xpt' in the components directory (to see just how
small it can be made) xpt_link fails with a seg fault.  Looks like it's memory
corruption; realloc in libxpt (or malloc too, tried that) fails when trying to
grow the data pool from 8k to 16k.

This could be limited to xpt_link, or it could be a problem with libxpt, which
means that it could affect xpidl and possibly (less likely) the xptinfo runtime
as well.

Comment 1

[Michael Ang]

Reassigning to self/easing mccabe's pain.

Comment 2

[Mike McCabe]

A little bird told me that there may be tools available to debug this sort of
problem on win32.  Would Purify find it?

Comment 3

[Michael Ang]

Attachment: purify trace

Comment 4

[Michael Ang]

Looking at the purify trace, it seems that pool->data gets set beyond the end of
the buffer, eventually causing the crash in Grow_Pool.

Comment 5

[Michael Ang]

Mass reassign to mccabe since I'm outta here.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I have a tentative fix, which I've mailed to mccabe for review.

Basically, we were only growing the data pool when we were writing in the _data_
section, so if you had a header that was > XPT_ALLOC_CHUNK you'd fault out.  I
have a patch that fixes that, makes DoHeader's annotation processing interative
rather than recursive to avoid blowing the stack in the link-many-typelibs case.

I can now link components/*.xpt into one mozilla.xpt, which should also help our
startup performance.

I _don't_ see the realloc failure that mang mentions, though; we never try to
realloc in this failure mode, as far as I can tell.  So it's possible that
there's another bug lurking.

(Oh, and I killed a compile warning, too. =) )

mccabe: if you positively review my diff, just mark this fixed.

Comment 7

[John Bandhauer]

shaver, if you have not alreay checked in your patch by the timne you read
this could you sent it to me?

I'm playing with linking all these files and seeing a bad assert. When I run
under purify I see uninitialized memreads, array out of bounds writes and the
like.

Even just linking 2 xpt files I see one case of an uninitialized memory read
when doing a XPT_Do32 "u.b8[3] = CURS_POINT(cursor);" doing ENCODE even though
it has clearly done a CHECK_CURSOR. I'm thinking that the buffer size is getting
out of whack with the info about the buffer size somewhere. Even though we are
not always stepping on memory in use for something else, we are getting out of
our sandbox sometimes even on small links. Is this what you fixed?

I'll give it a try in purify when it comes down the pike.

John.

Comment 8

[John Bandhauer]

Shaver's patch makes it not assert for me. I'm still seeing some purify oddness.
I'm not yet convinced that this is not a result of the idl array support junk
added to libxpt in my tree. I'll work on that problem.

Comment 9

[Mike McCabe]

Shaver - your diffs look fine to me.

Is blowing the stack really a problem?

Also, I believe malloc was failing b/c one of its structures was corrupted - as
the specific manifestation of memory corruption is usually platform specific, I
wouldn't worry if you don't see that problem.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think the stack blowing is a real concern; I was seeing it when linking large
sets of files (*.xpt -> mozilla.xpt).  Is the iterative nature of my fix
particularily repellent?

Comment 11

[Mike McCabe]

Not at all, especially if you were noticing stack blowage.  I'm just not used to
seeing stack as a constrained resource.  Did DoAnnotation push a lot?

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't think it was extreme in its per-frame usage, but coop and I never took
pains to minimize (or even analyze) it.  When linking *.xpt, though, you can
have some 300 annotations to emit, which causes a pretty deep call chain.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fixes are in.