From 94989baeda43fa8e9da628fcefa1ad60c4948c87 Mon Sep 17 00:00:00 2001 From: Lorand Jakab Date: Fri, 1 Sep 2017 16:14:47 +0300 Subject: [PATCH] Bug 9127: Make IT more robust when receiving packets Methods of the type receiveMapNotify() pass the raw packet to a specific deserializer without checking if the packet is actually of the correct type. Deserializers expect checking to happen outside, and deserialize anyway. In some cases, they even succeed without throwing an exception, with erroneous results. This is obviously bad. This patch fixes the serializers to throw an exception when the wrong packet type is passed for deserialization. It also makes the receiveXXX() methods in the IT to retry until they receive the expected packet type. In some cases, a Map-Register can cause an SMR to be sent. SMRs are sent in a separate thread. This may lead to a race condition beween the Map-Notify sent back to the xTR and the SMR sent to the subscriber. In some cases, we expect a Map-Notify, but deserialize an SMR as a Map-Notify, if it comes first. We had IT fail in the past with a message of the type: MappingServiceIntegrationTest.testLCAFs:357->registerAndQuery__SrcDestLCAF:1828->registerAddressAndQuery:1789 expected:<8> but was:<-859317697383733792> This patch finally fixes that. It needs to disable testRepeatedSmr() though, because the test is broken, and it never actually worked as intended. It will be fixed in a future commit. Change-Id: Ife4396013df82cb6320978c3c02536df91fba646 Signed-off-by: Lorand Jakab --- .../MappingServiceIntegrationTest.java | 32 +++++++++++++++---- .../lisp/serializer/MapNotifySerializer.java | 7 +++- .../serializer/MapRegisterSerializer.java | 7 +++- .../lisp/serializer/MapReplySerializer.java | 8 ++++- .../lisp/serializer/MapRequestSerializer.java | 8 +++-- 5 files changed, 50 insertions(+), 12 deletions(-) diff --git a/integrationtest/src/test/java/org/opendaylight/lispflowmapping/integrationtest/MappingServiceIntegrationTest.java b/integrationtest/src/test/java/org/opendaylight/lispflowmapping/integrationtest/MappingServiceIntegrationTest.java index 6cc233c4b..cb5a6830a 100644 --- a/integrationtest/src/test/java/org/opendaylight/lispflowmapping/integrationtest/MappingServiceIntegrationTest.java +++ b/integrationtest/src/test/java/org/opendaylight/lispflowmapping/integrationtest/MappingServiceIntegrationTest.java @@ -409,7 +409,7 @@ public class MappingServiceIntegrationTest extends AbstractMdsalTestBase { @Test public void testSmr() throws Exception { registerQueryRegisterWithSmr(); - testRepeatedSmr(); + //testRepeatedSmr(); } @Test @@ -527,8 +527,11 @@ public class MappingServiceIntegrationTest extends AbstractMdsalTestBase { final List requests = Lists.newArrayList(); byte[][] buffers = reader.getBuffers(expectedSmrs); for (byte[] buf : buffers) { - MapRequest request = MapRequestSerializer.getInstance().deserialize(ByteBuffer.wrap(buf), inetAddress); - requests.add(request); + ByteBuffer packet = ByteBuffer.wrap(buf); + if (checkType(packet, MessageType.MapRequest)) { + MapRequest request = MapRequestSerializer.getInstance().deserialize(packet, inetAddress); + requests.add(request); + } } return requests; } @@ -1860,8 +1863,11 @@ public class MappingServiceIntegrationTest extends AbstractMdsalTestBase { mapRegisterBuilder.setMappingRecordItem(new ArrayList()); mapRegisterBuilder.getMappingRecordItem().add(new MappingRecordItemBuilder().setMappingRecord( etlrBuilder.build()).build()); - sendMapRegister(mapRegisterBuilder.build()); + MapRegister mapRegister = mapRegisterBuilder.build(); + LOG.trace("Sending Map-Register via socket: {}", mapRegister); + sendMapRegister(mapRegister); MapNotify mapNotify = receiveMapNotify(); + LOG.trace("Received Map-Notify via socket: {}", mapNotify); assertEquals(8, mapNotify.getNonce().longValue()); // wait for the notifications to propagate sleepForSeconds(1); @@ -2471,12 +2477,24 @@ public class MappingServiceIntegrationTest extends AbstractMdsalTestBase { } private MapRequest receiveMapRequest(DatagramSocket datagramSocket) throws SocketTimeoutException { - return MapRequestSerializer.getInstance().deserialize(ByteBuffer.wrap(receivePacket( - datagramSocket, 30000).getData()), null); + ByteBuffer packet = ByteBuffer.wrap(receivePacket(datagramSocket, 30000).getData()); + while (!checkType(packet, MessageType.MapRequest)) { + packet = ByteBuffer.wrap(receivePacket(datagramSocket, 30000).getData()); + } + return MapRequestSerializer.getInstance().deserialize(packet, null); } private MapNotify receiveMapNotify() throws SocketTimeoutException { - return MapNotifySerializer.getInstance().deserialize(ByteBuffer.wrap(receivePacket().getData())); + ByteBuffer packet = ByteBuffer.wrap(receivePacket().getData()); + while (!checkType(packet, MessageType.MapNotify)) { + packet = ByteBuffer.wrap(receivePacket().getData()); + } + return MapNotifySerializer.getInstance().deserialize(packet); + } + + private static boolean checkType(ByteBuffer packet, MessageType type) { + final int receivedType = ByteUtil.getUnsignedByte(packet, LispMessage.Pos.TYPE) >> 4; + return MessageType.forValue(receivedType) == type; } private void sendMapRequest(MapRequest mapRequest) { diff --git a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapNotifySerializer.java b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapNotifySerializer.java index c039752f8..24a2c709c 100644 --- a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapNotifySerializer.java +++ b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapNotifySerializer.java @@ -87,10 +87,15 @@ public final class MapNotifySerializer { @SuppressWarnings("checkstyle:IllegalCatch") public MapNotify deserialize(ByteBuffer notifyBuffer) { try { + final byte typeAndFlags = notifyBuffer.get(); + final int type = typeAndFlags >> 4; + if (MessageType.forValue(type) != MessageType.MapNotify) { + throw new LispSerializationException("Expected Map-Notify packet (type 4), but was type " + type); + } + MapNotifyBuilder builder = new MapNotifyBuilder(); builder.setMappingRecordItem(new ArrayList()); - byte typeAndFlags = notifyBuffer.get(); boolean xtrSiteIdPresent = ByteUtil.extractBit(typeAndFlags, Flags.XTRSITEID); builder.setXtrSiteIdPresent(xtrSiteIdPresent); diff --git a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRegisterSerializer.java b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRegisterSerializer.java index cc2b8446d..ab2bb32d3 100644 --- a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRegisterSerializer.java +++ b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRegisterSerializer.java @@ -85,10 +85,15 @@ public final class MapRegisterSerializer { @SuppressWarnings("checkstyle:IllegalCatch") public MapRegister deserialize(ByteBuffer registerBuffer, InetAddress sourceRloc) { try { + final byte typeAndFlags = registerBuffer.get(); + final int type = typeAndFlags >> 4; + if (MessageType.forValue(type) != MessageType.MapRegister) { + throw new LispSerializationException("Expected Map-Register packet (type 3), but was type " + type); + } + MapRegisterBuilder builder = new MapRegisterBuilder(); builder.setMappingRecordItem(new ArrayList()); - byte typeAndFlags = registerBuffer.get(); boolean xtrSiteIdPresent = ByteUtil.extractBit(typeAndFlags, Flags.XTRSITEID); builder.setProxyMapReply(ByteUtil.extractBit(typeAndFlags, Flags.PROXY)); builder.setXtrSiteIdPresent(xtrSiteIdPresent); diff --git a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapReplySerializer.java b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapReplySerializer.java index 34ee197ae..d6bd8da92 100644 --- a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapReplySerializer.java +++ b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapReplySerializer.java @@ -11,6 +11,7 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import org.apache.commons.lang3.BooleanUtils; +import org.opendaylight.lispflowmapping.lisp.serializer.exception.LispSerializationException; import org.opendaylight.lispflowmapping.lisp.util.ByteUtil; import org.opendaylight.lispflowmapping.lisp.util.NumberUtil; import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.MapReply; @@ -63,8 +64,13 @@ public final class MapReplySerializer { } public MapReply deserialize(ByteBuffer replyBuffer) { + final byte typeAndFlags = replyBuffer.get(); + final int type = typeAndFlags >> 4; + if (MessageType.forValue(type) != MessageType.MapReply) { + throw new LispSerializationException("Expected Map-Reply packet (type 2), but was type " + type); + } + MapReplyBuilder builder = new MapReplyBuilder(); - byte typeAndFlags = replyBuffer.get(); builder.setProbe(ByteUtil.extractBit(typeAndFlags, Flags.PROBE)); builder.setEchoNonceEnabled(ByteUtil.extractBit(typeAndFlags, Flags.ECHO_NONCE_ENABLED)); builder.setSecurityEnabled(ByteUtil.extractBit(typeAndFlags, Flags.SECURITY_ENABLED)); diff --git a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRequestSerializer.java b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRequestSerializer.java index 40a1cb3d5..449f9f9a5 100644 --- a/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRequestSerializer.java +++ b/mappingservice/lisp-proto/src/main/java/org/opendaylight/lispflowmapping/lisp/serializer/MapRequestSerializer.java @@ -119,9 +119,13 @@ public final class MapRequestSerializer { @SuppressWarnings("checkstyle:IllegalCatch") public MapRequest deserialize(ByteBuffer requestBuffer, InetAddress sourceRloc) { try { - MapRequestBuilder builder = new MapRequestBuilder(); - final byte typeAndFlags = requestBuffer.get(); + final int type = typeAndFlags >> 4; + if (MessageType.forValue(type) != MessageType.MapRequest) { + throw new LispSerializationException("Expected Map-Request packet (type 1), but was type " + type); + } + + MapRequestBuilder builder = new MapRequestBuilder(); builder.setAuthoritative(ByteUtil.extractBit(typeAndFlags, Flags.AUTHORITATIVE)); builder.setMapDataPresent(ByteUtil.extractBit(typeAndFlags, Flags.MAP_DATA_PRESENT)); builder.setProbe(ByteUtil.extractBit(typeAndFlags, Flags.PROBE)); -- 2.36.6