Bug #1567
NullPointerException in Admin Applet
100%
History
#1 Updated by Ovidiu Maxiniuc over 11 years ago
The initial issue was that the "Add ACL" button is not working. However, after analyzing the callstack that was thrown:
Exception in thread "AWT-EventQueue-1" java.lang.NullPointerException
at com.goldencode.p2j.admin.client.AdminACL.handleAddACL(AdminACL.java:1497)
at com.goldencode.p2j.admin.client.AdminACL.access$600(AdminACL.java:61)
at com.goldencode.p2j.admin.client.AdminACL$5.actionPerformed(AdminACL.java:822)
at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1995)
at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2318)
at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:387)
at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:242)
at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:236)
at java.awt.Component.processMouseEvent(Component.java:6290)
at javax.swing.JComponent.processMouseEvent(JComponent.java:3267)
at java.awt.Component.processEvent(Component.java:6055)
at java.awt.Container.processEvent(Container.java:2039)
at java.awt.Component.dispatchEventImpl(Component.java:4653)
at java.awt.Container.dispatchEventImpl(Container.java:2097)
at java.awt.Component.dispatchEvent(Component.java:4481)
at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4575)
at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4236)
at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4166)
at java.awt.Container.dispatchEventImpl(Container.java:2083)
at java.awt.Component.dispatchEvent(Component.java:4481)
at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:648)
at java.awt.EventQueue.access$000(EventQueue.java:84)
at java.awt.EventQueue$1.run(EventQueue.java:607)
at java.awt.EventQueue$1.run(EventQueue.java:605)
at java.security.AccessController.doPrivileged(Native Method)
at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:87)
at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:98)
at java.awt.EventQueue$2.run(EventQueue.java:621)
at java.awt.EventQueue$2.run(EventQueue.java:619)
at java.security.AccessController.doPrivileged(Native Method)
at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:87)
at java.awt.EventQueue.dispatchEvent(EventQueue.java:618)
at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)
I found out that
com.goldencode.p2j.admin.client.AdminClient.resourceTypes was not correctly initialized before accessing it.
Indeed, when logon/authentication takes place a null pointer is thrown:
Exception in thread "AWT-EventQueue-1" java.lang.NullPointerException
at com.goldencode.p2j.security.SecurityAdmin.getAdminDef(SecurityAdmin.java:999)
at com.goldencode.p2j.admin.AdminServerImpl.getAdminDef(AdminServerImpl.java:353)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:675)
at com.goldencode.p2j.net.Conversation.block(Conversation.java:316)
at com.goldencode.p2j.net.Conversation.run(Conversation.java:158)
at java.lang.Thread.run(Thread.java:662)
As a result
com.goldencode.p2j.admin.client.AdminLogon.logon() ends abruptly and lets uninitialized some fields in com.goldencode.p2j.admin.client.AdminClient because postLogon() is not called any more.#2 Updated by Ovidiu Maxiniuc over 11 years ago
The initial exception (null pointer) is thrown in AdminLogon.logon() at line 492.
The call to com.goldencode.p2j.admin.AdminExports.getAdminDef() ends abruptly with a NullPointerException.
I was able to follow the call stack up to the following:
ThreadSafeQueue<E>.enqueue(E) line: 79 Queue.enqueueOutbound(Message) line: 802 Queue.transactImpl(Message, int) line: 1088 Queue.transact(Message, int) line: 575 DirectSession(BaseSession).transact(Message, int) line: 178 HighLevelObject.transact(RoutingKey, Object[]) line: 163 RemoteObject$RemoteAccess.invokeCore(Object, Method, Object[]) line: 1407 RemoteObject$RemoteAccess(InvocationStub).invoke(Object, Method, Object[]) line: 97 $Proxy0.getAdminDef() line: not available AdminLogon.logon() line: 494
There is no indication of any issue in the server log.
#3 Updated by Ovidiu Maxiniuc over 11 years ago
I found out and fixed the cause. In com.goldencode.p2j.security.AdminResource, the AdminResource ar is initialized in constructor as
ar = (AdminResource)sm.getPluginInstance("admin");However, because so it is configured, it is returned as
null. There are 6 references of this field but in only one place there is no null-guard before accessing its content (in getAdminDef():999).
I added the guard but I have an issue here, if ar is null, the newly created AdminDef.Profile s should have permission for this resource instance set to true or false ? For the moment, I assumed false even though the other guards are using true:
def.profile[i] = new AdminDef.Profile(prof[i], (null == ar) ? false : ar.checkAnyAdminAccess(prof[i]));
Now, after recompiling the project, the original NullPointerException does not occur any more.
Instead another one can be seen in console:
Exception in thread "AWT-EventQueue-1" java.lang.NullPointerException
at com.goldencode.p2j.admin.client.AdminClient.postLogon(AdminClient.java:796)
at com.goldencode.p2j.admin.client.AdminLogon.logon(AdminLogon.java:524)
at com.goldencode.p2j.admin.client.AdminLogon.access$100(AdminLogon.java:62)
at com.goldencode.p2j.admin.client.AdminLogon$2.actionPerformed(AdminLogon.java:249)
at javax.swing.JTextField.fireActionPerformed(JTextField.java:492)
at javax.swing.JTextField.postActionEvent(JTextField.java:705)
at javax.swing.JTextField$NotifyAction.actionPerformed(JTextField.java:820)
at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1639)
at javax.swing.JComponent.processKeyBinding(JComponent.java:2851)
at javax.swing.JComponent.processKeyBindings(JComponent.java:2886)
at javax.swing.JComponent.processKeyEvent(JComponent.java:2814)
...
Looking at AdminClient.java:796 you can see that the menuExt remains null because loadCustomExtensionsPlugin(String pluginClass) returns true without initializing it.
For the moment, I added, again, a null-guard here:
if (null != menuExt)
{
menuExt.insertCustomMenuItems(menuList);
}
However, the
menuList does contain a few items and they should be displayed in some kind of menu, I think.
At this moment, the "Add ACL" button is functioning fine, but I am not very confident about the patches I have done.
Please advise.
#4 Updated by Greg Shah over 11 years ago
- Status changed from New to WIP
- Assignee set to Ovidiu Maxiniuc
Constantin and Stanislav: please review this issue and advise Ovidiu.
#5 Updated by Constantin Asofiei over 11 years ago
About the NPE in SecurityAdmin.getAdminDef: if the ar field is null, then you should either use true or the value returned by sr.checkAdminAccess() (not sure which, as sr.checkAdminAccess() is used in some places to protected against null ar field and other places just assume true if ar field is null) - please check the implications if sr.checkAdminAccess() and if true is used.
About the NPE in AdminClient.postlogon - you are correct, you need to protect against null menuExt. But I think the entire "rebuild the menu with custom modifications" block can be skipped if null menuExt (please test):
if (null != menuExt)
{
// rebuild the menu with the custom modifications
List<JMenuItem>menuList = menu.getMenuList();
menuExt.insertCustomMenuItems(menuList);
menu.buildMenu(adminDef.profile);
setJMenuBar(menu.getMenu());
}
The menuList contains the menus as displayed in the admin console, it is passed to the insertCustomMenuItems call because the custom extension plugin can add custom menu items to the menubar.
#6 Updated by Ovidiu Maxiniuc over 11 years ago
- File om_upd20121002a.zip added
- Status changed from WIP to Review
Attached the old fix for issue.
Please review.
#7 Updated by Greg Shah over 11 years ago
I am OK with the changes. The only problem I see is that both files need their copyright date updated.
I also prefer (menuExt != null) instead of (null != menuExt). The reason is how it "reads". The first reads "null is not equivalent to menuExt" and the second reads "menuExt is not null". The first form requires a bit more thinking (at least it does for me) to determine its logical meaning.
Please update the copyright notices and repost the result here. Don't delete the original update file.
I assume you have tested this code in a generic P2J environment. The resulting code should be tested in a Majic environment since they heavily use the admin console. You can work with Stanislav or Constantin to get access to a proper environment for testing. Report the results here and so long as everything is working OK, we will schedule the check in to CVS and the application of the change to TIMCO's staging.
#8 Updated by Ovidiu Maxiniuc over 11 years ago
- File om_upd20121105a.zip added
I updated the files and attached the new update patch.
The (null == somePtr) test comes from my experience as a C/C++ programmer. This is is a good practice because if, by mistake, you type just one = the compile will not complain and the test will always fail, because in C/C++ NULL is 0, aka false. A side effect of this is that the pointer (or any other variable) will be altered by assignment. It is, indeed, awkward, and I am glad that Java is really a strong-typed languages so this kind of 'good practice' is not needed any more.
I tested the changes in Majic environment and everything is Ok. In fact the changes are irrelevant to Majic as the new code handles the lack of custom menu extensions.
The changes in the code do not interfere with normal run of the application because the Majic directory storage declares some extensions which makes the program flow to avoid the locations where the NPE was thrown. This is probably why this bug was not visible on the production environment.
However, to be sure, I altered the gso-directory.xml so that the admin interface lacked the extended menu items and also execute the tests with the old and new code for comparison. The new code worked fine.
#9 Updated by Greg Shah over 11 years ago
I am OK with the final result. You can check it into CVS. Don't forget to tag the specific files with their Hxxx history tag. Please work with Constantin to apply the change to staging. When it is both in CVS and in staging, you can send out the update notification email (with the zip file attached).
#10 Updated by Constantin Asofiei over 11 years ago
I've applied the update to staging; P2J is rebuilt.
#11 Updated by Greg Shah over 11 years ago
Please make sure this update is checked into Bazaar and I will close the issue. Remember that in Bazaar, we do NOT tag each file with a history tag. Just make sure that both files are checked in with the same "bzr commit" command, by explicitly listing those files (and ONLY those files). Don't check in other pending files with the same command.
#12 Updated by Ovidiu Maxiniuc over 11 years ago
Files committed. You can close the issue.
#13 Updated by Greg Shah over 11 years ago
- Status changed from Review to Closed
- % Done changed from 0 to 100