Change the way the IP options report problems

The error messages are not needed or used as these are not processing errors
so much as errors to be reported back to the packet sender. Implicitly
describe whether each error should generate ICMP packets or not. Most do
but there are a couple that do not.

Slightly alter some test expectations for Linux compatibility and add a
couple more. Improve Linux compatibility on error packet returns. Some
cosmetic changes to tests to match the upcoming packet impact version
of the same tests.

PiperOrigin-RevId: 352889785

5 files changed, 174 insertions, 117 deletions

diff --git a/pkg/tcpip/checker/checker.go b/pkg/tcpip/checker/checker.go
index 0ac2000ca..07b4393a4 100644
--- a/pkg/tcpip/checker/checker.go
+++ b/pkg/tcpip/checker/checker.go
@@ -1,4 +1,4 @@
-// Copyright 2018 The gVisor Authors.
+// Copyright 2021 The gVisor Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -234,7 +234,7 @@ func IPv4RouterAlert() NetworkChecker {
 		for {
 			opt, done, err := iterator.Next()
 			if err != nil {
-				t.Fatalf("error acquiring next IPv4 option %s", err)
+				t.Fatalf("error acquiring next IPv4 option at offset %d", err.Pointer)
 			}
 			if done {
 				break
diff --git a/pkg/tcpip/header/ipv4.go b/pkg/tcpip/header/ipv4.go
index e6103f4bc..48ca60319 100644
--- a/pkg/tcpip/header/ipv4.go
+++ b/pkg/tcpip/header/ipv4.go
@@ -1,4 +1,4 @@
-// Copyright 2018 The gVisor Authors.
+// Copyright 2021 The gVisor Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,7 +16,6 @@ package header
 
 import (
 	"encoding/binary"
-	"errors"
 	"fmt"
 
 	"gvisor.dev/gvisor/pkg/tcpip"
@@ -481,15 +480,13 @@ const (
 	IPv4OptionLengthOffset = 1
 )
 
-// Potential errors when parsing generic IP options.
-var (
-	ErrIPv4OptZeroLength   = errors.New("zero length IP option")
-	ErrIPv4OptDuplicate    = errors.New("duplicate IP option")
-	ErrIPv4OptInvalid      = errors.New("invalid IP option")
-	ErrIPv4OptMalformed    = errors.New("malformed IP option")
-	ErrIPv4OptionTruncated = errors.New("truncated IP option")
-	ErrIPv4OptionAddress   = errors.New("bad IP option address")
-)
+// IPv4OptParameterProblem indicates that a Parameter Problem message
+// should be generated, and gives the offset in the current entity
+// that should be used in that packet.
+type IPv4OptParameterProblem struct {
+	Pointer  uint8
+	NeedICMP bool
+}
 
 // IPv4Option is an interface representing various option types.
 type IPv4Option interface {
@@ -583,8 +580,9 @@ func (i *IPv4OptionIterator) Finalize() IPv4Options {
 // It returns
 // - A slice of bytes holding the next option or nil if there is error.
 // - A boolean which is true if parsing of all the options is complete.
-// - An error which is non-nil if an error condition was encountered.
-func (i *IPv4OptionIterator) Next() (IPv4Option, bool, error) {
+//   Undefined in the case of error.
+// - An error indication which is non-nil if an error condition was found.
+func (i *IPv4OptionIterator) Next() (IPv4Option, bool, *IPv4OptParameterProblem) {
 	// The opts slice gets shorter as we process the options. When we have no
 	// bytes left we are done.
 	if len(i.options) == 0 {
@@ -606,24 +604,22 @@ func (i *IPv4OptionIterator) Next() (IPv4Option, bool, error) {
 	// There are no more single byte options defined.  All the rest have a length
 	// field so we need to sanity check it.
 	if len(i.options) == 1 {
-		return nil, true, ErrIPv4OptMalformed
+		return nil, false, &IPv4OptParameterProblem{
+			Pointer:  i.ErrCursor,
+			NeedICMP: true,
+		}
 	}
 
 	optLen := i.options[IPv4OptionLengthOffset]
 
-	if optLen == 0 {
-		i.ErrCursor++
-		return nil, true, ErrIPv4OptZeroLength
-	}
+	if optLen <= IPv4OptionLengthOffset || optLen > uint8(len(i.options)) {
+		// The actual error is in the length (2nd byte of the option) but we
+		// return the start of the option for compatibility with Linux.
 
-	if optLen == 1 {
-		i.ErrCursor++
-		return nil, true, ErrIPv4OptMalformed
-	}
-
-	if optLen > uint8(len(i.options)) {
-		i.ErrCursor++
-		return nil, true, ErrIPv4OptionTruncated
+		return nil, false, &IPv4OptParameterProblem{
+			Pointer:  i.ErrCursor,
+			NeedICMP: true,
+		}
 	}
 
 	optionBody := i.options[:optLen]
@@ -635,7 +631,10 @@ func (i *IPv4OptionIterator) Next() (IPv4Option, bool, error) {
 	case IPv4OptionTimestampType:
 		if optLen < IPv4OptionTimestampHdrLength {
 			i.ErrCursor++
-			return nil, true, ErrIPv4OptMalformed
+			return nil, false, &IPv4OptParameterProblem{
+				Pointer:  i.ErrCursor,
+				NeedICMP: true,
+			}
 		}
 		retval := IPv4OptionTimestamp(optionBody)
 		return &retval, false, nil
@@ -643,7 +642,10 @@ func (i *IPv4OptionIterator) Next() (IPv4Option, bool, error) {
 	case IPv4OptionRecordRouteType:
 		if optLen < IPv4OptionRecordRouteHdrLength {
 			i.ErrCursor++
-			return nil, true, ErrIPv4OptMalformed
+			return nil, false, &IPv4OptParameterProblem{
+				Pointer:  i.ErrCursor,
+				NeedICMP: true,
+			}
 		}
 		retval := IPv4OptionRecordRoute(optionBody)
 		return &retval, false, nil
diff --git a/pkg/tcpip/network/ipv4/icmp.go b/pkg/tcpip/network/ipv4/icmp.go
index 3f60de749..284ad89cf 100644
--- a/pkg/tcpip/network/ipv4/icmp.go
+++ b/pkg/tcpip/network/ipv4/icmp.go
@@ -1,4 +1,4 @@
-// Copyright 2018 The gVisor Authors.
+// Copyright 2021 The gVisor Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -15,7 +15,6 @@
 package ipv4
 
 import (
-	"errors"
 	"fmt"
 
 	"gvisor.dev/gvisor/pkg/tcpip"
@@ -105,17 +104,12 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) {
 		} else {
 			op = &optionUsageReceive{}
 		}
-		aux, tmp, err := e.processIPOptions(pkt, opts, op)
-		if err != nil {
-			switch {
-			case
-				errors.Is(err, header.ErrIPv4OptDuplicate),
-				errors.Is(err, errIPv4RecordRouteOptInvalidLength),
-				errors.Is(err, errIPv4RecordRouteOptInvalidPointer),
-				errors.Is(err, errIPv4TimestampOptInvalidLength),
-				errors.Is(err, errIPv4TimestampOptInvalidPointer),
-				errors.Is(err, errIPv4TimestampOptOverflow):
-				_ = e.protocol.returnError(&icmpReasonParamProblem{pointer: aux}, pkt)
+		tmp, optProblem := e.processIPOptions(pkt, opts, op)
+		if optProblem != nil {
+			if optProblem.NeedICMP {
+				_ = e.protocol.returnError(&icmpReasonParamProblem{
+					pointer: optProblem.Pointer,
+				}, pkt)
 				e.protocol.stack.Stats().MalformedRcvdPackets.Increment()
 				e.stats.ip.MalformedPacketsReceived.Increment()
 			}
diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go
index 7f03696ae..f295a9192 100644
--- a/pkg/tcpip/network/ipv4/ipv4.go
+++ b/pkg/tcpip/network/ipv4/ipv4.go
@@ -1,4 +1,4 @@
-// Copyright 2018 The gVisor Authors.
+// Copyright 2021 The gVisor Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,7 +16,6 @@
 package ipv4
 
 import (
-	"errors"
 	"fmt"
 	"math"
 	"reflect"
@@ -780,17 +779,11 @@ func (e *endpoint) handlePacket(pkt *stack.PacketBuffer) {
 		// TODO(gvisor.dev/issue/4586):
 		// When we add forwarding support we should use the verified options
 		// rather than just throwing them away.
-		aux, _, err := e.processIPOptions(pkt, opts, &optionUsageReceive{})
-		if err != nil {
-			switch {
-			case
-				errors.Is(err, header.ErrIPv4OptDuplicate),
-				errors.Is(err, errIPv4RecordRouteOptInvalidPointer),
-				errors.Is(err, errIPv4RecordRouteOptInvalidLength),
-				errors.Is(err, errIPv4TimestampOptInvalidLength),
-				errors.Is(err, errIPv4TimestampOptInvalidPointer),
-				errors.Is(err, errIPv4TimestampOptOverflow):
-				_ = e.protocol.returnError(&icmpReasonParamProblem{pointer: aux}, pkt)
+		if _, optProblem := e.processIPOptions(pkt, opts, &optionUsageReceive{}); optProblem != nil {
+			if optProblem.NeedICMP {
+				_ = e.protocol.returnError(&icmpReasonParamProblem{
+					pointer: optProblem.Pointer,
+				}, pkt)
 				e.protocol.stack.Stats().MalformedRcvdPackets.Increment()
 				stats.ip.MalformedPacketsReceived.Increment()
 			}
@@ -1233,16 +1226,9 @@ func (*optionUsageEcho) actions() optionActions {
 	}
 }
 
-var (
-	errIPv4TimestampOptInvalidLength  = errors.New("invalid Timestamp length")
-	errIPv4TimestampOptInvalidPointer = errors.New("invalid Timestamp pointer")
-	errIPv4TimestampOptOverflow       = errors.New("overflow in Timestamp")
-	errIPv4TimestampOptInvalidFlags   = errors.New("invalid Timestamp flags")
-)
-
 // handleTimestamp does any required processing on a Timestamp option
 // in place.
-func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Address, clock tcpip.Clock, usage optionsUsage) (uint8, error) {
+func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Address, clock tcpip.Clock, usage optionsUsage) *header.IPv4OptParameterProblem {
 	flags := tsOpt.Flags()
 	var entrySize uint8
 	switch flags {
@@ -1253,7 +1239,10 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres
 		header.IPv4OptionTimestampWithPredefinedIPFlag:
 		entrySize = header.IPv4OptionTimestampWithAddrSize
 	default:
-		return header.IPv4OptTSOFLWAndFLGOffset, errIPv4TimestampOptInvalidFlags
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptTSOFLWAndFLGOffset,
+			NeedICMP: true,
+		}
 	}
 
 	pointer := tsOpt.Pointer()
@@ -1261,7 +1250,10 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres
 	// Since the pointer is 1 based, and the header is 4 bytes long the
 	// pointer must point beyond the header therefore 4 or less is bad.
 	if pointer <= header.IPv4OptionTimestampHdrLength {
-		return header.IPv4OptTSPointerOffset, errIPv4TimestampOptInvalidPointer
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptTSPointerOffset,
+			NeedICMP: true,
+		}
 	}
 	// To simplify processing below, base further work on the array of timestamps
 	// beyond the header, rather than on the whole option. Also to aid
@@ -1295,14 +1287,17 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres
 		//    timestamp, but the overflow count is incremented by one.
 		if flags == header.IPv4OptionTimestampWithPredefinedIPFlag {
 			// By definition we have nothing to do.
-			return 0, nil
+			return nil
 		}
 
 		if tsOpt.IncOverflow() != 0 {
-			return 0, nil
+			return nil
 		}
 		// The overflow count is also full.
-		return header.IPv4OptTSOFLWAndFLGOffset, errIPv4TimestampOptOverflow
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptTSOFLWAndFLGOffset,
+			NeedICMP: true,
+		}
 	}
 	if nextSlot+entrySize > dataLength {
 		// The data area isn't full but there isn't room for a new entry.
@@ -1321,32 +1316,36 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres
 			if dataLength%entrySize != 0 {
 				// The Data section size should be a multiple of the expected
 				// timestamp entry size.
-				return header.IPv4OptionLengthOffset, errIPv4TimestampOptInvalidLength
+				return &header.IPv4OptParameterProblem{
+					Pointer:  header.IPv4OptionLengthOffset,
+					NeedICMP: false,
+				}
 			}
 			// If the size is OK, the pointer must be corrupted.
 		}
-		return header.IPv4OptTSPointerOffset, errIPv4TimestampOptInvalidPointer
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptTSPointerOffset,
+			NeedICMP: true,
+		}
 	}
 
 	if usage.actions().timestamp == optionProcess {
 		tsOpt.UpdateTimestamp(localAddress, clock)
 	}
-	return 0, nil
+	return nil
 }
 
-var (
-	errIPv4RecordRouteOptInvalidLength  = errors.New("invalid length in Record Route")
-	errIPv4RecordRouteOptInvalidPointer = errors.New("invalid pointer in Record Route")
-)
-
 // handleRecordRoute checks and processes a Record route option. It is much
 // like the timestamp type 1 option, but without timestamps. The passed in
 // address is stored in the option in the correct spot if possible.
-func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Address, usage optionsUsage) (uint8, error) {
+func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Address, usage optionsUsage) *header.IPv4OptParameterProblem {
 	optlen := rrOpt.Size()
 
 	if optlen < header.IPv4AddressSize+header.IPv4OptionRecordRouteHdrLength {
-		return header.IPv4OptionLengthOffset, errIPv4RecordRouteOptInvalidLength
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptionLengthOffset,
+			NeedICMP: true,
+		}
 	}
 
 	pointer := rrOpt.Pointer()
@@ -1356,7 +1355,10 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad
 	// Since the pointer is 1 based, and the header is 3 bytes long the
 	// pointer must point beyond the header therefore 3 or less is bad.
 	if pointer <= header.IPv4OptionRecordRouteHdrLength {
-		return header.IPv4OptRRPointerOffset, errIPv4RecordRouteOptInvalidPointer
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptRRPointerOffset,
+			NeedICMP: true,
+		}
 	}
 
 	// RFC 791 page 21 says
@@ -1373,7 +1375,7 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad
 	// of these words is a copy/paste error from the timestamp option where
 	// there are two failure reasons given.
 	if pointer > optlen {
-		return 0, nil
+		return nil
 	}
 
 	// The data area isn't full but there isn't room for a new entry.
@@ -1398,17 +1400,23 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad
 			//    }
 			if (optlen-header.IPv4OptionRecordRouteHdrLength)%header.IPv4AddressSize != 0 {
 				// Length is bad, not on integral number of slots.
-				return header.IPv4OptionLengthOffset, errIPv4RecordRouteOptInvalidLength
+				return &header.IPv4OptParameterProblem{
+					Pointer:  header.IPv4OptionLengthOffset,
+					NeedICMP: true,
+				}
 			}
 			// If not length, the fault must be with the pointer.
 		}
-		return header.IPv4OptRRPointerOffset, errIPv4RecordRouteOptInvalidPointer
+		return &header.IPv4OptParameterProblem{
+			Pointer:  header.IPv4OptRRPointerOffset,
+			NeedICMP: true,
+		}
 	}
 	if usage.actions().recordRoute == optionVerify {
-		return 0, nil
+		return nil
 	}
 	rrOpt.StoreAddress(localAddress)
-	return 0, nil
+	return nil
 }
 
 // processIPOptions parses the IPv4 options and produces a new set of options
@@ -1419,7 +1427,7 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad
 // - The location of an error if there was one (or 0 if no error)
 // - If there is an error, information as to what it was was.
 // - The replacement option set.
-func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Options, usage optionsUsage) (uint8, header.IPv4Options, error) {
+func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Options, usage optionsUsage) (header.IPv4Options, *header.IPv4OptParameterProblem) {
 	stats := e.stats.ip
 	opts := header.IPv4Options(orig)
 	optIter := opts.MakeIterator()
@@ -1439,15 +1447,17 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt
 		h := header.IPv4(pkt.NetworkHeader().View())
 		dstAddr := h.DestinationAddress()
 		if pkt.NetworkPacketInfo.LocalAddressBroadcast || header.IsV4MulticastAddress(dstAddr) {
-			return 0 /* errCursor */, nil, header.ErrIPv4OptionAddress
+			return nil, &header.IPv4OptParameterProblem{
+				NeedICMP: false,
+			}
 		}
 		localAddress = dstAddr
 	}
 
 	for {
-		option, done, err := optIter.Next()
-		if done || err != nil {
-			return optIter.ErrCursor, optIter.Finalize(), err
+		option, done, optProblem := optIter.Next()
+		if done || optProblem != nil {
+			return optIter.Finalize(), optProblem
 		}
 		optType := option.Type()
 		if optType == header.IPv4OptionNOPType {
@@ -1456,12 +1466,15 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt
 		}
 		if optType == header.IPv4OptionListEndType {
 			optIter.PushNOPOrEnd(optType)
-			return 0 /* errCursor */, optIter.Finalize(), nil /* err */
+			return optIter.Finalize(), nil
 		}
 
 		// check for repeating options (multiple NOPs are OK)
 		if seenOptions[optType] {
-			return optIter.ErrCursor, nil, header.ErrIPv4OptDuplicate
+			return nil, &header.IPv4OptParameterProblem{
+				Pointer:  optIter.ErrCursor,
+				NeedICMP: true,
+			}
 		}
 		seenOptions[optType] = true
 
@@ -1473,9 +1486,9 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt
 				clock := e.protocol.stack.Clock()
 				newBuffer := optIter.RemainingBuffer()[:len(*option)]
 				_ = copy(newBuffer, option.Contents())
-				offset, err := handleTimestamp(header.IPv4OptionTimestamp(newBuffer), localAddress, clock, usage)
-				if err != nil {
-					return optIter.ErrCursor + offset, nil, err
+				if optProblem := handleTimestamp(header.IPv4OptionTimestamp(newBuffer), localAddress, clock, usage); optProblem != nil {
+					optProblem.Pointer += optIter.ErrCursor
+					return nil, optProblem
 				}
 				optIter.ConsumeBuffer(optLen)
 			}
@@ -1485,9 +1498,9 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt
 			if usage.actions().recordRoute != optionRemove {
 				newBuffer := optIter.RemainingBuffer()[:len(*option)]
 				_ = copy(newBuffer, option.Contents())
-				offset, err := handleRecordRoute(header.IPv4OptionRecordRoute(newBuffer), localAddress, usage)
-				if err != nil {
-					return optIter.ErrCursor + offset, nil, err
+				if optProblem := handleRecordRoute(header.IPv4OptionRecordRoute(newBuffer), localAddress, usage); optProblem != nil {
+					optProblem.Pointer += optIter.ErrCursor
+					return nil, optProblem
 				}
 				optIter.ConsumeBuffer(optLen)
 			}
diff --git a/pkg/tcpip/network/ipv4/ipv4_test.go b/pkg/tcpip/network/ipv4/ipv4_test.go
index a9e137c24..af6f22598 100644
--- a/pkg/tcpip/network/ipv4/ipv4_test.go
+++ b/pkg/tcpip/network/ipv4/ipv4_test.go
@@ -1,4 +1,4 @@
-// Copyright 2018 The gVisor Authors.
+// Copyright 2021 The gVisor Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -270,6 +270,11 @@ func TestIPv4Sanity(t *testing.T) {
 		nicID          = 1
 		randomSequence = 123
 		randomIdent    = 42
+		// In some cases Linux sets the error pointer to the start of the option
+		// (offset 0) instead of the actual wrong value, which is the length byte
+		// (offset 1). For compatibility we must do the same. Use this constant
+		// to indicate where this happens.
+		pointerOffsetForInvalidLength = 0
 	)
 	var (
 		ipv4Addr = tcpip.AddressWithPrefix{
@@ -439,6 +444,21 @@ func TestIPv4Sanity(t *testing.T) {
 			replyOptions: header.IPv4Options{},
 		},
 		{
+			name:              "bad option - no length",
+			maxTotalLength:    ipv4.MaxTotalSize,
+			transportProtocol: uint8(header.ICMPv4ProtocolNumber),
+			TTL:               ttl,
+			options: header.IPv4Options{
+				1, 1, 1, 68,
+				//        ^-start of timestamp.. but no length..
+			},
+			shouldFail:          true,
+			expectErrorICMP:     true,
+			ICMPType:            header.ICMPv4ParamProblem,
+			ICMPCode:            header.ICMPv4UnusedCode,
+			paramProblemPointer: header.IPv4MinimumSize + 3,
+		},
+		{
 			name:              "bad option - length 0",
 			maxTotalLength:    ipv4.MaxTotalSize,
 			transportProtocol: uint8(header.ICMPv4ProtocolNumber),
@@ -448,7 +468,27 @@ func TestIPv4Sanity(t *testing.T) {
 				//  ^
 				1, 2, 3, 4,
 			},
-			shouldFail: true,
+			shouldFail:          true,
+			expectErrorICMP:     true,
+			ICMPType:            header.ICMPv4ParamProblem,
+			ICMPCode:            header.ICMPv4UnusedCode,
+			paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength,
+		},
+		{
+			name:              "bad option - length 1",
+			maxTotalLength:    ipv4.MaxTotalSize,
+			transportProtocol: uint8(header.ICMPv4ProtocolNumber),
+			TTL:               ttl,
+			options: header.IPv4Options{
+				68, 1, 9, 0,
+				//  ^
+				1, 2, 3, 4,
+			},
+			shouldFail:          true,
+			expectErrorICMP:     true,
+			ICMPType:            header.ICMPv4ParamProblem,
+			ICMPCode:            header.ICMPv4UnusedCode,
+			paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength,
 		},
 		{
 			name:              "bad option - length big",
@@ -462,7 +502,11 @@ func TestIPv4Sanity(t *testing.T) {
 				// space is not possible. (Second byte)
 				1, 2, 3, 4,
 			},
-			shouldFail: true,
+			shouldFail:          true,
+			expectErrorICMP:     true,
+			ICMPType:            header.ICMPv4ParamProblem,
+			ICMPCode:            header.ICMPv4UnusedCode,
+			paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength,
 		},
 		{
 			// This tests for some linux compatible behaviour.
@@ -484,7 +528,7 @@ func TestIPv4Sanity(t *testing.T) {
 			expectErrorICMP:     true,
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
-			paramProblemPointer: header.IPv4MinimumSize + 2,
+			paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptTSPointerOffset,
 		},
 		{
 			name:              "multiple type 0 with room",
@@ -589,7 +633,7 @@ func TestIPv4Sanity(t *testing.T) {
 			expectErrorICMP:     true,
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
-			paramProblemPointer: header.IPv4MinimumSize + 2,
+			paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptTSPointerOffset,
 		},
 		{
 			name:              "valid timestamp pointer",
@@ -624,7 +668,7 @@ func TestIPv4Sanity(t *testing.T) {
 			expectErrorICMP:     true,
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
-			paramProblemPointer: header.IPv4MinimumSize + 2,
+			paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptTSPointerOffset,
 		},
 		// End of option list with illegal option after it, which should be ignored.
 		{
@@ -636,24 +680,31 @@ func TestIPv4Sanity(t *testing.T) {
 				68, 12, 13, 0x11,
 				192, 168, 1, 12,
 				1, 2, 3, 4,
-				0, 10, 3, 99,
+				0, 10, 3, 99, // EOL followed by junk
 			},
 			replyOptions: header.IPv4Options{
 				68, 12, 13, 0x21,
 				192, 168, 1, 12,
 				1, 2, 3, 4,
-				0, 0, 0, 0, // 3 bytes unknown option
-			}, //   ^  End of options hides following bytes.
+				0,       // End of Options hides following bytes.
+				0, 0, 0, // 3 bytes unknown option removed.
+			},
 		},
 		{
-			// Timestamp with a size too small.
+			// Timestamp with a size much too small.
 			name:              "timestamp truncated",
 			maxTotalLength:    ipv4.MaxTotalSize,
 			transportProtocol: uint8(header.ICMPv4ProtocolNumber),
 			TTL:               ttl,
-			options:           header.IPv4Options{68, 1, 0, 0},
-			//                            ^ Smallest possible is 8.
-			shouldFail: true,
+			options: header.IPv4Options{
+				68, 1, 0, 0,
+				//  ^ Smallest possible is 8. Linux points at the 68.
+			},
+			shouldFail:          true,
+			expectErrorICMP:     true,
+			ICMPType:            header.ICMPv4ParamProblem,
+			ICMPCode:            header.ICMPv4UnusedCode,
+			paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength,
 		},
 		{
 			name:              "single record route with room",
@@ -751,7 +802,7 @@ func TestIPv4Sanity(t *testing.T) {
 			expectErrorICMP:     true,
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
-			paramProblemPointer: header.IPv4MinimumSize + 2,
+			paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptRRPointerOffset,
 		},
 		{
 			// Pointer must be 4 or more as it must point past the 3 byte header
@@ -769,7 +820,7 @@ func TestIPv4Sanity(t *testing.T) {
 			expectErrorICMP:     true,
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
-			paramProblemPointer: header.IPv4MinimumSize + 2,
+			paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptRRPointerOffset,
 		},
 		{
 			// Pointer must be 4 or more as it must point past the 3 byte header
@@ -808,8 +859,7 @@ func TestIPv4Sanity(t *testing.T) {
 			expectErrorICMP:     true,
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
-			paramProblemPointer: header.IPv4MinimumSize + 2,
-			replyOptions:        header.IPv4Options{},
+			paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptRRPointerOffset,
 		},
 		{
 			name:              "duplicate record route",
@@ -828,7 +878,6 @@ func TestIPv4Sanity(t *testing.T) {
 			ICMPType:            header.ICMPv4ParamProblem,
 			ICMPCode:            header.ICMPv4UnusedCode,
 			paramProblemPointer: header.IPv4MinimumSize + 7,
-			replyOptions:        header.IPv4Options{},
 		},
 	}
 
@@ -884,7 +933,6 @@ func TestIPv4Sanity(t *testing.T) {
 			if test.maxTotalLength < totalLen {
 				totalLen = test.maxTotalLength
 			}
-
 			ip.Encode(&header.IPv4Fields{
 				TotalLength: totalLen,
 				Protocol:    test.transportProtocol,