changeset 45060:adbeae0f677e

6515161: If remote removeNotificationListener gets SecurityException, client no longer gets notifications Summary: there won't be any inconsistent state if remote removeNotificationListener gets any exception Reviewed-by: dfuchs, hb Contributed-by: ujwal.vangapally@oracle.com
author uvangapally
date Wed, 10 May 2017 12:00:08 +0530
parents 30e75693ae99
children 74b09ee3cd55
files jdk/src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnector.java jdk/src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java jdk/test/javax/management/remote/mandatory/notif/NoPermToRemoveTest.java
diffstat 3 files changed, 248 insertions(+), 40 deletions(-) [+]
line wrap: on
line diff
--- a/jdk/src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnector.java	Wed Jul 05 23:23:09 2017 +0200
+++ b/jdk/src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnector.java	Wed May 10 12:00:08 2017 +0530
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1258,7 +1258,7 @@
                     + ", listener=" + listener);
 
             final Integer[] ret =
-                    rmiNotifClient.removeNotificationListener(name, listener);
+                    rmiNotifClient.getListenerIds(name, listener);
 
             if (debug) logger.debug("removeNotificationListener",
                     "listenerIDs=" + objects(ret));
@@ -1278,7 +1278,7 @@
             } finally {
                 popDefaultClassLoader(old);
             }
-
+            rmiNotifClient.removeNotificationListener(name, listener);
         }
 
         public void removeNotificationListener(ObjectName name,
@@ -1300,7 +1300,7 @@
                         + ", handback=" + handback);
 
             final Integer ret =
-                    rmiNotifClient.removeNotificationListener(name, listener,
+                    rmiNotifClient.getListenerId(name, listener,
                     filter, handback);
 
             if (debug) logger.debug("removeNotificationListener",
@@ -1320,7 +1320,8 @@
             } finally {
                 popDefaultClassLoader(old);
             }
-
+            rmiNotifClient.removeNotificationListener(name, listener,
+                    filter, handback);
         }
     }
 
--- a/jdk/src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java	Wed Jul 05 23:23:09 2017 +0200
+++ b/jdk/src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java	Wed May 10 12:00:08 2017 +0530
@@ -188,6 +188,53 @@
     }
 
     public synchronized Integer[]
+    getListenerIds(ObjectName name,
+                   NotificationListener listener)
+            throws ListenerNotFoundException, IOException {
+
+        List<Integer> ids = new ArrayList<Integer>();
+        List<ClientListenerInfo> values =
+                new ArrayList<ClientListenerInfo>(infoList.values());
+        for (int i=values.size()-1; i>=0; i--) {
+            ClientListenerInfo li = values.get(i);
+
+            if (li.sameAs(name, listener)) {
+                ids.add(li.getListenerID());
+            }
+        }
+
+        if (ids.isEmpty())
+            throw new ListenerNotFoundException("Listener not found");
+
+        return ids.toArray(new Integer[0]);
+    }
+
+    public synchronized Integer
+    getListenerId(ObjectName name,
+                   NotificationListener listener,
+                   NotificationFilter filter,
+                   Object handback)
+            throws ListenerNotFoundException, IOException {
+
+        Integer id = null;
+
+        List<ClientListenerInfo> values =
+                new ArrayList<ClientListenerInfo>(infoList.values());
+        for (int i=values.size()-1; i>=0; i--) {
+            ClientListenerInfo li = values.get(i);
+            if (li.sameAs(name, listener, filter, handback)) {
+                id=li.getListenerID();
+                break;
+            }
+        }
+
+        if (id == null)
+            throw new ListenerNotFoundException("Listener not found");
+
+        return id;
+    }
+
+    public synchronized Integer[]
         removeNotificationListener(ObjectName name,
                                    NotificationListener listener)
         throws ListenerNotFoundException, IOException {
@@ -198,24 +245,12 @@
             logger.trace("removeNotificationListener",
                          "Remove the listener "+listener+" from "+name);
         }
-
-        List<Integer> ids = new ArrayList<Integer>();
-        List<ClientListenerInfo> values =
-                new ArrayList<ClientListenerInfo>(infoList.values());
-        for (int i=values.size()-1; i>=0; i--) {
-            ClientListenerInfo li = values.get(i);
-
-            if (li.sameAs(name, listener)) {
-                ids.add(li.getListenerID());
-
-                infoList.remove(li.getListenerID());
-            }
+        Integer[] liIds = getListenerIds(name, listener);
+        for (int i = 0; i < liIds.length; i++) {
+            infoList.remove(liIds[i]);
         }
 
-        if (ids.isEmpty())
-            throw new ListenerNotFoundException("Listener not found");
-
-        return ids.toArray(new Integer[0]);
+        return liIds;
     }
 
     public synchronized Integer
@@ -231,26 +266,11 @@
         }
 
         beforeRemove();
+        Integer liId = getListenerId(name, listener,
+                filter, handback);
+        infoList.remove(liId);
 
-        Integer id = null;
-
-        List<ClientListenerInfo> values =
-                new ArrayList<ClientListenerInfo>(infoList.values());
-        for (int i=values.size()-1; i>=0; i--) {
-            ClientListenerInfo li = values.get(i);
-            if (li.sameAs(name, listener, filter, handback)) {
-                id=li.getListenerID();
-
-                infoList.remove(id);
-
-                break;
-            }
-        }
-
-        if (id == null)
-            throw new ListenerNotFoundException("Listener not found");
-
-        return id;
+        return liId;
     }
 
     public synchronized Integer[] removeNotificationListener(ObjectName name) {
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/javax/management/remote/mandatory/notif/NoPermToRemoveTest.java	Wed May 10 12:00:08 2017 +0530
@@ -0,0 +1,187 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+ /*
+ * @test
+ * @bug 6515161
+ * @summary checks the behaviour of  mbeanServerConnection.removeNotificationListener
+ * operation when there is a exception thrown during removal
+ * @modules java.management
+ * @run main NoPermToRemoveTest
+ */
+
+import java.lang.management.ManagementFactory;
+import java.security.AllPermission;
+import java.security.CodeSource;
+import java.security.Permission;
+import java.security.PermissionCollection;
+import java.security.Permissions;
+import java.security.Policy;
+import java.security.ProtectionDomain;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.management.ListenerNotFoundException;
+import javax.management.MBeanPermission;
+import javax.management.MBeanServer;
+import javax.management.MBeanServerConnection;
+import javax.management.Notification;
+import javax.management.NotificationBroadcasterSupport;
+import javax.management.NotificationFilter;
+import javax.management.NotificationListener;
+import javax.management.ObjectName;
+import javax.management.remote.JMXConnector;
+import javax.management.remote.JMXConnectorFactory;
+import javax.management.remote.JMXConnectorServer;
+import javax.management.remote.JMXConnectorServerFactory;
+import javax.management.remote.JMXServiceURL;
+
+public class NoPermToRemoveTest {
+    public static void main(String[] args) throws Exception {
+        Policy.setPolicy(new NoRemovePolicy());
+        System.setSecurityManager(new SecurityManager());
+
+        JMXServiceURL url = new JMXServiceURL("service:jmx:rmi:///");
+        MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
+        ObjectName name = new ObjectName("foo:type=Sender");
+        mbs.registerMBean(new Sender(), name);
+        JMXConnectorServer cs = JMXConnectorServerFactory.newJMXConnectorServer(
+                url, null, mbs);
+        cs.start();
+        try {
+            JMXServiceURL addr = cs.getAddress();
+            JMXConnector cc = JMXConnectorFactory.connect(addr);
+            MBeanServerConnection mbsc = cc.getMBeanServerConnection();
+            SnoopListener listener = new SnoopListener();
+            mbsc.addNotificationListener(name, listener, null, null);
+            mbsc.invoke(name, "send", null, null);
+            if (!listener.waitForNotification(60))
+                throw new Exception("Did not receive expected notification");
+
+            try {
+                mbsc.removeNotificationListener(name, listener);
+                throw new Exception("RemoveNL did not get SecurityException");
+            } catch (SecurityException e) {
+                System.out.println("removeNL got expected exception: " + e);
+            }
+            mbsc.invoke(name, "send", null, null);
+            if (!listener.waitForNotification(60)) {
+                int listenerCount =
+                        (Integer) mbsc.getAttribute(name, "ListenerCount");
+                System.out.println("Listener count: " + listenerCount);
+                if (listenerCount != 0)
+                    throw new Exception("TEST FAILED");
+                    /* We did not receive the notification, but the MBean still
+                     * has a listener coming from the connector server, which
+                     * means the connector server still thinks there is a
+                     * listener.  If we retained the listener after the failing
+                     * removeNL that would be OK, and if the listener were
+                     * dropped by both client and server that would be OK too,
+                     * but the inconsistency is not OK.
+                     */
+            }
+            cc.close();
+        } finally {
+            cs.stop();
+        }
+    }
+
+    private static class SnoopListener implements NotificationListener {
+        private Semaphore sema = new Semaphore(0);
+
+        public void handleNotification(Notification notification, Object handback) {
+            System.out.println("Listener got: " + notification);
+            sema.release();
+        }
+
+        boolean waitForNotification(int seconds) throws InterruptedException {
+            return sema.tryAcquire(seconds, TimeUnit.SECONDS);
+        }
+    }
+
+    private static class NoRemovePolicy extends Policy {
+        public PermissionCollection getPermissions(CodeSource codesource) {
+            PermissionCollection pc = new Permissions();
+            pc.add(new AllPermission());
+            return pc;
+        }
+
+        public void refresh() {
+        }
+
+        public boolean implies(ProtectionDomain domain, Permission permission) {
+            if (!(permission instanceof MBeanPermission))
+                return true;
+            MBeanPermission jmxp = (MBeanPermission) permission;
+            if (jmxp.getActions().contains("removeNotificationListener")) {
+                System.out.println("DENIED");
+                return false;
+            }
+            return true;
+        }
+    }
+
+    public static interface SenderMBean {
+        public void send();
+        public int getListenerCount();
+    }
+
+    public static class Sender extends NotificationBroadcasterSupport
+            implements SenderMBean {
+        private AtomicInteger listenerCount = new AtomicInteger();
+
+        public void send() {
+            System.out.println("Sending notif");
+            sendNotification(new Notification("type", this, 0L));
+        }
+
+        public synchronized int getListenerCount() {
+            return listenerCount.get();
+        }
+
+        public void removeNotificationListener(
+                NotificationListener listener,
+                NotificationFilter filter,
+                Object handback) throws ListenerNotFoundException {
+            System.out.println("Sender.removeNL(3)");
+            super.removeNotificationListener(listener, filter, handback);
+            listenerCount.decrementAndGet();
+        }
+
+        public void addNotificationListener(
+                NotificationListener listener,
+                NotificationFilter filter,
+                Object handback) {
+            System.out.println("Sender.addNL(3)");
+            super.addNotificationListener(listener, filter, handback);
+            listenerCount.incrementAndGet();
+        }
+
+        public void removeNotificationListener(NotificationListener listener)
+        throws ListenerNotFoundException {
+            System.out.println("Sender.removeNL(1)");
+            super.removeNotificationListener(listener);
+            listenerCount.decrementAndGet();
+        }
+    }
+}