runtime: fix race condition between timer and event handler

This change fixes a race condition between beforeIdle waking up the
innermost event handler and a timer causing a different goroutine to
wake up at the exact same moment. This messes up the wasm event handling
and leads to memory corruption. The solution is to make beforeIdle
return the goroutine that must run next and have findrunnable pick
this goroutine without considering timers again.

Fixes #38093
Fixes #38574

Change-Id: Iffbe99411d25c2730953d1c8b0741fd892f8e540
Reviewed-on: https://go-review.googlesource.com/c/go/+/230178
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

diff --git a/src/runtime/lock_futex.go b/src/runtime/lock_futex.go
index 29b7be0d8f6e412c3207b1caaeac5231e0d97dfb..91467fdfae0dab5b5c2b0de95bc9e3e09be13af5 100644 (file)
--- a/src/runtime/lock_futex.go
+++ b/src/runtime/lock_futex.go
@@ -238,8 +238,8 @@ func notetsleepg(n *note, ns int64) bool {
        return ok
 }
 
-func beforeIdle(int64) bool {
-       return false
+func beforeIdle(int64) (*g, bool) {
+       return nil, false
 }
 
 func checkTimeouts() {}

diff --git a/src/runtime/lock_js.go b/src/runtime/lock_js.go
index 429ce639231f74b6fa621de73518b338b3501244..14bdc76842cc4749a68d6e7b40ae6d54bf597807 100644 (file)
--- a/src/runtime/lock_js.go
+++ b/src/runtime/lock_js.go
@@ -173,7 +173,9 @@ var idleID int32
 // beforeIdle gets called by the scheduler if no goroutine is awake.
 // If we are not already handling an event, then we pause for an async event.
 // If an event handler returned, we resume it and it will pause the execution.
-func beforeIdle(delay int64) bool {
+// beforeIdle either returns the specific goroutine to schedule next or
+// indicates with otherReady that some goroutine became ready.
+func beforeIdle(delay int64) (gp *g, otherReady bool) {
        if delay > 0 {
                clearIdleID()
                if delay < 1e6 {
@@ -190,15 +192,14 @@ func beforeIdle(delay int64) bool {
 
        if len(events) == 0 {
                go handleAsyncEvent()
-               return true
+               return nil, true
        }
 
        e := events[len(events)-1]
        if e.returned {
-               goready(e.gp, 1)
-               return true
+               return e.gp, false
        }
-       return false
+       return nil, false
 }
 
 func handleAsyncEvent() {

diff --git a/src/runtime/lock_sema.go b/src/runtime/lock_sema.go
index bf2584ac929fbe7337850c90637b21afdf3b75d2..671e524e45885bfe6443126d6c17a7b9ed37d0e5 100644 (file)
--- a/src/runtime/lock_sema.go
+++ b/src/runtime/lock_sema.go
@@ -297,8 +297,8 @@ func notetsleepg(n *note, ns int64) bool {
        return ok
 }
 
-func beforeIdle(int64) bool {
-       return false
+func beforeIdle(int64) (*g, bool) {
+       return nil, false
 }
 
 func checkTimeouts() {}

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index e5823dd804740b871b328026f166943f09dd2ccc..766784c07edac57d0bbc1042302df23deb8104e7 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -2286,9 +2286,17 @@ stop:
 
        // wasm only:
        // If a callback returned and no other goroutine is awake,
-       // then pause execution until a callback was triggered.
-       if beforeIdle(delta) {
-               // At least one goroutine got woken.
+       // then wake event handler goroutine which pauses execution
+       // until a callback was triggered.
+       gp, otherReady := beforeIdle(delta)
+       if gp != nil {
+               casgstatus(gp, _Gwaiting, _Grunnable)
+               if trace.enabled {
+                       traceGoUnpark(gp, 0)
+               }
+               return gp, false
+       }
+       if otherReady {
                goto top
        }
 

diff --git a/test/fixedbugs/issue38093.go b/test/fixedbugs/issue38093.go
new file mode 100644 (file)
index 0000000..db92664
--- /dev/null
+++ b/test/fixedbugs/issue38093.go
@@ -0,0 +1,49 @@
+// +build js
+// run
+
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test race condition between timers and wasm calls that led to memory corruption.
+
+package main
+
+import (
+       "os"
+       "syscall/js"
+       "time"
+)
+
+func main() {
+       ch1 := make(chan struct{})
+
+       go func() {
+               for {
+                       time.Sleep(5 * time.Millisecond)
+                       ch1 <- struct{}{}
+               }
+       }()
+       go func() {
+               for {
+                       time.Sleep(8 * time.Millisecond)
+                       ch1 <- struct{}{}
+               }
+       }()
+       go func() {
+               time.Sleep(2 * time.Second)
+               os.Exit(0)
+       }()
+
+       for range ch1 {
+               ch2 := make(chan struct{}, 1)
+               f := js.FuncOf(func(this js.Value, args []js.Value) interface{} {
+                       ch2 <- struct{}{}
+                       return nil
+               })
+               defer f.Release()
+               fn := js.Global().Get("Function").New("cb", "cb();")
+               fn.Invoke(f)
+               <-ch2
+       }
+}