Fix AS number comparison

Squashing AS number to an integer makes it possible for it to be
interpreted as a negative number -- which we then fail to squash
to AS_TRANS.

Fix this by forcing the comparison to happen on longs, which does
not suffer from this.

JIRA: BGPCEP-860
Change-Id: Iee10264aef22be70f34b2f122a063331bd0ec142
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java
index 059f82a6cd5a102f1fc4037bd8bee559bba9d4e1..7069bba2738de45bd9700f21372ea8970e17f1a8 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java
@@ -5,7 +5,6 @@
  * terms of the Eclipse Public License v1.0 which accompanies this distribution,
  * and is available at http://www.eclipse.org/legal/epl-v10.html
  */
-
 package org.opendaylight.protocol.bgp.rib.impl;
 
 import static java.util.Objects.requireNonNull;
@@ -48,7 +47,8 @@ abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter
     private static final int INITIAL_HOLDTIMER = 4;
 
     // <a href="http://tools.ietf.org/html/rfc6793">BGP Support for 4-Octet AS Number Space</a>
-    private static final int AS_TRANS = 23456;
+    @VisibleForTesting
+    static final int AS_TRANS = 23456;
     private static final Logger LOG = LoggerFactory.getLogger(AbstractBGPSessionNegotiator.class);
     private final BGPPeerRegistry registry;
     private final Promise<BGPSessionImpl> promise;
@@ -105,12 +105,7 @@ abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter
             }
 
             final BGPSessionPreferences preferences = this.registry.getPeerPreferences(remoteIp);
-
-            int as = preferences.getMyAs().getValue().intValue();
-            // Set as AS_TRANS if the value is bigger than 2B
-            if (as > Values.UNSIGNED_SHORT_MAX_VALUE) {
-                as = AS_TRANS;
-            }
+            final int as = openASNumber(preferences.getMyAs().getValue().longValue());
             sendMessage(new OpenBuilder().setMyAsNumber(as).setHoldTimer(preferences.getHoldTime()).setBgpIdentifier(
                     preferences.getBgpId()).setBgpParameters(preferences.getParams()).build());
             if (this.state != State.FINISHED) {
@@ -305,4 +300,10 @@ abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter
         LOG.info("Unexpected error during negotiation", cause);
         negotiationFailedCloseChannel(cause);
     }
+
+    @VisibleForTesting
+    static int openASNumber(final long configuredASNumber) {
+        // Return AS_TRANS if the value is bigger than 2B.
+        return configuredASNumber > Values.UNSIGNED_SHORT_MAX_VALUE ? AS_TRANS : (int) configuredASNumber;
+    }
 }

diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiatorTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiatorTest.java
new file mode 100644 (file)
index 0000000..693f331
--- /dev/null
+++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiatorTest.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2019 Pantheon Technologies, s.r.o. 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,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.protocol.bgp.rib.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.opendaylight.protocol.bgp.rib.impl.AbstractBGPSessionNegotiator.AS_TRANS;
+import static org.opendaylight.protocol.bgp.rib.impl.AbstractBGPSessionNegotiator.openASNumber;
+
+import org.junit.Test;
+
+public class AbstractBGPSessionNegotiatorTest {
+    @Test
+    public void testOpenASNumber() {
+        assertEquals(0, openASNumber(0));
+        assertEquals(65535, openASNumber(65535));
+        assertEquals(AS_TRANS, openASNumber(65536));
+        assertEquals(AS_TRANS, openASNumber(Integer.MAX_VALUE));
+        assertEquals(AS_TRANS, openASNumber(2147483648L));
+        assertEquals(AS_TRANS, openASNumber(4294967295L));
+    }
+}