Fix for FindReturnRef crash when the value of an expression is assigned to a field #3269

gtoison · 2025-01-19T13:47:42Z

As reported in #3266 FindReturnRef crashes on:

        @Override
        public Builder setCharset(final Charset charset) {
            nullCharset = charset == null;
            return super.setCharset(charset);
        }

This seems to be triggered by the CFG optimization in:

spotbugs/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/BetterCFGBuilder2.java

Lines 661 to 703 in 4f66ba3

    
           if (i instanceof IFNULL || i instanceof IFNONNULL) { 
        
               IfInstruction ii = (IfInstruction) i; 
        
               InstructionHandle target = ii.getTarget(); 
        
               InstructionHandle next1 = head.getNext(); // ICONST 
        
               if (next1 == null) { 
        
                   break; 
        
               } 
        
               if (next1.getInstruction() instanceof ICONST) { 
        
                   InstructionHandle next2 = next1.getNext(); // GOTO 
        
                   if (next2 == null) { 
        
                       break; 
        
                   } 
        
                   InstructionHandle next3 = next2.getNext(); // ICONST 
        
                   if (next3 == null) { 
        
                       break; 
        
                   } 
        
                   InstructionHandle next4 = next3.getNext(); 
        
                   if (next4 == null) { 
        
                       break; 
        
                   } 
        
                   if (target.equals(next3) && next2.getInstruction() instanceof GOTO 
        
                           && next3.getInstruction() instanceof ICONST && next1.getTargeters().length == 0 
        
                           && next2.getTargeters().length == 0 && next3.getTargeters().length == 1 
        
                           && next4.getTargeters().length == 1) { 
        
                       int c1 = ((ICONST) next1.getInstruction()).getValue().intValue(); 
        
                       GOTO g = (GOTO) next2.getInstruction(); 
        
                       int c2 = ((ICONST) next3.getInstruction()).getValue().intValue(); 
        
                       if (g.getTarget().equals(next4) && (c1 == 1 && c2 == 0 || c1 == 0 && c2 == 1)) { 
        
                           boolean nullIsTrue = i instanceof IFNULL && c2 == 1 || i instanceof IFNONNULL && c2 == 0; 
        
                           if (nullIsTrue) { 
        
                               // System.out.println("Found NULL2Z instruction"); 
        
                               head.swapInstruction(new NULL2Z()); 
        
                           } else { 
        
                               // System.out.println("Found NONNULL2Z instruction"); 
        
                               head.swapInstruction(new NONNULL2Z()); 
        
                           } 
        
                           next3.removeAllTargeters(); 
        
                           next4.removeAllTargeters(); 
        
                           next1.swapInstruction(new NOP()); 
        
                           next2.swapInstruction(new NOP()); 
        
                           next3.swapInstruction(new NOP());

For instance the following bytecode:

     0  aload_0 [this]
     1  aload_1 [x]
     2  ifnonnull 9
     5  iconst_1
     6  goto 10
     9  iconst_0
    10  putfield ghIssues.Issue3266.initialized : boolean [2]
    13  return

Is transformed to add the synthetic (made up) NONNULL2Z instruction and the instruction targeters are removed.

See #3080

This should fix #3266

In case the CFG was optimized the instruction targeters are removed

gtoison · 2025-01-24T19:25:27Z

Although the symptoms are similar, #3265 is a different issue, I'll see if I can come up with a separate fix for it

test: reproducer for issue spotbugs#3266

1216993

gtoison added the bug label Jan 19, 2025

fix: check if the instruction has targeters before checking branches

690175a

In case the CFG was optimized the instruction targeters are removed

gtoison force-pushed the issue-3266 branch from 760f59e to 690175a Compare January 19, 2025 14:02

PhilippWendler mentioned this pull request Jan 20, 2025

IllegalStateException in FindReturnRef since SpotBugs 4.9.0 (getFullyQualifiedMethodName called while not visiting method) #3265

Closed

gtoison added 2 commits January 24, 2025 19:21

Merge remote-tracking branch 'origin/master' into issue-3266

356542e

doc: added a changelog entry

2e826c9

gtoison marked this pull request as ready for review January 24, 2025 19:24

Merge remote-tracking branch 'origin/master' into issue-3266

85359e5

hazendaz approved these changes Jan 26, 2025

View reviewed changes

hazendaz merged commit 9b8228e into spotbugs:master Jan 26, 2025
11 of 15 checks passed

hazendaz added this to the SpotBugs 4.9.1 milestone Feb 1, 2025

hazendaz assigned hazendaz and gtoison Feb 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix for FindReturnRef crash when the value of an expression is assigned to a field #3269

Fix for FindReturnRef crash when the value of an expression is assigned to a field #3269

Uh oh!

gtoison commented Jan 19, 2025

Uh oh!

gtoison commented Jan 24, 2025

Uh oh!

Uh oh!

Uh oh!

	if (i instanceof IFNULL \|\| i instanceof IFNONNULL) {
	IfInstruction ii = (IfInstruction) i;
	InstructionHandle target = ii.getTarget();
	InstructionHandle next1 = head.getNext(); // ICONST
	if (next1 == null) {
	break;
	}
	if (next1.getInstruction() instanceof ICONST) {
	InstructionHandle next2 = next1.getNext(); // GOTO
	if (next2 == null) {
	break;
	}
	InstructionHandle next3 = next2.getNext(); // ICONST
	if (next3 == null) {
	break;
	}
	InstructionHandle next4 = next3.getNext();
	if (next4 == null) {
	break;
	}
	if (target.equals(next3) && next2.getInstruction() instanceof GOTO
	&& next3.getInstruction() instanceof ICONST && next1.getTargeters().length == 0
	&& next2.getTargeters().length == 0 && next3.getTargeters().length == 1
	&& next4.getTargeters().length == 1) {
	int c1 = ((ICONST) next1.getInstruction()).getValue().intValue();
	GOTO g = (GOTO) next2.getInstruction();
	int c2 = ((ICONST) next3.getInstruction()).getValue().intValue();
	if (g.getTarget().equals(next4) && (c1 == 1 && c2 == 0 \|\| c1 == 0 && c2 == 1)) {
	boolean nullIsTrue = i instanceof IFNULL && c2 == 1 \|\| i instanceof IFNONNULL && c2 == 0;

	if (nullIsTrue) {
	// System.out.println("Found NULL2Z instruction");
	head.swapInstruction(new NULL2Z());

	} else {
	// System.out.println("Found NONNULL2Z instruction");
	head.swapInstruction(new NONNULL2Z());
	}
	next3.removeAllTargeters();
	next4.removeAllTargeters();
	next1.swapInstruction(new NOP());
	next2.swapInstruction(new NOP());
	next3.swapInstruction(new NOP());

Fix for FindReturnRef crash when the value of an expression is assigned to a field #3269

Fix for FindReturnRef crash when the value of an expression is assigned to a field #3269

Uh oh!

Conversation

gtoison commented Jan 19, 2025

Uh oh!

gtoison commented Jan 24, 2025

Uh oh!

Uh oh!

Uh oh!