Add Stream buffer limit to WriteSCTP configurable via environment var.

[geenkel]

No description provided.

[Copilot]

Pull request overview

This PR adds configurable stream buffer limits to WriteSCTP operations via the SCTP_STREAM_BUFFER_LIMIT environment variable. The implementation helps prevent excessive buffering by rejecting writes that would exceed the configured limit.

• Adds bufferLimit field to the Stream struct to store the configured limit per stream
• Implements buffer limit checking in WriteSCTP before accepting new data
• Reads and parses the environment variable during stream creation in createStream()

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
stream.go	Adds bufferLimit field to Stream struct and implements buffer limit check in WriteSCTP that rejects writes exceeding the limit
association.go	Reads SCTP_STREAM_BUFFER_LIMIT environment variable during stream creation and initializes the bufferLimit field

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Using fmt.Sscanf with %d format specifier can parse negative numbers into the uint64 variable, which would result in a very large positive value due to unsigned integer wrapping. Consider using strconv.ParseUint instead, which will return an error for negative values and provide better control over the parsing behavior.

[Copilot]

There's a race condition between checking the buffer limit and incrementing the buffered amount. After the check passes (line 278) but before packetize() increments bufferedAmount (line 386), another goroutine could write data. This could allow the buffer to exceed the limit. The buffer limit check should be moved inside the packetize() function under the same lock that increments bufferedAmount, or the lock should be held across both the check and the increment.

Suggested change

	// Check buffer limit.
	if s.bufferLimit > 0 {
	s.lock.RLock()
	currentBuffered := s.bufferedAmount
	s.lock.RUnlock()
	if currentBuffered+uint64(len(payload)) > s.bufferLimit {
	return 0, fmt.Errorf("stream buffer limit exceeded: %d bytes", s.bufferLimit)
	}
	// Check and update buffer under a single lock to avoid races.
	if s.bufferLimit > 0 {
	s.lock.Lock()
	prospective := s.bufferedAmount + uint64(len(payload))
	if prospective > s.bufferLimit {
	s.lock.Unlock()
	return 0, fmt.Errorf("stream buffer limit exceeded: %d bytes", s.bufferLimit)
	}
	s.bufferedAmount = prospective
	s.lock.Unlock()
	} else {
	// Keep bufferedAmount consistent even when there is no explicit limit.
	s.lock.Lock()
	s.bufferedAmount += uint64(len(payload))
	s.lock.Unlock()

[Copilot]

The environment variable is read and parsed on every stream creation, which could impact performance when many streams are created. Consider reading the environment variable once during Association initialization and passing the value to createStream(), or caching it in the Association struct.

[Copilot]

The condition 'err != nil || bufferLimit == 0' on line 1627 sets bufferLimit to 0 if parsing succeeds but yields a value of 0. This means a user cannot explicitly disable the limit by setting it to 0 in the environment variable - the only way to disable it is to not set the variable at all. If 0 should disable the limit, remove the '|| bufferLimit == 0' check. If 0 should be treated as invalid, consider logging a warning to help users debug misconfiguration.

Suggested change

	if err != nil || bufferLimit == 0 {
	if err != nil {

[Copilot]

The new buffer limit check logic lacks test coverage. Consider adding tests that verify the buffer limit is enforced correctly, including scenarios where writes succeed when under the limit, fail when exceeding the limit, and handle concurrent writes appropriately.