Repository: pion/sctp
Pull request #102 (2020-01-31 09:27:57)

Tear down assocation on ABORT chunk
Properly tear down the association when we receive an ABORT
chunk.

Sean-Der: @enobufs I am going to write a test to confirm this doesn't regress as well, but ran out of time tonight!
codecov-io: # [Codecov](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=h1) Report
> Merging [#102](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=desc) into [master](https://codecov.io/gh/pion/sctp/commit/32f160c5ee1fe015c168a0fc8272633b73c5e4bf?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `21.42%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/sctp/pull/102/graphs/tree.svg?width=650&token=AB8S3U6xR9&height=150&src=pr)](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   77.05%   77.01%   -0.04%     
==========================================
  Files          43       43              
  Lines        2711     2715       +4     
==========================================
+ Hits         2089     2091       +2     
- Misses        495      498       +3     
+ Partials      127      126       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `77.01% <21.42%> (-0.04%)` | :arrow_down: |
| #wasm | `69.65% <7.14%> (-0.14%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [association.go](https://codecov.io/gh/pion/sctp/pull/102/diff?src=pr&el=tree#diff-YXNzb2NpYXRpb24uZ28=) | `82.4% <21.42%> (-0.13%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=footer). Last update [32f160c...78f8122](https://codecov.io/gh/pion/sctp/pull/102?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

enobufs: @Sean-Der Just to be sure that it can handle multiple errorCauses, I have added a marshaling test. I think your changes look good!
Sean-Der: @enobufs thank you so much. I appreciate how much work you have put forward :)

Patch:
diff --git a/association.go b/association.go
index e579f2c..e47195d 100644
--- a/association.go
+++ b/association.go
@@ -426,11 +426,12 @@ func (a *Association) readLoop() {
 
 		atomic.AddUint64(&a.bytesReceived, uint64(n))
 		if err = a.handleInbound(buffer[:n]); err != nil {
-			a.log.Warnf("[%s] %s]", a.name, errors.Wrap(err, "failed to push SCTP packet").Error())
+			closeErr = err
+			break
 		}
 	}
 
-	a.log.Debugf("[%s] readLoop exited", a.name)
+	a.log.Debugf("[%s] readLoop exited %s %s", a.name, closeErr)
 }
 
 func (a *Association) writeLoop() {
@@ -487,19 +488,20 @@ func (a *Association) unregisterStream(s *Stream, err error) {
 func (a *Association) handleInbound(raw []byte) error {
 	p := &packet{}
 	if err := p.unmarshal(raw); err != nil {
-		return errors.Wrap(err, "unable to parse SCTP packet")
+		a.log.Warnf("[%s] unable to parse SCTP packet %s", a.name, err)
+		return nil
 	}
 
 	if err := checkPacket(p); err != nil {
-		return errors.Wrap(err, "failed validating packet")
+		a.log.Warnf("[%s] failed validating packet %s", a.name, err)
+		return nil
 	}
 
 	a.handleChunkStart(p)
 
 	for _, c := range p.chunks {
-		err := a.handleChunk(p, c)
-		if err != nil {
-			a.log.Warnf("[%s] %s", a.name, errors.Wrap(err, "failed handling chunk").Error())
+		if err := a.handleChunk(p, c); err != nil {
+			return err
 		}
 	}
 
@@ -1963,8 +1965,8 @@ func (a *Association) handleChunk(p *packet, c chunk) error {
 	var err error
 
 	if _, err = c.check(); err != nil {
-		return errors.Wrap(err, "failed validating chunk")
-		// TODO: Create ABORT
+		a.log.Errorf("[ %s ] failed validating chunk: %s ", a.name, err)
+		return nil
 	}
 
 	switch c := c.(type) {
@@ -1979,7 +1981,7 @@ func (a *Association) handleChunk(p *packet, c chunk) error {
 		for _, e := range c.errorCauses {
 			errStr += fmt.Sprintf("(%s)", e)
 		}
-		a.log.Debugf("[%s] Abort chunk, with following errors: %s", a.name, errStr)
+		return fmt.Errorf("[%s] Abort chunk, with following errors: %s", a.name, errStr)
 
 	case *chunkError:
 		var errStr string
@@ -2013,8 +2015,10 @@ func (a *Association) handleChunk(p *packet, c chunk) error {
 		err = errors.Errorf("unhandled chunk type")
 	}
 
+	// Log and return, the only condition that is fatal is a ABORT chunk
 	if err != nil {
-		return err
+		a.log.Errorf("Failed to handle chunk: %v", err)
+		return nil
 	}
 
 	if len(packets) > 0 {
diff --git a/chunk_abort.go b/chunk_abort.go
index aef1a8e..caae026 100644
--- a/chunk_abort.go
+++ b/chunk_abort.go
@@ -58,7 +58,17 @@ func (a *chunkAbort) unmarshal(raw []byte) error {
 	return nil
 }
 func (a *chunkAbort) marshal() ([]byte, error) {
-	return nil, errors.Errorf("Unimplemented")
+	a.chunkHeader.typ = ctAbort
+	a.flags = 0x00
+	a.raw = []byte{}
+	for _, ec := range a.errorCauses {
+		raw, err := ec.marshal()
+		if err != nil {
+			return nil, err
+		}
+		a.raw = append(a.raw, raw...)
+	}
+	return a.chunkHeader.marshal()
 }
 
 func (a *chunkAbort) check() (abort bool, err error) {


Test patch:
diff --git a/association_test.go b/association_test.go
index c0a18d5..4778203 100644
--- a/association_test.go
+++ b/association_test.go
@@ -2186,6 +2186,39 @@ func TestAssocReset(t *testing.T) {
 	})
 }
 
+func TestAssocAbort(t *testing.T) {
+	const si uint16 = 1
+	br := test.NewBridge()
+
+	a0, a1, err := createNewAssociationPair(br, ackModeNoDelay, 0)
+	assert.NoError(t, err)
+
+	abort := &chunkAbort{
+		errorCauses: []errorCause{&errorCauseProtocolViolation{
+			errorCauseHeader: errorCauseHeader{code: protocolViolation},
+		}},
+	}
+	packet, err := a0.createPacket([]chunk{abort}).marshal()
+	assert.NoError(t, err)
+
+	_, _, err = establishSessionPair(br, a0, a1, si)
+	assert.NoError(t, err)
+
+	// Both associations are established
+	assert.Equal(t, established, a0.getState())
+	assert.Equal(t, established, a1.getState())
+
+	_, err = a0.netConn.Write(packet)
+	assert.NoError(t, err)
+	br.Process()
+
+	// The receiving association should be closed because it got an ABORT
+	assert.Equal(t, established, a0.getState())
+	assert.Equal(t, closed, a1.getState())
+
+	closeAssociationPair(br, a0, a1)
+}
+
 type fakeEchoConn struct {
 	echo     chan []byte
 	done     chan struct{}
diff --git a/chunk_abort_test.go b/chunk_abort_test.go
new file mode 100644
index 0000000..998c4ae
--- /dev/null
+++ b/chunk_abort_test.go
@@ -0,0 +1,57 @@
+package sctp
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAbortChunk(t *testing.T) {
+	t.Run("One error cause", func(t *testing.T) {
+		abort1 := &chunkAbort{
+			errorCauses: []errorCause{&errorCauseProtocolViolation{
+				errorCauseHeader: errorCauseHeader{code: protocolViolation},
+			}},
+		}
+		bytes, err := abort1.marshal()
+		assert.NoError(t, err, "should succeed")
+
+		abort2 := &chunkAbort{}
+		err = abort2.unmarshal(bytes)
+		assert.NoError(t, err, "should succeed")
+		assert.Equal(t, 1, len(abort2.errorCauses), "should have only one cause")
+		assert.Equal(t,
+			abort1.errorCauses[0].errorCauseCode(),
+			abort2.errorCauses[0].errorCauseCode(),
+			"errorCause code should match")
+	})
+
+	t.Run("Many error causes", func(t *testing.T) {
+		abort1 := &chunkAbort{
+			errorCauses: []errorCause{
+				&errorCauseProtocolViolation{
+					errorCauseHeader: errorCauseHeader{code: invalidMandatoryParameter},
+				},
+				&errorCauseProtocolViolation{
+					errorCauseHeader: errorCauseHeader{code: unrecognizedChunkType},
+				},
+				&errorCauseProtocolViolation{
+					errorCauseHeader: errorCauseHeader{code: protocolViolation},
+				},
+			},
+		}
+		bytes, err := abort1.marshal()
+		assert.NoError(t, err, "should succeed")
+
+		abort2 := &chunkAbort{}
+		err = abort2.unmarshal(bytes)
+		assert.NoError(t, err, "should succeed")
+		assert.Equal(t, 3, len(abort2.errorCauses), "should have only one cause")
+		for i, errorCause := range abort1.errorCauses {
+			assert.Equal(t,
+				errorCause.errorCauseCode(),
+				abort2.errorCauses[i].errorCauseCode(),
+				"errorCause code should match")
+		}
+	})
+}