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.
Copilot uses AI. Check for mistakes.
| 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.
Copilot uses AI. Check for mistakes.
The old code will cause parsed IPv4 without version, srcIP, dstIP, and more fields, but only have options.