546415 - If you call VMPI_lockRelease twice on the same lock we should assert/crash

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Tommy Reilly]

Right now it just silently ignores the second lock release call

Comment 1

[Dan Smith]

Deferring to 10.2.

Comment 2

[Lars T Hansen]

Poaching, I'm currently looking into a related bug, we need to nail down the VMPI for this properly.

Comment 3

[Lars T Hansen]

Attachment: New VMPI spec for locking primitives

Changes documentation / semantics to being less lenient / easier to implement / less error prone; locks that are used when they're not in the appropriate state will cause asserts in debug builds.  This includes locking locked locks, unlocking unlocked locks, and unlocking locks held by other threads.  I need to know if this is too draconian.

Note I've dispensed with the return values for lockAcquire and lockRelease; they were almost never being checked, and when they were checked they were checked by GCAssert.  Instead the assertions in the platform layer make sure that that kind of checking is always happening.

Updates to the platform layers as well as to vm code will follow.

Comment 4

[Lars T Hansen]

Attachment: Implementation of VMPI locking primitives

This diff probably looks bigger than it is.  The changes are:

- changes in return types from bool to void in a couple of cases
- debug code on all platforms
- change in how the unix implementation is factored.

Tested on Mac only so far, consider it a little preliminary but good enough to review.  On Mac it appears to uncover a bug; regress/bug_515935.abc apparently leaves a lock hanging during OOM abort and that triggers an assert.  That probably means FixedMalloc needs to set up an unwind handler, but the bigger question is whether there are going to be similar problems for OOM handling elsewhere.

Comment 5

[Lars T Hansen]

Attachment: Changes to the GC code as a result of new VMPIs

Very gentle changes resulting from no longer returning values from VMPI_lockAcquire and VMPI_lockRelease.

Comment 6

[Edwin Smith]

Comment on attachment 444098 [details] [diff] [review]
New VMPI spec for locking primitives

Adding simon for feedback since this relates to bug 555760 which adds more VMPI concurrency apis.

Comment 7

[Simon Wilkinson]

One comment:
For VMPI_lockDestroy I can see the utility of documenting: "..The lock must not be held by any thread when this function is called."
But trying to actually ASSERT it is a race-condition.

Comment 8

[Tommy Reilly]

Comment on attachment 444099 [details] [diff] [review]
Implementation of VMPI locking primitives

rubber stamping, gave it a quick scan, just noticed that VMPI_lockRelease for mac has an = that should be a ==

Comment 9

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

(In reply to comment #8)
> (From update of attachment 444099 [details] [diff] [review])
> rubber stamping, gave it a quick scan, just noticed that VMPI_lockRelease for
> mac has an = that should be a ==

That bug appears elsewhere in the patch, e.g. in symbian-platform.h ...

Comment 10

[Lars T Hansen]

(In reply to comment #7)
> One comment:
> For VMPI_lockDestroy I can see the utility of documenting: "..The lock must not
> be held by any thread when this function is called."
> But trying to actually ASSERT it is a race-condition.

It's a race, but isn't it a benign one, in that the assert does not catch all the cases where the invariant is violated, but when it does fire there's definitely a violation?

Comment 11

[Lars T Hansen]

(Patch can't land without considering the need for an unwind handler around the FixedMalloc locks, at a minimum, and the consequences for the Flash Player are not understood.)

Comment 12

[Lars T Hansen]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 444099 [details] [diff] [review] [details])
> > rubber stamping, gave it a quick scan, just noticed that VMPI_lockRelease for
> > mac has an = that should be a ==
> 
> That bug appears elsewhere in the patch, e.g. in symbian-platform.h ...

I checked all the files and I found those two cases, no others.

Comment 13

[Lars T Hansen]

(Removing has-patch since we're still blocked on a reasonable unwind handler, see comment #11.)

Comment 14

[Lars T Hansen]

Removing targeting and unassigning; it's not clear that fixing this is actually desirable - ignoring the second call might be the right thing to do, or at most print a message in debug mode.