pion / srtp

A Go implementation of SRTP
MIT License
119 stars 49 forks source link

requires timeout of AcceptStream() #235

Closed lolgopher closed 1 year ago

lolgopher commented 1 year ago

Your environment.

What did you do?

What did you expect?

What happened?

// AcceptStream returns a stream to handle RTCP for a single SSRC
func (s *SessionSRTP) AcceptStream() (*ReadStreamSRTP, uint32, error) {
    stream, ok := <-s.newStream
    if !ok {
        return nil, 0, errStreamAlreadyClosed
    }

    readStream, ok := stream.(*ReadStreamSRTP)
    if !ok {
        return nil, 0, errFailedTypeAssertion
    }

    return readStream, stream.GetSSRC(), nil
}
func (s *session) start(localMasterKey, localMasterSalt, remoteMasterKey, remoteMasterSalt []byte, profile ProtectionProfile, child streamSession) error {
    var err error
    s.localContext, err = CreateContext(localMasterKey, localMasterSalt, profile, s.localOptions...)
    if err != nil {
        return err
    }

    s.remoteContext, err = CreateContext(remoteMasterKey, remoteMasterSalt, profile, s.remoteOptions...)
    if err != nil {
        return err
    }

    go func() {
        defer func() {
            close(s.newStream)

            s.readStreamsLock.Lock()
            s.readStreamsClosed = true
            s.readStreamsLock.Unlock()
            close(s.closed)
        }()

        b := make([]byte, 8192)
        for {
            var i int
            i, err = s.nextConn.Read(b)
            if err != nil {
                if !errors.Is(err, io.EOF) {
                    s.log.Error(err.Error())
                }
                return
            }

            if err = child.decrypt(b[:i]); err != nil {
                s.log.Info(err.Error())
            }
        }
    }()

    close(s.started)

    return nil
}

When calling AcceptStream(), it waits for a new Stream to come in via stream, ok := <-s.newStream. However, if there is an error on the sender side or a network problem preventing packets from being properly transmitted, s.nextConn.Read(b) will not be able to proceed in session.start(). This will cause AcceptStream() to wait indefinitely.

To prevent situations where AcceptStream() cannot proceed, SetReadDeadline() should be set on nextConn.

Sean-Der commented 1 year ago

That makes sense to me! @lolgopher are you interested in opening a PR for this?

lolgopher commented 1 year ago

Thanks, I'm interested in opening a PR for this. :)

I will try @Sean-Der