561402 - HeapBlock's need initialization in ExpandHeapInternal

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P2)

Description

[Tommy Reilly]

If VMPI_areNewPagesDirty() is true and we went down the path this path:

// if baseAddr was used for HeapBlocks split
 if((char*)newBlocks == baseAddr) 

and the nextREgion == NULL path after it the block used for the regions needs to be initialized before calling split.    Both paths that call Split should init the new "head" block.

Comment 1

[Tommy Reilly]

Attachment: Makes sure blocks get inited properly

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Tommy, what's up with the tab characters in the patch?

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(oh, this is for argo, sorry, I was trying to apply it to tr)

Comment 4

[Tommy Reilly]

Actually its a good point, I should use spaces so I can pull it from tr-argo to tr, I won't bother to repost the patch though

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Quick initial and very local feedback on local issues in the patch (while I am still getting my head around the context of the bug itself):

I do not understand why you are passing newPagesDirty() as the dirty flag to newBlock->init(..) on line 1308, when a few lines after that the dirty field of newBlock is explicitly set to block->dirty.  You should either pass block->dirty as the argument, or get rid of the assignment statement following init, or change the logic so that you are doing something with both values, but as it stands it does not make sense to me to have both involved in the computation when one is obviously supposed to be unused.

Also if if you have a concrete test case associated with this bug, it would useful for me.  But if you don't I will keep exploring on my own.

Comment 6

[Tommy Reilly]

+1, I'll fix and post a new patch.

This bug only reproduces in an environment where we aren't using virtual memory and new pages are dirty.    Symbian is like this.   We could force a crash by turning off virtual memory and poisoning memory returned from VMPI_allocateAlignedMemory (where memory comes from when virtual memory is off).   I'll post a patch that does that.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

I'm getting by for the moment by just locally changing the mac VMPI to return true from areNewPagesDirty  instead of false; (I am assuming that this is a conservative change with respect to correctness and should only make a difference with respect to performance).

We may want to consider trying to create a VMPI testing environment that does things like this, returning non-standard but conservative values for methods like this, in order to catch corner cases like this on things besides the odd mobile device...

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> This bug only reproduces in an environment where we aren't using virtual memory
> and new pages are dirty.

You said this only reproduces in an environment were we *aren't* using virtual memory, but the code in question is/was only invoking VMPI_areNewPagesDirty when we *are* using virtual memory (if I understand the meaning of "config.useVirtualMemory" correctly).

Did you misspeak about where this reproduces, or ... ?

Comment 9

[Tommy Reilly]

the bug occurs when fresh memory used to expand the heap isn't pre-zeroed.   That only occurs when using virtual memory AND VMPI_areNewPagesDirty is true or we aren't using virtual memory in which case we assume new memory is always dirty.    So to reproduce the bug you have to either have VMPI_areNewPagesDirty return true and have VMPI_commitMemory scribble, or turn off virtual memory and have VMPI_allocateAlignedMemory scribble.   The actual problem that occurred in the wild that lead to this bug is the second one.     Setting MallocScribble env var and having VMPI_useVirtualMemory return false would be a good test, trying that now...

Comment 10

[Tommy Reilly]

Attachment: re-create the problem

This patch shows the problem, asking for review to submit the ifdef DEBUG part of VMPI_allocateAlignedMemory.

Comment 11

[Tommy Reilly]

It turns out MallocScribble=1 has no affect on valloc

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Do we have an adversarial version of VMPI that we can swap in for testing purposes?

If not, we should either make one, or make a configure switch that turns the "standard host" VMPI implementation into an adversarial system that does things like scribble on memory (while of course conforming to our specs, so e.g. in this case such scribbling would also require changing the return value of areNewPagesDirty.

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: other way to re-create problem

(this is the other approach Tommy had originally suggested that I happened to be independently investigating this morning.  I'm just attaching it to this ticket for completeness.)

I think a "full-featured" adversarial VMPI would include both tommy's recreation change and this one (guarded under #ifdefs or put into an entirely separate VMPI-adversary.cpp file).

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 441787 [details] [diff] [review]
re-create the problem

The #ifdef DEBUG part looks fine to me.  (Obviously the change to useVirtualMemory should not land.)  You might add a comment pointing to this ticket with the memset line?

Anyway, I think it is a good idea to put this in, independently of my idea of an adversarial vmpi, which would have the much more substantive change (and not worthy of guarding under just DEBUG) of flipping areNewPagesDirty.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

One more tiny nitpick about the original patch: your Init function uses "this->foo = ..." to initialize some members but omits it for "committed = true;"  I can see that you have to do this since the Init parameter names are shadowing the member names, I'd prefer that you use the same style and include "this->" throughout the Init function definition.  (But man that is a tiny nit.)

Comment 16

[Tommy Reilly]

Attachment: ensure we don't regress this bug

For submission after fix is submitted, also guards against regresion in the case where we are using virtual memory and VMPI_areNewPagesDirty is true.    This covers the Symbian case that is currently untested otherwise.

Comment 17

[Tommy Reilly]

Attachment: Fix

Incorporates Felix's suggested simplification of the Split case, otherwise unchanged.   I opted not to change tabs to spaces, will make sure that happens in the tr-argo->tr merge which is where it should happen I think.

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 441798 [details] [diff] [review]
Fix

(last teensy nit still stands, but that's obviously not worth holding up this fix.)

Comment 19

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 441797 [details] [diff] [review]
ensure we don't regress this bug

Okay, looks like Tommy's not as worried as I am about changing code paths in debug builds... If Lars or Ed say its okay to change areNewPagesDirty based on DEBUG, then I'll go along with it, but I am not sure its an appropriate behavioral change to associate with that flag.  (Thus my suggestion of a separate adversarial mode.)

Comment 20

[Tommy Reilly]

Yeah I'm just following the FixedMalloc precedent and extending it.   I sometimes think that we should make Release/Debug builds exactly the same except for assertions, compiler optimizations and put these pedantic things under a new different feature but we've never done that.    I'm just going for getting good testing coverage under todays system.    Ie mac 10.5+ will use virtual memory and DEBUG builds will test VMPI_newPagesAreDirty and Mac 10.4 which doesn't use virtual memory will test the dirty VMPI_allocateAlignedMemory path.

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

aybe for Argo we should adopt iyuuor(In reply to comment #20)
> Yeah I'm just following the FixedMalloc precedent and extending it.   I
> sometimes think that we should make Release/Debug builds exactly the same
> except for assertions, compiler optimizations and put these pedantic things
> under a new different feature but we've never done that.    I'm just going for
> getting good testing coverage under todays system.

Right, seems sane (except that if a bug predicated on dirty=false arises in release builds for 10.5+ it may disappear under debug mode, which is frustrating.  But this is not news to you).

For Argo maybe we adopt your current approach and file a separate ticket suggesting the addition of a emulate_adversarial_environment feature?

Comment 22

[Felix S. Klock II [:pnkfelix, :fklock]]

(or "pedantic" rather than "adversarial", if you prefer that term.  :)

Comment 23

[Tommy Reilly]

Attachment: GCHeap::Commit doesn't respect VMPI_areNewPagesDirty()

fallout from new DEBUG code in sandbox builder

Comment 24

[Tommy Reilly]

sandbox green just waiting for SR, switching to Ed, since Lars is out

Comment 25

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 441797 [details] [diff] [review]
ensure we don't regress this bug

r+, apart from the issue about whether we should have a separate pedantic/adversarial mode separate from debug as discussed in comment 19 and below it.

Comment 26

[Lars T Hansen]

Comment on attachment 441797 [details] [diff] [review]
ensure we don't regress this bug

I don't think it's a good idea for this debugging code always to be on in debug builds, because that makes it harder to recreate release conditions in debug builds.

The appropriate fix here would be an ad-hoc #define in mmgc.h (like the one we have for MMGC_POLICY_PROFILING); the default can be "on" in the shell or even in the player for that matter, but it should be visible and it should be possible to disable it.

(Also it's good hygiene to use VMPI_memset, though not strictly necessary here.)

Comment 27

[Lars T Hansen]

Comment on attachment 441798 [details] [diff] [review]
Fix

There's no reason for the 'dirty' argument to Init to have a default value, it only invites bugs.  (If it does have a default value the default value should be true, or it should be a cookie that forces Init to go out and ask the VMPI layer.  Both seem wrong but I'm open to arguments.)

SR+ on the condition that that's fixed.

Comment 28

[Tommy Reilly]

tamarin-redux - changeset - 4650:acc98fa4ce04

Comment 29

[Tommy Reilly]

fix is submitted but leaving open until we agree upon a regression strategy.   There's conflict that we don't want to introduce any (more) artificial Debug/Release divergence but want to instead go with some compile time enabled extra testing code and need the build to also pick up on that so it gets run.    Maybe a "DEBUG_PARANOID" feature run during deep tests?

Comment 30

[Tommy Reilly]

Attachment: Instead of DEBUG use MMGC_POISON_MEMORY_FROM_OS

Idea is that this would be defined by some MMgc aggressive DEBUG feature

Comment 31

[Lars T Hansen]

Comment on attachment 471111 [details] [diff] [review]
Instead of DEBUG use MMGC_POISON_MEMORY_FROM_OS

The feature is fine but let's not reintroduce more magic poison constants.  Poison constants are all defined in GCHeap.h.

R+ on the condition that that's fixed.

Comment 32

[Tommy Reilly]

http://hg.mozilla.org/tamarin-redux/rev/3d509ea4d9ab