hypfvieh / dbus-java

Improved version of java DBus library provided by freedesktop.org (https://dbus.freedesktop.org/doc/dbus-java/)
https://hypfvieh.github.io/dbus-java/
MIT License
185 stars 73 forks source link

SpotBugs analysis: Null passed for non-null parameter of unExportObject(String) #203

Closed ghost closed 1 year ago

ghost commented 1 year ago

From SpotBugs:

dbus-npe-03

I'm not sure if this one is a real issue:

private void handleMessage(final MethodCall _methodCall) throws DBusException {
  logger.debug("Handling incoming method call: {}", _methodCall);

  ExportedObject exportObject;
  Method meth = null;
  Object o = null;

  if( null == _methodCall.getInterface() ||
    _methodCall.getInterface().equals("org.freedesktop.DBus.Peer") ||
    _methodCall.getInterface().equals("org.freedesktop.DBus.Introspectable") ) {
      exportObject = getExportedObjects().get(null);

      if( null != exportObject && null == exportObject.getObject().get() ) {
        unExportObject(null);

The line of note from the SpotBugs report, but I'm not sure why SpotBugs is reporting an issue:

unExportObject(null);

There are some minor simplifications that can be made, relating to encapsulation violations. Consider:

if( _methodCall.isInterface("org.freedesktop.DBus.Peer") ||
    _methodCall.isInterface("org.freedesktop.DBus.Introspectable") ) {

This puts the onus of analyzing the data back into the class that has the data and can be further simplified:

if( _methodCall.interfaceMatches( "org.freedesktop.DBus.Peer", "org.freedesktop.DBus.Introspectable" ) ) {

Whether the interface is null is an implementation detail of MethodCall that clients needn't worry about.

Within MethodCall, the isInterface method (and by extension interfaceMatches) can then use a more null-safe comparison by checking the string against the interface name (as opposed to vice-versa), to obviate the null check, such as:

return "org.freedesktop.DBus.Peer".equals(getInterface());
hypfvieh commented 1 year ago

I don't see a bug here, so I won't change anything. I know SpotBugs and I know that it often reports false positives.