Skip to content

Refactor STUN client for safety, performance, and maintainability - #59

Open
ErickXavier wants to merge 2 commits into
ccding:masterfrom
ErickXavier:refactor-stun
Open

Refactor STUN client for safety, performance, and maintainability#59
ErickXavier wants to merge 2 commits into
ccding:masterfrom
ErickXavier:refactor-stun

Conversation

@ErickXavier

@ErickXavier ErickXavier commented Jun 30, 2026

Copy link
Copy Markdown

This PR refactors the STUN client for safety, performance, and usability:

Detailed Changes:

  • stun/attribute.go: Added safety bounds checking to rawAddr and xorAddr to prevent runtime panics on truncated or malformed attributes.
  • stun/client.go: Extracted connection setup logic into acquireConn to eliminate code duplication, and used net.JoinHostPort to fix IPv6 literal address resolution.
  • stun/log.go: Redirected logger outputs to os.Stderr to prevent stdout pollution when using verbose mode.
  • stun/net.go: Propagated packet parsing errors from newResponse to the send/receive loop.
  • stun/packet.go: Decoupled parsed attributes from the temporary socket read buffer by copying parsed byte slices, and optimized packet serialization (bytes()) to pre-allocate the output buffer.
  • stun/response.go: Updated newResponse to handle and propagate attribute decoding errors.
  • stun/tests.go: Cleaned up the return values in sendWithLog.
  • stun/const.go: Updated the default STUN server to stun.sipgate.net:10000 since stun.ekiga.net has been retired.
  • Unit & Integration Tests: Added a mock net.PacketConn and comprehensive tests to verify DiscoverNATType state machine and error handling logic.

@ccding ccding left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found two protocol-correctness regressions in the packet serialization/parsing changes. Both are reproducible with focused tests; details are inline.

Comment thread stun/packet.go
binary.BigEndian.PutUint16(packetBytes[0:2], v.types)
binary.BigEndian.PutUint16(packetBytes[2:4], v.length)
packetBytes = append(packetBytes, v.transID...)
size := 20 + int(v.length)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the FINGERPRINT CRC input boundary

sendBindingReq temporarily increments pkt.length by 8 before calling newFingerprintAttribute, so the header advertises the FINGERPRINT while the CRC input still excludes that attribute. Because this allocation now uses v.length, packet.bytes() returns eight unwritten zero bytes and newFingerprintAttribute hashes them too. As a result every generated request carries an invalid FINGERPRINT. Please either return only buf[:offset] here (while retaining the advertised header length) or make the fingerprint calculation explicitly hash only the bytes through the last serialized attribute.

Comment thread stun/packet.go
return nil, errors.New("Received data format mismatch")
}
value := packetBytes[pos+4 : end]
value := make([]byte, length)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the wire length before padding parsed attributes

This copied slice is passed to newAttribute, which pads it to a multiple of four and overwrites attribute.length with the padded size. A MAPPED-ADDRESS declaring only 5 bytes therefore becomes 8 bytes before rawAddr validates it, and is accepted as an address such as 127.0.0.0 instead of producing the intended decoding error (lengths 6/7 and IPv6 lengths 17–19 have the same issue). Please preserve the declared length for parsed attributes, or use a parsing constructor that does not turn wire padding into attribute data, and validate the exact family-specific length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants