debug symbols trigger false positive RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE on java11

[dmivankov]

Starting 4.4.0 and up to at least 4.5.3 there's a regression described below.
Especially annoying as is block log4j CVE mitigation upgrade to 4.5.3

import java.util.stream.Stream;

public class A {
  private int foo() {
    try (final Stream<Object> stream = Stream.of()) {
      return stream.hashCode();
    }
  }
}

with debug symbols

$ javac --version
javac 11.0.9
$ javac -g A.java && jar -cf A.jar A.class
$ java -cp spotbugs.jar edu.umd.cs.findbugs.LaunchAppropriateUI -textui -longBugCodes A.jar
M C RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE RCN: Nullcheck of stream at line 6 of value previously dereferenced in A.foo()  At A.java:[line 6]

without debug symbols

$ javac A.java && jar -cf A.jar A.class
$ java -cp spotbugs.jar edu.umd.cs.findbugs.LaunchAppropriateUI -textui -longBugCodes A.jar

Output of javap -l -p -c -v related to method foo

  private int foo();
    descriptor: ()I
    flags: (0x0002) ACC_PRIVATE
    Code:
      stack=2, locals=4, args_size=1
         0: iconst_0
         1: anewarray     #2                  // class java/lang/Object
         4: invokestatic  #3                  // InterfaceMethod java/util/stream/Stream.of:([Ljava/lang/Object;)Ljava/util/stream/Stream;
         7: astore_1
         8: aload_1
         9: invokevirtual #4                  // Method java/lang/Object.hashCode:()I
        12: istore_2
        13: aload_1
        14: ifnull        23
        17: aload_1
        18: invokeinterface #5,  1            // InterfaceMethod java/util/stream/Stream.close:()V
        23: iload_2
        24: ireturn
        25: astore_2
        26: aload_1
        27: ifnull        45
        30: aload_1
        31: invokeinterface #5,  1            // InterfaceMethod java/util/stream/Stream.close:()V
        36: goto          45
        39: astore_3
        40: aload_2
        41: aload_3
        42: invokevirtual #7                  // Method java/lang/Throwable.addSuppressed:(Ljava/lang/Throwable;)V
        45: aload_2
        46: athrow
      Exception table:
         from    to  target type
             8    13    25   Class java/lang/Throwable
            30    36    39   Class java/lang/Throwable
      LineNumberTable:
        line 5: 0
        line 6: 8
        line 7: 13
        line 6: 23
        line 5: 25
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            8      39     1 stream   Ljava/util/stream/Stream;
            0      47     0  this   LA;
      LocalVariableTypeTable:
        Start  Length  Slot  Name   Signature
            8      39     1 stream   Ljava/util/stream/Stream<Ljava/lang/Object;>;
      StackMapTable: number_of_entries = 4
        frame_type = 253 /* append */
          offset_delta = 23
          locals = [ class java/util/stream/Stream, int ]
        frame_type = 255 /* full_frame */
          offset_delta = 1
          locals = [ class A, class java/util/stream/Stream ]
          stack = [ class java/lang/Throwable ]
        frame_type = 255 /* full_frame */
          offset_delta = 13
          locals = [ class A, class java/util/stream/Stream, class java/lang/Throwable ]
          stack = [ class java/lang/Throwable ]
        frame_type = 5 /* same */

the difference is that debug version has this block

      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            8      39     1 stream   Ljava/util/stream/Stream;
            0      47     0  this   LA;
      LocalVariableTypeTable:
        Start  Length  Slot  Name   Signature
            8      39     1 stream   Ljava/util/stream/Stream<Ljava/lang/Object;>;

and triggers false positive, while non-debug version doesn't have this block and works

related to
#868
#1338
#1694

likely introduced in 07d001a #1575

[dmivankov]

Oddly enough 07d001a looks at line numbers that are present regardless of debug symbols, so could be that the exact culprit is somewhere else

[divanorama]

Generated code semantically looks like following (except that try-catch blocks are set in extra table and there are gotos between blocks)

final Stream<Object> stream = Stream.of();  // note: try-with-resources can enter body with null resource, that's not an exceptional case
try {
  stream.hashCode(); // user code
} catch (Throwable e1) {
  try {
    if (stream != null) { // javac generated code
      stream.close()
    }  
  } catch (Throwable e2) {
    e1.addSuppressed(e2)
  }
} finally { // not quite, this doesn't get executed after catch block as catch block handles close() already
  if (stream != null) { // javac generated code
    stream.close()
  }
}

but actually finally block gets inlined to become

final Stream<Object> stream = Stream.of();  // note: try-with-resources can enter body with null resource, that's not an exceptional case
try {
  stream.hashCode(); // user code
  if (stream != null) { // javac generated code, also try block ends before this line
    stream.close()
  }
} catch (Throwable e1) {
  try {
    if (stream != null) { // javac generated code
      stream.close()
    }  
  } catch (Throwable e2) {
    e1.addSuppressed(e2)
  }
}

so inspection makes sense, and maybe javac could potentially eliminate the null check.

But it doesn't explain why debug info presence triggers the inspection

[dmivankov]

Another clue is that it also matters whether resource used is an interface or a class (invokeinterface vs invokevirtual)
And that LocalVariableTypeTable (generics) is not crucial, all methods below only have LocalVariableTable in debug mode

public class A {
  private int foo(AutoCloseable x) throws Exception {
    try (var obj = x) { // RCN with debug symbols
      return obj.hashCode();
    }
  }

  private int bar(B x) throws Exception {
    try (var obj = x) { // no RCN regardless of debug symbols
      return obj.hashCode();
    }
  }

  private int baz(C x) throws Exception {
    try (var obj = x) { // RCN with debug symbols
      return obj.hashCode();
    }
  }

  private static abstract class B implements AutoCloseable {
  }

  private static interface C extends AutoCloseable {
  }
}

[dmivankov]

So in the original bytecode

        Start  Length  Slot  Name   Signature
            8      39     1 stream   Ljava/util/stream/Stream;
...            
         0: iconst_0
         1: anewarray     #2                  // class java/lang/Object
         4: invokestatic  #3                  // InterfaceMethod java/util/stream/Stream.of:([Ljava/lang/Object;)Ljava/util/stream/Stream;
         7: astore_1
         8: aload_1  // !!! here variable_1 of type Stream is valid
         9: invokevirtual #4                  // Method java/lang/Object.hashCode:()I   /// !!! variable_1 is dereferenced
        12: istore_2
        13: aload_1  //  !!! astore_1 wasn't called anywhere so far, so variable_1 value is unchanged
        14: ifnull        23  /// !!! variable_1 is checked against null
        17: aload_1
        18: invokeinterface #5,  1            // InterfaceMethod java/util/stream/Stream.close:()V
        23: iload_2
        24: ireturn
        ... /// scope of variable_1 continues further into catch blocks

having variable debug info seems to be "helping" analysis connect dereferencing with null check

[dmivankov]

will try adding invokeinterface block along with invokevirtual in https://github.com/spotbugs/spotbugs/blob/master/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java#L1907 might be the charm