fix wrong return when parse ip options#120
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes premature termination in IPv4 option parsing to ensure full header fields (Checksum, SrcIP, DstIP) are populated, and adds a new test case for multiple option types.
- Removed the early
returnin the options loop so subsequent header fields are set after options. - Preserved parsed padding by removing
ip.Padding = nilat the end ofDecodeFromBytes. - Added a subtest in
TestIPv4Optionsfor packets with multiple options and wrapped tests int.Run.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| layers/ip4.go | Removed premature return in options parsing, adjusted loop label and padding handling |
| layers/ip4_test.go | Added a new multi-option test case, introduced t.Run, and moved ip4 initialization into subtests |
Comments suppressed due to low confidence (1)
layers/ip4_test.go:195
- [nitpick] The comment suggests reusing
ip4across iterations, butip4is now reinitialized inside each subtest. Consider updating or removing this comment to reflect the current initialization strategy.
var ip4 IPv4 // reuse ip4 to test reset
| return nil | ||
| headerOptionsData = headerOptionsData[1:] | ||
| break pullOutOptions | ||
| case 1: // 1 byte padding |
There was a problem hiding this comment.
The code for option type 1 (NOP) does not append the parsed option to ip.Options, so OptionType 1 entries are omitted. Consider adding ip.Options = append(ip.Options, opt) before advancing headerOptionsData.
| if !bytes.Equal(ip4.Padding, test.padding) { | ||
| t.Fatalf("Padding mismatch.\nGot:\n%#v\nExpected:\n%#v\n", ip4.Padding, test.padding) | ||
| } | ||
| t.Run(test.packet, func(t *testing.T) { |
There was a problem hiding this comment.
[nitpick] The table-driven test uses the range variable test inside t.Run closures, which can lead to unexpected captures if tests run in parallel. Consider shadowing the loop variable (e.g., test := test) before invoking t.Run to bind the current value.
The old code will cause parsed IPv4 without version, srcIP, dstIP, and more fields, but only have options.