Skip to content

CombineSemanticallyEqualCatchBlocks indexes J.Case#getCaseLabels() unguarded, and only ever compares the first label #985

Description

@timtebeek

Found while auditing for the crash class fixed in the PR that guards ForLoopIncrementInUpdate, WhileInsteadOfFor and DefaultComesLastVisitor against J collections that non-Java parsers leave empty.

CombineSemanticallyEqualCatchBlocks.java:599:

J.Case compareTo = (J.Case) j;
if (_case.getStatements().size() != compareTo.getStatements().size() ||
        doesNotContainSameComments(_case.getPrefix(), compareTo.getPrefix())) {
    isEqual.set(false);
    return _case;
}

this.visit(_case.getCaseLabels().get(0), compareTo.getCaseLabels().get(0));

Two problems:

  1. Unguarded .get(0). The size check above covers getStatements(), not getCaseLabels(). The Java parser always puts at least one label on a J.Case (default: becomes a J.Identifier named "default"), but other parsers do not — Go emits a J.Case with no labels at all, which is what made DefaultComesLastVisitor.isDefaultCase throw IndexOutOfBoundsException in production. Not reachable from Go here, since this comparator only descends into catch bodies and Go has no try/catch, but reachable from any J-based language that has both try/catch and a label-less case.

  2. Only label 0 is compared. For a multi-label case the remaining labels are ignored, so case 1, 2: compares equal to case 1, 3: and two catch blocks that are not semantically equal can be combined. This one bites plain Java today.

The natural fix closes both: add getCaseLabels().size() to the early-return size check, then loop over all labels rather than indexing 0. Since that changes Java behaviour for multi-label cases it wants its own tests, which is why it was left out of the crash-fix PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions