# HG changeset patch # User uvangapally # Date 1494397808 -19800 # Node ID adbeae0f677e762e98913d255205274601ba3d00 # Parent 30e75693ae99fd8e47fd2f5116527aff1b59aff9 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 diff -r 30e75693ae99 -r adbeae0f677e jdk/src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnector.java --- 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); } } diff -r 30e75693ae99 -r adbeae0f677e jdk/src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java --- 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 ids = new ArrayList(); + List values = + new ArrayList(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 values = + new ArrayList(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 ids = new ArrayList(); - List values = - new ArrayList(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 id = null; - - List values = - new ArrayList(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(); + Integer liId = getListenerId(name, listener, + filter, handback); + infoList.remove(liId); - infoList.remove(id); - - break; - } - } - - if (id == null) - throw new ListenerNotFoundException("Listener not found"); - - return id; + return liId; } public synchronized Integer[] removeNotificationListener(ObjectName name) { diff -r 30e75693ae99 -r adbeae0f677e jdk/test/javax/management/remote/mandatory/notif/NoPermToRemoveTest.java --- /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(); + } + } +}