Repository: pion/webrtc (go)

Issue:
[Crash] coredump while invoke  ICEGatherer.GetLocalCandidates after ICEGatherer closed
my server run into a crash, and I thought it's caused by concurrent ice closed. coredump stack pasted below. I'm not very sure,  thanks for your help.

```
#1  0x00000000004b1045 in runtime.dieFromSignal (sig=6) at runtime/signal_unix.go:769
#2  0x00000000004b1716 in runtime.sigfwdgo (sig=6, info=<optimized out>, ctx=<optimized out>) at runtime/signal_unix.go:983
#3  0x00000000004aff07 in runtime.sigtrampgo (sig=6, info=0x0, ctx=0xffffffffffffffff) at runtime/signal_unix.go:417
#4  0x00000000004d02ae in runtime.sigtrampgo (sig=6, info=0xc0012e74b0, ctx=0xc0012e7380) at <autogenerated>:1
#5  0x00000000004cf7dd in runtime.sigtramp () at runtime/sys_linux_amd64.s:343
#6  <signal handler called>
#7  runtime.raise () at runtime/sys_linux_amd64.s:165
#8  0x00000000004b1045 in runtime.dieFromSignal (sig=6) at runtime/signal_unix.go:769
#9  0x000000000049b00d in runtime.crash () at runtime/signal_unix.go:861
#10 runtime.fatalpanic (msgs=<optimized out>) at runtime/panic.go:1298
#11 0x000000000049a828 in runtime.gopanic (e=...) at runtime/panic.go:1147
#12 0x00000000004b0f07 in runtime.panicmem () at runtime/panic.go:221
#13 runtime.sigpanic () at runtime/signal_unix.go:735
#14 0x000000000085669a in github.com/pion/ice/v2.(*Agent).context (a=0x0) at github.com/pion/ice/v2@v2.2.2/context.go:9
#15 github.com/pion/ice/v2.(*Agent).GetLocalCandidates (a=0x0) at github.com/pion/ice/v2@v2.2.2/agent.go:849
#16 0x00000000008a1ff3 in github.com/pion/webrtc/v3.(*ICEGatherer).GetLocalCandidates (g=0xc0210919e0)
    at github.com/pion/webrtc/v3@v3.1.24/icegatherer.go:218
#17 0x00000000008cc0a5 in github.com/pion/webrtc/v3.populateLocalCandidates (sessionDescription=0xc0cd40f4a0, i=0x0, 
    iceGatheringState=-1) at github.com/pion/webrtc/v3@v3.1.24/sdp.go:318
#18 0x00000000008bad7e in github.com/pion/webrtc/v3.(*PeerConnection).PendingLocalDescription (pc=0xc035ef5200)
    at github.com/pion/webrtc/v3@v3.1.24/peerconnection.go:2022
#19 0x00000000008b27fe in github.com/pion/webrtc/v3.(*PeerConnection).LocalDescription (pc=0xc035ef5200)
    at github.com/pion/webrtc/v3@v3.1.24/peerconnection.go:1005
(gdb) frame 16
#16 0x00000000008a1ff3 in github.com/pion/webrtc/v3.(*ICEGatherer).GetLocalCandidates (g=0xc0210919e0)
    at github.com/pion/webrtc/v3@v3.1.24/icegatherer.go:218
218	in github.com/pion/webrtc/v3@v3.1.24/icegatherer.go
(gdb) p g.state
$8 = 4
```

state 4 is ICEGathererStateClosed. and agent is set to nil when Close. And createAgent will return nil after Close because of the state != ICEGathererStateNew but g.agent is nil . and use g.agent directly when createAgent nil will lead to coredump.
```
func (g *ICEGatherer) Close() error {
        g.lock.Lock()
        defer g.lock.Unlock()

        if g.agent == nil {
                return nil 
        } else if err := g.agent.Close(); err != nil {
                return err 
        }   

        g.agent = nil 
        g.setState(ICEGathererStateClosed)                                                                                                                                                                                                                   

        return nil 
}
```

```
func (g *ICEGatherer) createAgent() error {
        g.lock.Lock()
        defer g.lock.Unlock()

        if g.agent != nil || g.State() != ICEGathererStateNew {
                return nil
        }
        ..........
```


/// Code like this will coredump if g.agent==nil and g.State() == ICEGathererStateClosed

```
// GetLocalParameters returns the ICE parameters of the ICEGatherer.
func (g *ICEGatherer) GetLocalParameters() (ICEParameters, error) {
        if err := g.createAgent(); err != nil {
                return ICEParameters{}, err
        }

        frag, pwd, err := g.agent.GetLocalUserCredentials()
        if err != nil {
                return ICEParameters{}, err
        }
        
        return ICEParameters{
                UsernameFragment: frag, 
                Password:         pwd,
                ICELite:          false,
        }, nil  
}
                
// GetLocalCandidates returns the sequence of valid local candidates associated with the ICEGatherer.
func (g *ICEGatherer) GetLocalCandidates() ([]ICECandidate, error) {
        if err := g.createAgent(); err != nil {
                return nil, err
        }
        iceCandidates, err := g.agent.GetLocalCandidates()
        if err != nil {
                return nil, err
        }

        return newICECandidatesFromICE(iceCandidates)
}

```

Pull request description:
Fix panics in ICEGatherer when it is already closed
#### Description

ICEGatherer throws panics in GetLocalCandidates(), GetLocalParameters() when it is already closed.
We have seen these panics during our load tests even though the PR #2230 has been merged.

This PR fixes these panics and added some test codes.

#### Reference issue

Fixes #2261 
Related to #2230 


Patch:
diff --git a/icegatherer.go b/icegatherer.go
index 5e94368a..ab7e76db 100644
--- a/icegatherer.go
+++ b/icegatherer.go
@@ -142,10 +142,7 @@ func (g *ICEGatherer) Gather() error {
 		return err
 	}
 
-	g.lock.Lock()
-	agent := g.agent
-	g.lock.Unlock()
-
+	agent := g.getAgent()
 	// it is possible agent had just been closed
 	if agent == nil {
 		return fmt.Errorf("%w: unable to gather", errICEAgentNotExist)
@@ -205,7 +202,13 @@ func (g *ICEGatherer) GetLocalParameters() (ICEParameters, error) {
 		return ICEParameters{}, err
 	}
 
-	frag, pwd, err := g.agent.GetLocalUserCredentials()
+	agent := g.getAgent()
+	// it is possible agent had just been closed
+	if agent == nil {
+		return ICEParameters{}, fmt.Errorf("%w: unable to get local parameters", errICEAgentNotExist)
+	}
+
+	frag, pwd, err := agent.GetLocalUserCredentials()
 	if err != nil {
 		return ICEParameters{}, err
 	}
@@ -222,7 +225,14 @@ func (g *ICEGatherer) GetLocalCandidates() ([]ICECandidate, error) {
 	if err := g.createAgent(); err != nil {
 		return nil, err
 	}
-	iceCandidates, err := g.agent.GetLocalCandidates()
+
+	agent := g.getAgent()
+	// it is possible agent had just been closed
+	if agent == nil {
+		return nil, fmt.Errorf("%w: unable to get local candidates", errICEAgentNotExist)
+	}
+
+	iceCandidates, err := agent.GetLocalCandidates()
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/icegatherer_test.go b/icegatherer_test.go
index 9e37a59d..280f1a4c 100644
--- a/icegatherer_test.go
+++ b/icegatherer_test.go
@@ -98,3 +98,58 @@ func TestICEGather_mDNSCandidateGathering(t *testing.T) {
 	<-gotMulticastDNSCandidate.Done()
 	assert.NoError(t, gatherer.Close())
 }
+
+func TestICEGatherer_AlreadyClosed(t *testing.T) {
+	// Limit runtime in case of deadlocks
+	lim := test.TimeOut(time.Second * 20)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	opts := ICEGatherOptions{
+		ICEServers: []ICEServer{{URLs: []string{"stun:stun.l.google.com:19302"}}},
+	}
+
+	t.Run("Gather", func(t *testing.T) {
+		gatherer, err := NewAPI().NewICEGatherer(opts)
+		assert.NoError(t, err)
+
+		err = gatherer.createAgent()
+		assert.NoError(t, err)
+
+		err = gatherer.Close()
+		assert.NoError(t, err)
+
+		err = gatherer.Gather()
+		assert.ErrorIs(t, err, errICEAgentNotExist)
+	})
+
+	t.Run("GetLocalParameters", func(t *testing.T) {
+		gatherer, err := NewAPI().NewICEGatherer(opts)
+		assert.NoError(t, err)
+
+		err = gatherer.createAgent()
+		assert.NoError(t, err)
+
+		err = gatherer.Close()
+		assert.NoError(t, err)
+
+		_, err = gatherer.GetLocalParameters()
+		assert.ErrorIs(t, err, errICEAgentNotExist)
+	})
+
+	t.Run("GetLocalCandidates", func(t *testing.T) {
+		gatherer, err := NewAPI().NewICEGatherer(opts)
+		assert.NoError(t, err)
+
+		err = gatherer.createAgent()
+		assert.NoError(t, err)
+
+		err = gatherer.Close()
+		assert.NoError(t, err)
+
+		_, err = gatherer.GetLocalCandidates()
+		assert.ErrorIs(t, err, errICEAgentNotExist)
+	})
+}