leibnitz27 / cfr

This is the public repository for the CFR Java decompiler
https://www.benf.org/other/cfr
MIT License
1.93k stars 249 forks source link

MatchIterator throws "ArrayIndexOutOfBoundsException and the code is not robust #270

Closed chenggwang closed 2 years ago

chenggwang commented 2 years ago

CFR Version

0.152-SNAPSHOT (44bddad)

Compiler

OpenJDK jdk-16.0.2

Description

  1. ArrayIndexOutOfBoundsException for the toString () method of Matchiterator: IDEA in Debug mode,Variable'view will call toString(),but this time idx==-1, so this method"toString()" will throw ArrayIndexOutOfBoundsException.When the advance() method has not been called,the idx has always been -1.
  2. NullPointerException for the Matchiterator's hasnext(), isfinished() method: The data.size() method will throw NullPointerException,Although in the current code, this problem will not be raised,because the constructor parameter(List data) of MatchIterator is 'ListFactory.newList()', So so far, The data of MatchIterator won't be null.But other places use this class Matchiterator,Inevitably, it will not cause NullPointerException.In terms of code robustness,data!=null is necessary

Example

//src/org/benf/cfr/reader/bytecode/analysis/opgraph/op4rewriters/SwitchStringRewriter.java
//Line:#137
private void rewriteComplex(List<StructuredStatement> structuredStatements) {
        // Rather than have a non-greedy kleene star at the start, we cheat and scan for valid start points.
        // switch OB (case OB (if-testalternativevalid OB assign break CB)* if-notvalid break assign break CB)+ CB
        MatchIterator<StructuredStatement> mi = new MatchIterator<StructuredStatement>(structuredStatements);
error
//src/org/benf/cfr/reader/bytecode/analysis/opgraph/op4rewriters/matchutil/MatchIterator.java
public class MatchIterator<T> {
    private final List<T> data;
    private int idx;

    public MatchIterator(List<T> data) {
        this.data = data;
        this.idx = -1;
    }

    public boolean hasNext() {
        return idx < data.size() - 1;
        }
    private boolean isFinished() {
        return idx >= data.size();
    }

    private MatchIterator(List<T> data, int idx) {
        this.data = data;
        this.idx = idx;
    }
    public String toString() {
        if (isFinished()) return "Finished";
        T t = data.get(idx);
        return t == null ? "null" : t.toString();
    }
}
leibnitz27 commented 2 years ago

As per comment on PR - I'm going to revert the null protection here - I'd rather it blew up noisly than silently failed. (actually, worse, if it was null isFinished would never work here, so possible infinite loop)

leibnitz27 commented 2 years ago

Have also restricted the size of the dump - this is potentially used on massive data, so walking it all is pointless. (esp since you're in the debugger here!)