dict: resolve unknown vendor AVPs to Unknown, not a same-code base AVP#255
Open
pbosch wants to merge 1 commit into
Open
dict: resolve unknown vendor AVPs to Unknown, not a same-code base AVP#255pbosch wants to merge 1 commit into
pbosch wants to merge 1 commit into
Conversation
FindAVPByCode falls back to a vendor-agnostic lookup when an exact
{appid, code, vendorID} match is missing. For a vendor-specific AVP that
the dictionary does not define, this can match an unrelated base AVP that
shares the same code (e.g. 3GPP code 5 / vendor 10415 resolving to base
NAS-Port, code 5 / vendor 0). The AVP then decodes under the wrong type,
its reported length stops matching the wire bytes, and the surrounding
message is parsed from the wrong offset — ending in a nil-pointer panic in
decodeAVPs that ServeConn recovers by closing the connection.
Every AVP is already indexed under both its real vendor and
UndefinedVendorID, and inheritance is pre-merged into child apps, so the
exact-match lookup already resolves every valid query. The fallback only
ever fires for a genuinely undefined vendor AVP, where the correct result
is Unknown (RFC 6733 §4.1, §11.1.1). Drop it.
Adds FindAVPByCode unit tests (exact match, inheritance, unknown vendor)
and decode-level tests asserting an unknown vendor AVP decodes as Unknown
and a message carrying one parses without desync or panic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #254.
Problem
FindAVPByCodefalls back to a vendor-agnostic lookup when an exact{appid, code, vendorID}match is missing. For a vendor-specific AVP the dictionary does not define, this can match an unrelated base AVP sharing the same code — e.g. 3GPP3GPP-GPRS-Negotiated-QoS-Profile(code 5, vendor 10415, variable-length) resolving to baseNAS-Port(code 5, vendor 0,Unsigned32). The AVP then decodes under the wrong type,(*AVP).Len()no longer matches the wire length,decodeAVPsadvances by the wrong offset, and a later iteration nil-derefs in(*AVP).Len().ServeConnrecovers by closing the connection, so one such message drops the connection unanswered.Per RFC 6733 §4.1 an AVP is identified by its Code combined with its Vendor-Id, and §11.1.1 keeps each vendor's code space disjoint from the IETF (vendor 0) space. The fallback crosses that boundary.
Fix
Drop the vendor-agnostic fallback. Every AVP is already indexed under both its real vendor and
UndefinedVendorID(parser.go), and inheritance is pre-merged into child apps bymergeInheritedAVPs, so the exact-match lookup already resolves every valid query and every inherited base AVP. The fallback only ever fired for a genuinely undefined vendor AVP, where the correct result isUnknown— full payload, correct length, no desync.This removes a map lookup rather than adding one, so the fast path is unchanged. A caller that explicitly passes
UndefinedVendorIDstill matches the vendor-agnostic alias on the first line, so name/vendor-agnostic lookups are unaffected.Tests
TestFindAVPByCode: exact match, inherited base AVP, inheritance through the full parent chain, and unknown vendor →Unknown-5-10415(not base NAS-Port).TestDecodeAVPUnknownVendor: an unknown vendor AVP decodes asUnknownwith the full payload andLen()equal to the wire length.TestReadMessageUnknownVendorAVP: a full message carrying such an AVP parses without desync or panic.All of
./diam/...passes with-race -cover.