From 6d8b00f5a86b543be5d9fa300970383c128250d3 Mon Sep 17 00:00:00 2001 From: Shigeru Yasuda Date: Thu, 11 Sep 2014 15:50:24 +0900 Subject: [PATCH] Bug 1805: Fixed 2 bugs in ICMP.computeChecksum(). * Fixed a bug that caused ArrayIndexOutOfBoundsException if the size of the payload was odd. * Fixed a bug that generated incorrect checksum if the 16-bit checksum overflowed more than 255 times. Change-Id: I8f26cc41dec2d5ba64c3ab31e66a846dfbea7bc5 Signed-off-by: Shigeru Yasuda --- .../controller/sal/packet/ICMP.java | 13 +++- .../controller/sal/packet/ICMPTest.java | 62 ++++++++++++++----- 2 files changed, 57 insertions(+), 18 deletions(-) diff --git a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/packet/ICMP.java b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/packet/ICMP.java index 35ae71d001..987394402d 100644 --- a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/packet/ICMP.java +++ b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/packet/ICMP.java @@ -1,6 +1,6 @@ /* - * Copyright (c) 2013 Cisco Systems, Inc. and others. All rights reserved. + * Copyright (c) 2013-2014 Cisco Systems, Inc. and others. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v1.0 which accompanies this distribution, @@ -190,8 +190,9 @@ public class ICMP extends Packet { end += rawPayload.length; } int checksumStartByte = start + getfieldOffset(CHECKSUM) / NetUtils.NumBitsInAByte; + int even = end & ~1; - for (int i = start; i <= (end - 1); i = i + 2) { + for (int i = start; i < even; i = i + 2) { // Skip, if the current bytes are checkSum bytes if (i == checksumStartByte) { continue; @@ -199,7 +200,13 @@ public class ICMP extends Packet { wordData = ((data[i] << 8) & 0xFF00) + (data[i + 1] & 0xFF); sum = sum + wordData; } - carry = (sum >> 16) & 0xFF; + if (even < end) { + // Add the last octet with zero padding. + wordData = (data[even] << 8) & 0xFF00; + sum = sum + wordData; + } + + carry = sum >>> 16; finalSum = (sum & 0xFFFF) + carry; return (short) ~((short) finalSum & 0xFFFF); } diff --git a/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/packet/ICMPTest.java b/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/packet/ICMPTest.java index e81fbf02cf..287b73ae3c 100644 --- a/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/packet/ICMPTest.java +++ b/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/packet/ICMPTest.java @@ -1,6 +1,6 @@ /* - * Copyright (c) 2013 Cisco Systems, Inc. and others. All rights reserved. + * Copyright (c) 2013-2014 Cisco Systems, Inc. and others. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v1.0 which accompanies this distribution, @@ -9,6 +9,8 @@ package org.opendaylight.controller.sal.packet; +import java.util.Arrays; + import junit.framework.Assert; import org.junit.Test; @@ -74,28 +76,58 @@ public class ICMPTest { (byte) 0x2b, (byte) 0x2c, (byte) 0x2d, (byte) 0x2e, (byte) 0x2f, (byte) 0x30, (byte) 0x31, (byte) 0x32, (byte) 0x33, (byte) 0x34, (byte) 0x35, (byte) 0x36, (byte) 0x37 }; + serializeTest(icmpRawPayload, (short)0xe553); + + serializeTest(null, (short)0xb108); + serializeTest(new byte[0], (short)0xb108); + + byte[] odd = { + (byte)0xba, (byte)0xd4, (byte)0xc7, (byte)0x53, + (byte)0xf8, (byte)0x59, (byte)0x68, (byte)0x77, + (byte)0xfd, (byte)0x27, (byte)0xe0, (byte)0x5b, + (byte)0xd0, (byte)0x2e, (byte)0x28, (byte)0x41, + (byte)0xa3, (byte)0x48, (byte)0x5d, (byte)0x2e, + (byte)0x7d, (byte)0x5b, (byte)0xd3, (byte)0x60, + (byte)0xb3, (byte)0x88, (byte)0x8d, (byte)0x0f, + (byte)0x1d, (byte)0x87, (byte)0x51, (byte)0x0f, + (byte)0x6a, (byte)0xff, (byte)0xf7, (byte)0xd4, + (byte)0x40, (byte)0x35, (byte)0x4e, (byte)0x01, + (byte)0x36, + }; + serializeTest(odd, (short)0xd0ad); + + // Large payload that causes 16-bit checksum overflow more than + // 255 times. + byte[] largeEven = new byte[1024]; + Arrays.fill(largeEven, (byte)0xff); + serializeTest(largeEven, (short)0xb108); + + byte[] largeOdd = new byte[1021]; + Arrays.fill(largeOdd, (byte)0xff); + serializeTest(largeOdd, (short)0xb207); + } - short checksum = (short)0xe553; - - // Create ICMP object + private void serializeTest(byte[] payload, short checksum) + throws PacketException { ICMP icmp = new ICMP(); - icmp.setType((byte)8); - icmp.setCode((byte)0); - icmp.setIdentifier((short) 0x46f5); - icmp.setSequenceNumber((short) 2); - icmp.setRawPayload(icmpRawPayload); - //icmp.setChecksum(checksum); + icmp.setType((byte)8).setCode((byte)0). + setIdentifier((short)0x46f5).setSequenceNumber((short)2); + int payloadSize = 0; + if (payload != null) { + icmp.setRawPayload(payload); + payloadSize = payload.length; + } // Serialize - byte[] stream = icmp.serialize(); - Assert.assertTrue(stream.length == 64); + byte[] data = icmp.serialize(); + Assert.assertEquals(payloadSize + 8, data.length); // Deserialize ICMP icmpDes = new ICMP(); - icmpDes.deserialize(stream, 0, stream.length); + icmpDes.deserialize(data, 0, data.length); Assert.assertFalse(icmpDes.isCorrupted()); - Assert.assertTrue(icmpDes.getChecksum() == checksum); - Assert.assertTrue(icmp.equals(icmpDes)); + Assert.assertEquals(checksum, icmpDes.getChecksum()); + Assert.assertEquals(icmp, icmpDes); } } -- 2.36.6