Petals BC SOAP

Algorithmic error in function isLocalAddress() from NetworkUtil

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 4.0.4
  • Fix Version/s: 4.0.5, 4.1
  • Component/s: None
  • Security Level: Public
  • Description:
    Hide

    In the current code, the value of 'result' can be overwritten by subsequent non-local addresses. Some 'true' result can be lost.

    I suggest the following patch:

    Index: src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java
    ===================================================================
    --- src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(revision 16340)
    +++ src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(working copy)
    @@ -129,22 +129,23 @@
          * @return
          */
         public static boolean isLocalAddress(InetAddress address) {
    -        boolean result = false;
             if (address != null) {
                 try {
                     Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
                     while (itfs.hasMoreElements()) {
                         NetworkInterface itf = itfs.nextElement();
                         Enumeration<InetAddress> iaenum = itf.getInetAddresses();
    -                    while (iaenum.hasMoreElements() && !result) {
    +                    while (iaenum.hasMoreElements()) {
                             InetAddress ia = iaenum.nextElement();
    -                        result = ia.equals(address);
    +                        if (ia.equals(address)) {
    +							return true;
    +						}
                         }
                     }
                 } catch (SocketException e) {
                 }
             }
    -        return result;
    +        return false;
         }
     
         /**

    By the way, the management of SocketException is still problematic, because it can occur before every network interface has been scanned and make isLocalAddress() return false while it should have returned true.

    Show
    In the current code, the value of 'result' can be overwritten by subsequent non-local addresses. Some 'true' result can be lost. I suggest the following patch:
    Index: src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java
    ===================================================================
    --- src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(revision 16340)
    +++ src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(working copy)
    @@ -129,22 +129,23 @@
          * @return
          */
         public static boolean isLocalAddress(InetAddress address) {
    -        boolean result = false;
             if (address != null) {
                 try {
                     Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
                     while (itfs.hasMoreElements()) {
                         NetworkInterface itf = itfs.nextElement();
                         Enumeration<InetAddress> iaenum = itf.getInetAddresses();
    -                    while (iaenum.hasMoreElements() && !result) {
    +                    while (iaenum.hasMoreElements()) {
                             InetAddress ia = iaenum.nextElement();
    -                        result = ia.equals(address);
    +                        if (ia.equals(address)) {
    +							return true;
    +						}
                         }
                     }
                 } catch (SocketException e) {
                 }
             }
    -        return result;
    +        return false;
         }
     
         /**
    By the way, the management of SocketException is still problematic, because it can occur before every network interface has been scanned and make isLocalAddress() return false while it should have returned true.
  • Environment:
    All

Activity

Hide
Sébastien André added a comment - Thu, 25 Nov 2010 - 16:52:16 +0100

Sorry for the + and - from the diff output that have been transformed into bullets

Show
Sébastien André added a comment - Thu, 25 Nov 2010 - 16:52:16 +0100 Sorry for the + and - from the diff output that have been transformed into bullets
Christophe DENEUX made changes - Fri, 26 Nov 2010 - 08:25:59 +0100
Field Original Value New Value
Priority Blocker [ 1 ]
Description In the current code, the value of 'result' can be overwritten by subsequent non-local addresses. Some 'true' result can be lost.

I suggest the following patch:

Index: src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java
===================================================================
--- src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java (revision 16340)
+++ src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java (working copy)
@@ -129,22 +129,23 @@
      * @return
      */
     public static boolean isLocalAddress(InetAddress address) {
- boolean result = false;
         if (address != null) {
             try {
                 Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
                 while (itfs.hasMoreElements()) {
                     NetworkInterface itf = itfs.nextElement();
                     Enumeration<InetAddress> iaenum = itf.getInetAddresses();
- while (iaenum.hasMoreElements() && !result) {
+ while (iaenum.hasMoreElements()) {
                         InetAddress ia = iaenum.nextElement();
- result = ia.equals(address);
+ if (ia.equals(address)) {
+ return true;
+ }
                     }
                 }
             } catch (SocketException e) {
             }
         }
- return result;
+ return false;
     }
 
     /**



By the way, the management of SocketException is still problematic, because it can occur before every network interface has been scanned and make isLocalAddress() return false while it should have returned true.

In the current code, the value of 'result' can be overwritten by subsequent non-local addresses. Some 'true' result can be lost.

I suggest the following patch:
{code}
Index: src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java
===================================================================
--- src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java (revision 16340)
+++ src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java (working copy)
@@ -129,22 +129,23 @@
      * @return
      */
     public static boolean isLocalAddress(InetAddress address) {
- boolean result = false;
         if (address != null) {
             try {
                 Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
                 while (itfs.hasMoreElements()) {
                     NetworkInterface itf = itfs.nextElement();
                     Enumeration<InetAddress> iaenum = itf.getInetAddresses();
- while (iaenum.hasMoreElements() && !result) {
+ while (iaenum.hasMoreElements()) {
                         InetAddress ia = iaenum.nextElement();
- result = ia.equals(address);
+ if (ia.equals(address)) {
+ return true;
+ }
                     }
                 }
             } catch (SocketException e) {
             }
         }
- return result;
+ return false;
     }
 
     /**
{code}


By the way, the management of SocketException is still problematic, because it can occur before every network interface has been scanned and make isLocalAddress() return false while it should have returned true.

Hide
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 08:27:15 +0100 - edited

To avoid formating problem as you have had, you can use the tag '{code}'. Update done in the issue description.

Show
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 08:27:15 +0100 - edited To avoid formating problem as you have had, you can use the tag '{code}'. Update done in the issue description.
Hide
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 08:43:08 +0100 - edited

I propose you the following patch to manage also the SocketException:

Index: src/main/java/org/ow2/petals/binding/soap/listener/incoming/SoapExternalListenerManager.java
===================================================================
--- src/main/java/org/ow2/petals/binding/soap/listener/incoming/SoapExternalListenerManager.java	(revision 16332)
+++ src/main/java/org/ow2/petals/binding/soap/listener/incoming/SoapExternalListenerManager.java	(working copy)
@@ -260,14 +261,21 @@
             }

             // address exists; check if it is a local one
-            if (NetworkUtil.isLocalAddress(address)) {
-                // yes : restrict access
-                config.addAddress(address);
-                config.setRestrict(true);
+            if (address != null) {
+                if (NetworkUtil.isLocalAddress(address)) {
+                    // yes : restrict access
+                    config.addAddress(address);
+                    config.setRestrict(true);
+                } else {
+                    logger.warning(address + " is not a valid local address, using the wildcard one");
+                    // no : set to localhost and do not restrict (FIXME : To be
+                    // defined)
+                    config.setRestrict(false);
+                    config.addAddresses(NetworkUtil.getAllIPv4InetAddresses());
+                }
             } else {
-                logger.warning(address + " is not a valid local address, using the wildcard one");
-                // no : set to localhost and do not restrict (FIXME : To be
-                // defined)
+                logger.warning("Unable to get the IP adress associated to the host '" + host + "'.");
+                logger.warning("All available local adresses are used to listen incoming requests.");
                 config.setRestrict(false);
                 config.addAddresses(NetworkUtil.getAllIPv4InetAddresses());
             }

Index: src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java
===================================================================
--- src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(revision 16332)
+++ src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(working copy)
@@ -122,29 +122,27 @@
     }
 
     /**
-     * Returns true if the given address is not null and is one of the local
-     * host address including the loopback address
+     * Returns true if the given address is not null and is one of the local host address including the loopback address
      * 
      * @param address
+     *            The {@link InetAddress} to check as local adress. Must be non null.
      * @return
+     * @throws SocketException
+     *             An error occurs getting local network interfaces.
      */
-    public static boolean isLocalAddress(InetAddress address) {
-        boolean result = false;
-        if (address != null) {
-            try {
-                Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
-                while (itfs.hasMoreElements()) {
-                    NetworkInterface itf = itfs.nextElement();
-                    Enumeration<InetAddress> iaenum = itf.getInetAddresses();
-                    while (iaenum.hasMoreElements() && !result) {
-                        InetAddress ia = iaenum.nextElement();
-                        result = ia.equals(address);
-                    }
+    public static boolean isLocalAddress(InetAddress address) throws SocketException {
+        final Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
+        while (itfs.hasMoreElements()) {
+            final NetworkInterface itf = itfs.nextElement();
+            final Enumeration<InetAddress> iaenum = itf.getInetAddresses();
+            while (iaenum.hasMoreElements()) {
+                final InetAddress ia = iaenum.nextElement();
+                if (ia.equals(address)) {
+                    return true;
                 }
-            } catch (SocketException e) {
             }
         }
-        return result;
+        return false;
     }
 
     /**
Show
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 08:43:08 +0100 - edited I propose you the following patch to manage also the SocketException:
Index: src/main/java/org/ow2/petals/binding/soap/listener/incoming/SoapExternalListenerManager.java
===================================================================
--- src/main/java/org/ow2/petals/binding/soap/listener/incoming/SoapExternalListenerManager.java	(revision 16332)
+++ src/main/java/org/ow2/petals/binding/soap/listener/incoming/SoapExternalListenerManager.java	(working copy)
@@ -260,14 +261,21 @@
             }

             // address exists; check if it is a local one
-            if (NetworkUtil.isLocalAddress(address)) {
-                // yes : restrict access
-                config.addAddress(address);
-                config.setRestrict(true);
+            if (address != null) {
+                if (NetworkUtil.isLocalAddress(address)) {
+                    // yes : restrict access
+                    config.addAddress(address);
+                    config.setRestrict(true);
+                } else {
+                    logger.warning(address + " is not a valid local address, using the wildcard one");
+                    // no : set to localhost and do not restrict (FIXME : To be
+                    // defined)
+                    config.setRestrict(false);
+                    config.addAddresses(NetworkUtil.getAllIPv4InetAddresses());
+                }
             } else {
-                logger.warning(address + " is not a valid local address, using the wildcard one");
-                // no : set to localhost and do not restrict (FIXME : To be
-                // defined)
+                logger.warning("Unable to get the IP adress associated to the host '" + host + "'.");
+                logger.warning("All available local adresses are used to listen incoming requests.");
                 config.setRestrict(false);
                 config.addAddresses(NetworkUtil.getAllIPv4InetAddresses());
             }

Index: src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java
===================================================================
--- src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(revision 16332)
+++ src/main/java/org/ow2/petals/binding/soap/util/NetworkUtil.java	(working copy)
@@ -122,29 +122,27 @@
     }
 
     /**
-     * Returns true if the given address is not null and is one of the local
-     * host address including the loopback address
+     * Returns true if the given address is not null and is one of the local host address including the loopback address
      * 
      * @param address
+     *            The {@link InetAddress} to check as local adress. Must be non null.
      * @return
+     * @throws SocketException
+     *             An error occurs getting local network interfaces.
      */
-    public static boolean isLocalAddress(InetAddress address) {
-        boolean result = false;
-        if (address != null) {
-            try {
-                Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
-                while (itfs.hasMoreElements()) {
-                    NetworkInterface itf = itfs.nextElement();
-                    Enumeration<InetAddress> iaenum = itf.getInetAddresses();
-                    while (iaenum.hasMoreElements() && !result) {
-                        InetAddress ia = iaenum.nextElement();
-                        result = ia.equals(address);
-                    }
+    public static boolean isLocalAddress(InetAddress address) throws SocketException {
+        final Enumeration<NetworkInterface> itfs = NetworkInterface.getNetworkInterfaces();
+        while (itfs.hasMoreElements()) {
+            final NetworkInterface itf = itfs.nextElement();
+            final Enumeration<InetAddress> iaenum = itf.getInetAddresses();
+            while (iaenum.hasMoreElements()) {
+                final InetAddress ia = iaenum.nextElement();
+                if (ia.equals(address)) {
+                    return true;
                 }
-            } catch (SocketException e) {
             }
         }
-        return result;
+        return false;
     }
 
     /**
Hide
noddoux added a comment - Fri, 26 Nov 2010 - 09:46:25 +0100

I will correct this bug by implement the following behavior:

  • if there is no host in the jbi.xml, all the local interfaces are listened
  • if there is a host in the jbi.xml, it checks that it is a local host/address:
    > if the host/address is valid, it is used to listen to incoming requests
    > if the host/address is not valid, an error is raised (it is not a good behavior to change the parameter set by the user)

The fix version will be the version 4.1.

Show
noddoux added a comment - Fri, 26 Nov 2010 - 09:46:25 +0100 I will correct this bug by implement the following behavior:
  • if there is no host in the jbi.xml, all the local interfaces are listened
  • if there is a host in the jbi.xml, it checks that it is a local host/address: > if the host/address is valid, it is used to listen to incoming requests > if the host/address is not valid, an error is raised (it is not a good behavior to change the parameter set by the user)
The fix version will be the version 4.1.
Hide
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 09:57:51 +0100

If you raise an error:

  • the component must correctly start to avoid to have it in a undefined state
  • the HTTP server is not started
  • provide a MBean to fix the listen adresse and start the HTTP server without to have to restart the BC SOAP (see PETALSBCSOAP-63 about starting jetty using JMX).

Ok to implement this in 4.1, but you must fix the algorithm in 4.0.x to unblock our clients

Show
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 09:57:51 +0100 If you raise an error:
  • the component must correctly start to avoid to have it in a undefined state
  • the HTTP server is not started
  • provide a MBean to fix the listen adresse and start the HTTP server without to have to restart the BC SOAP (see PETALSBCSOAP-63 about starting jetty using JMX).
Ok to implement this in 4.1, but you must fix the algorithm in 4.0.x to unblock our clients
Hide
noddoux added a comment - Fri, 26 Nov 2010 - 10:57:31 +0100 - edited

If an error is raised:

  • the component will be not started because the check is done at the initialization step of the component life cycle (it will not be copied in the repository)
  • consequently the HTTP server is not started
  • is it a client requirement ? If not, I do not think it really necessary and urgent.
Show
noddoux added a comment - Fri, 26 Nov 2010 - 10:57:31 +0100 - edited If an error is raised:
  • the component will be not started because the check is done at the initialization step of the component life cycle (it will not be copied in the repository)
  • consequently the HTTP server is not started
  • is it a client requirement ? If not, I do not think it really necessary and urgent.
Hide
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 14:52:10 +0100

The existing algorithmic must be fixed. It's urgent.

The new algorithm is not urgent. If the init/start of the component is stopped because of misconfiguration, you forget all SA deployment link to this component. In an operating point of view, it's better to be able to continue the deployment and after to fix the configuration. My previous proposition permits to fix the misconfiguration after to have deploy all SAs.

Show
Christophe DENEUX added a comment - Fri, 26 Nov 2010 - 14:52:10 +0100 The existing algorithmic must be fixed. It's urgent. The new algorithm is not urgent. If the init/start of the component is stopped because of misconfiguration, you forget all SA deployment link to this component. In an operating point of view, it's better to be able to continue the deployment and after to fix the configuration. My previous proposition permits to fix the misconfiguration after to have deploy all SAs.
Charles Casadei made changes - Wed, 1 Dec 2010 - 10:39:20 +0100
Link This issue blocks SPVEOLIAE-92 [ SPVEOLIAE-92 ]
Christophe DENEUX made changes - Fri, 30 Sep 2011 - 09:19:30 +0200
Fix Version/s 4.1_4.0 [ 10269 ]
Fix Version/s 4.1 [ 10186 ]
Affects Version/s 4.1 [ 10186 ]
Sébastien Rebiere made changes - Thu, 16 Feb 2012 - 17:06:34 +0100
Fix Version/s 4.1 [ 10186 ]
Fix Version/s 4.1_4.0 [ 10269 ]
Transition Status Change Time Execution Times Last Executer Last Execution Date
New New Open Open
16h 57m
1
noddoux
Fri, 26 Nov 2010 - 09:46:46 +0100
Open Open In Progress In Progress
7s
1
noddoux
Fri, 26 Nov 2010 - 09:46:53 +0100
In Progress In Progress Resolved Resolved
3d 2h 32m
1
noddoux
Mon, 29 Nov 2010 - 12:18:58 +0100



People

Dates

  • Created:
    Thu, 25 Nov 2010 - 16:49:04 +0100
    Updated:
    Thu, 16 Feb 2012 - 17:06:34 +0100
    Resolved:
    Mon, 29 Nov 2010 - 12:18:58 +0100