PVS-Studio's Blog

The check of the Rhino JavaScript engine or how the unicorn met the rhino

Unicorn Developer's photo
Unicorn Developer
·

28 min read

Among the wide variety of programming languages, what our users want the most is that the PVS-Studio analyzer to start supporting JavaScript. The Rhino engine is a project that our team can use to create a PVS-Studio analyzer for JavaScript. In this article, we will use the PVS-Studio analyzer to check the source code of Rhino.

image.png

Beginning

The PVS-Studio analyzer has gradually evolved and supported new programming languages. The first additional language (to the original C and C++) was C#, then we supported Java. Users of the analyzer often wonder if PVS-Studio will support a new language in the future. And if so, what language will it be?

I'm one of those developers who occasionally explore something work-related in my spare time. Writing a static analyzer for the JavaScript language is one of my current areas of interest.

However, it all started with another language — Kotlin. One day, I decided to spend my free time learning a new programming language. Since I am familiar with Java and C#, my choice fell on Kotlin. In my opinion, it combines the features of both Java and C#. In the process of studying Kotlin, I learned that there is a possibility to transpile the program in Kotlin not only into Java, but also into JavaScript.

Transpiler is a type of translator that takes the source code of a program written in one programming language and produces an equivalent source code in a different programming language.

This got me very excited so I opened GitHub to study the sources of a transpiler that translates from Kotlin into JavaScript. As it turned out, developers of the transpiler did not create their own parser and classes for AST (abstract syntactic tree) — they reused the code from the Rhino engine. So, the Rhino engine became the next project to explore.

The Rhino engine has several interesting features:

  • It is written in Java;
  • Its first version was released in 1997 (25 years ago), when Java didn't even have generics (they appeared in Java 5.0 in 2004);
  • The engine works in both compiled and interpreted mode. "How is it possible to compile JavaScript?" you may ask. It's simple: code written in JavaScript is converted to Java bytecode;
  • Rhino in compiler mode often produces more performance than the C implementation of JavaScript;
  • Rhino does not have browser integration by default;
  • You can use Rhino to write application's business logic in the simpler JavaScript language instead of Java. This function allows to write business logic even for experts with little knowledge of programming.

At the same time as studying the Rhino's source code, I downloaded its source files and ran the analysis using the PVS-Studio plugin for IntelliJ IDEA. The analyzer found quite a few warnings for a 25-year-old project (they are listed in descending order of certainty):

  • High: 79
  • Medium: 158
  • Low: 208

After a brief glance at warnings, it seemed to me that some of them turned out to be false positives. Here are the most striking ones:

  • In my opinion, all V6022 diagnostic warnings turned out to be false positives. They appeared due to the API change of some methods and constructors. V6022 looks for arguments that are not used inside the method's or constructor's body. That is, the analyzer issues a warning when nothing is assigned to arguments, no properties/methods are called, arguments are not passed to any method/constructor and are not assigned to any properties/fields;

  • About 25% of the V6008 diagnostic rules are of a low certainty level (Low). The diagnostic looks for places where zero-link access can occur. The warnings appeared because the XMLList.getXML method can potentially return null if the passed index turns out to be negative. However, this will never happen, because when this method is called, an array from the _annos field is always passed as the first argument. Inside the XMLList.getXML method, the XMLList.length method is called, which is also related to the _annos field. Therefore, the method will never return a negative value:

class XMLList extends XMLObjectImpl implements Function {
  ....
  private XmlNode.InternalList _annos;
  ....
  XMLList(XMLLibImpl lib, Scriptable scope, XMLObject prototype) {
    super(lib, scope, prototype);
    _annos = new XmlNode.InternalList();
  }
  ....
  private XML getXML(XmlNode.InternalList _annos, int index) {
    if (index >= 0 && index < length()) {
      return xmlFromNode(_annos.item(index));
    } else {
      return null;
    }
  }
  ....
  @Override
  int length() {
    int result = 0;

    if (_annos != null) {
      result = _annos.length();
    }

    return result;
  }
  ....
  private XML getXmlFromAnnotation(int index) {
    return getXML(_annos, index);               //<=
  }
  ....
}

I quickly found all false positive warnings and filtered them out using filters of the plugin's interface — I removed false positives from the table with the analysis results so that they would not distract me when I view other warnings. As a result, the number of warnings in the table decreased:

  • High: 79 (did not change)
  • Medium: 47 (there were 158)
  • Low: 154 (there were 208)

After that, I reviewed other warnings and selected the most interesting ones. Let's not waste a second and look at the warnings that the PVS-Studio analyzer found in the Rhino project's code.

Forgotten field + a typo

V6001 There are identical sub-expressions 't2Docked' to the left and to the right of the '&&' operator. SwingGui.java(2718), SwingGui.java(2718)

class ContextWindow extends JPanel implements ActionListener {
  ....
  public ContextWindow(final SwingGui debugGui) {
    ....
    ComponentListener clistener =
      new ComponentListener() {
          boolean t2Docked = true;

          void check(Component comp) {
              ....
              if (leftDocked && t2Docked && rightDocked && t2Docked) { // <=
                  // no change
                  return;
              }

              t2Docked = rightDocked;

              // Further t2Docked is not used
              ....
          }
          ....      
    };
    ....
  }
  ....
}

In the ContextWindow constructor, a local object of the ComponentListener interface type is created. This object calls the check method in each of the methods that are implemented by this object. Inside the method, there is a check whether the component is aligned left or right side of some window, and whether the location of the window dividers changes in JSplitPane.

In this method, the analyzer detected an error — the t2Docked field of the clistener object (yes, this is an object field, not a variable, since it is declared in the initialization block of the local object) is used twice in the if condition:

if (leftDocked && 
    t2Docked &&     // <=
    rightDocked && 
    t2Docked) {     // <=
  // no change
  return;
}

Considering that in the check method the aligning of components is checked for the left and right sides, I thought that initially the clistener object had another t1Docked field, which was eventually deleted. In the case with the additional field, the condition in if looked more logical. It would have had an error though, most likely due to a typo, which the analyzer would also have warned about. After correcting this typo, the condition in if would become like this:

leftDocked && t1Docked && rightDocked && t2Docked

To fix the current code fragment, I would recommend using t2Docked once in the condition. For example, as follows:

t2Docked && leftDocked && rightDocked

Copy paste, impossible conditions, and unreachable code

V6007 Expression 'power < 2' is always false. UintMap.java(39)

public class UintMap implements Serializable {
  ....
  // If true, enables consitency checks
  private static final boolean check = false;
  ....
  public UintMap(int initialCapacity) {
    if (initialCapacity < 0) Kit.codeBug();
    // Table grow when number of stored keys >= 3/4 of max capacity
    int minimalCapacity = initialCapacity * 4 / 3;
    int i;
    for (i = 2; (1 << i) < minimalCapacity; ++i) {}
    power = i;
    if (check && power < 2) Kit.codeBug();      // <=
  }
  ....
}

V6007 Expression 'power < 2' is always false. ObjToIntMap.java(102)

public class ObjToIntMap implements Serializable {
  ....
  // If true, enables consitency checks
  private static final boolean check = false;
  ....
  public ObjToIntMap(int initialCapacity) {
    if (initialCapacity < 0) Kit.codeBug();
    // Table grow when number of stored keys >= 3/4 of max capacity
    int minimalCapacity = initialCapacity * 4 / 3;
    int i;
    for (i = 2; (1 << i) < minimalCapacity; ++i) {}
    power = i;
    if (check && power < 2) Kit.codeBug();      // <=
  }
  ....
}

Code for the constructors of the UintMap and ObjToIntMap classes is identical, the only difference is the argument's name. The check is performed at the end of the code of both constructors:

if (check && power < 2) Kit.codeBug();

If this check is true, the Kit.codeBug method is called and it simply throws an exception:

public static RuntimeException codeBug() throws RuntimeException {
  RuntimeException ex = new IllegalStateException("FAILED ASSERTION");
  // Print stack trace ASAP
  ex.printStackTrace(System.err);
  throw ex;
}

Firstly, the check depends on the value of the final private check field. Since this field is false by default, execution will never reach the power < 2 expression. A developer may have used this field to test the class, so the field can still take the true value when the developer will test the class again.

Secondly, the analyzer pointed out that even if the execution reaches this expression, it will always be false, because by the time the check is performed, the value of the power field will be at least 2. The thing is that power stores the value of the i local variable that is also a counter in the for loop. This variable is initialized in the loop with the 2 value, and after that is incremented at each iteration of the loop. The maximum value of i can grow to 31, because 1 << 31 = Integer.MIN_VALUE (-2147483648). If there is the minimalCapacity variable overflow, then no iteration will be performed, which means the value of the i variable will remain 2.

Because of this, the Kit.codeBug method will never be called. It means the analyzer helped detect unreachable code. I don't know how to fix this code, as I don't really understand why this check is here. Therefore, it is up to the programmer who wrote the code to fix this error.

The memory of the bug carried through the years

V6007 Expression '!BUG_369394_IS_VALID' is always true. XmlProcessor.java(288)

class XmlProcessor implements Serializable {
  ....
  private void addTextNodesToRemoveAndTrim(List<Node> toRemove, Node node) {
    if (node instanceof Text) {
      Text text = (Text) node;
      boolean BUG_369394_IS_VALID = false;
      if (!BUG_369394_IS_VALID) {                 // <=
        text.setData(text.getData().trim());
      } else {
        if (text.getData().trim().length() == 0) {
          text.setData("");
        }
      }
      if (text.getData().length() == 0) {
        toRemove.add(node);
      }
    }
    ....
  }
  ....
}

The analyzer warnings don't just help you find bugs and vulnerabilities in your code. Sometimes they also help you remember to delete unnecessary code. The developer created a branch in addTextNodesToRemoveAndTrim method that depends on the local BUG_369394_IS_VALID boolean variable, which is initialized with false and does not change over the course of the method. The analyzer noticed that and issued a warning.

This variable, as I understand it, turned out to be necessary for fixing a bug with the 369394 number from the Bugzilla website. Considering that BUG_369394_IS_VALID is set to false, this bug is most likely no longer relevant. If this is the case, it is a good idea to remove the branch that depends on this variable. If this bug is still relevant, it is not clear why the value of this variable is false and not true.

When pretty isn't so pretty

V6007 Expression 'i < indentLevel' is always false. XmlProcessor.java(454)

class XmlProcessor implements Serializable {
  ....
  private boolean prettyPrint;
  ....
  private int prettyIndent;  
  ....
  final String ecmaToXmlString(Node node) {
    // See ECMA 357 Section 10.2.1
    StringBuilder s = new StringBuilder();
    int indentLevel = 0;
    if (prettyPrint) {
      for (int i = 0; i < indentLevel; i++) { // <=
        s.append(' ');
      }
    }
    ....
  }
  ....
}

The analyzer detected that the i < indentLevel expression is always false, because the i and indentLevel variables equal 0 on the first loop. It means, the iteration of the for loop will not occur. As a result, when information is written to StringBuilder, space indentation will not be added if the prettyPrint mode is selected. This will likely worsen the readability of the text written in StringBuilder.

According to the comment at the beginning of the method, the algorithm for this method should follow the algorithm from the ECMA documentation described in section 10.2.1 (page 41). It follows from the documentation that another optional argument with the indentLevel name should be passed to the method. The error may have occurred for the following reasons. Either this argument was there originally, but then it was removed, or the developer forgot to fine-tune the method so that the algorithm corresponded the documentation.

Potential dereference of a null reference

V6008 Potential null dereference of 'generatorState' in function 'freezeGenerator'. Interpreter.java(1244), Interpreter.java(3188)

public final class Interpreter extends Icode implements Evaluator {
  ....
  private static Object interpretLoop(Context cx, 
                                      CallFrame frame, 
                                      Object throwable) {

    GeneratorState generatorState = null;
    if (throwable != null) {
      if (throwable instanceof GeneratorState) {
        generatorState = (GeneratorState) throwable;
        ....
      } else if (!(throwable instanceof ContinuationJump)) {
        Kit.codeBug();
      }
    }
    ....
    StateLoop:
    for (; ; ) {
      ....
      if (throwable != null) {
        ....
      } else {
        if (generatorState == null && frame.frozen) Kit.codeBug();
      }

      for (; ; ) {
        ....
        int op = iCode[frame.pc++];
        jumplessRun:
        {
          switch (op) {
            ....
            case ....:
              {
                if (!frame.frozen) {
                  return freezeGenerator(...., 
                                         generatorState, // <=
                                         ....);
                }                  
                ....
              }
            // many more cases
            ....
          }
        }
        ....
      }               
      ....
    }
    ....
  }
  ....
}

The analyzer reports that generatorState is null, it may be dereferenced inside the freezeGenerator method. Given that the interpretLoop method stretched for almost 1500 lines, it would be difficult (or almost impossible) to understand this without the analyzer. I have a feeling that the analyzer is correct. The proof starts from the place where the freezeGenerator method is called.

The first. The frame.frozen field is set to false by default for all CallFrame objects when cloning happens:

CallFrame cloneFrozen() {
  ....
  copy.frozen = false;        // <=
  return copy;
}

Second. The freezeGenerator method will be called only if frame.frozen is false:

....
  if (!frame.frozen) {
    return freezeGenerator(
      cx,
      frame,
      stackTop,
      generatorState,                // <=
      op == Icode_YIELD_STAR);
  }
  ....

The third. Above the call to this method, the generatorState variable is checked for null:

if (generatorState == null && frame.frozen) Kit.codeBug();

If the expression in if is true, an exception will occur, because the Kit.codeBug method throws an exception without any checks:

public static RuntimeException codeBug() throws RuntimeException {
  RuntimeException ex = new IllegalStateException("FAILED ASSERTION");
  // Print stack trace ASAP
  ex.printStackTrace(System.err);
  throw ex;
}

The fourth. The check will be performed only if the throwable argument of the interpretLoop method equals null:

if (throwable != null) { // <=
    ....
  } else {
    if (generatorState == null && frame.frozen) Kit.codeBug(); // <=
  }

The fifth. If you look even higher up the code, you will find that the original value of the generatorState variable is null. It changes only if it is an instance of the GeneratorState class and if throwable is not null:

....
  GeneratorState generatorState = null; // <=
  if (throwable != null) {
    if (throwable instanceof GeneratorState) {
      generatorState = (GeneratorState) throwable; // <=

      // reestablish this call frame
      enterFrame(cx, frame, ScriptRuntime.emptyArgs, true);
      throwable = null;
    } else if (!(throwable instanceof ContinuationJump)) {
      // It should be continuation
      Kit.codeBug();
    }
  }
  ....

Results. Taking all of the above into account, it turns out that if throwable is null or not GeneratorState, then generatorState will also be null. And if frame.frozen is false, calling the freezeGenerator method will throw a NullPointerException when the operation field is accessed:

private static Object freezeGenerator(...., 
                                      GeneratorState generatorState, 
                                      ....) {
  if (generatorState.operation == NativeGenerator.GENERATOR_CLOSE){     // <=
      // Error: no yields when generator is closing
      throw ScriptRuntime.typeErrorById("msg.yield.closing");
  }
  ....
}

Unreachable error handling

V6019 Unreachable code detected. It is possible that an error is present. Parser.java(3138)

public class Token {
  ....
  public static final int ERROR = -1, .... 
  ....
}

public class Parser {

  static final int CLEAR_TI_MASK = 0xFFFF, ....;    
  private AstNode primaryExpr() throws IOException {
    int ttFlagged = peekFlaggedToken();
    int tt = ttFlagged & CLEAR_TI_MASK; 

    switch (tt) {
      ....
      case Token.ERROR:
            consumeToken();                                               // <=
            // the scanner or one of its subroutines reported the error.
            break;
      ....
    }
    ....
  }
  ....
}

The peekFlaggedToken method returns the int value. Then, this value is changed using the bitwise operation "&" (and). And the right operand of this expression is a static unchanging constant — 0xFFFFFF (65535). Therefore, the result of a bitwise "and" will always be the number >= 0. However, further down the code, the result of the bitwise "and" is used in switch, where one of the branches is executed when the value of the expression in switch is Token.ERROR, namely -1. The analyzer warns us that this branch will never be executed.

Copy paste and missing curly brackets

V6021 The value is assigned to the 'localMax' variable but is not used. NativeRegExp.java(568)

public class NativeRegExp extends IdScriptableObject {
  ....
  private static boolean calculateBitmapSize(....) {
    ....                             
    switch (src[index]) {
      case '\\':
        ....
        switch (c) {
          ....
          case 'c':
            if ((index < end) && isControlLetter(src[index]))
              localMax = (char) (src[index++] & 0x1F);         // <=
            else --index;
            localMax = '\\';
            break;
          ....
        }
      ....
    }
    // use localMax
    ....
  }
  ....
}

In the if branch, the result of the expression:

(char) (src[index++] & 0x1F)

is saved to the localMax variable. However, the value recorded to the variable is not used until the new '\' value is written to localMax. After exiting the switch, the localMax value is used in the if condition. Therefore, it is possible that one of the if...else branches will not be executed when needed. The same class also has the processCharSetImpl method that uses a switch almost identical in structure. However, there is no error in the branch for the 'c' character because the else body is in curly brackets:

....
case 'c':
  if ((src < end) && isControlLetter(gData.regexp.source[src]))
    thisCh = (char) (gData.regexp.source[src++] & 0x1F);
  else {
    --src;
    thisCh = '\\';
  }
  break;
....

So, to fix this error, you need to wrap the instructions after else and before break into curly brackets in the NativeRegExp.calculateBitmapSize method:

case 'c':
  if ((index < end) && isControlLetter(src[index]))
    localMax = (char) (src[index++] & 0x1F);         
  else { // <=
    --index;
    localMax = '\\';
  }      // <=
  break;
....

Forgotten unused value

V6021 The value is assigned to the 'nameString' variable but is not used. QName.java(293)

final class QName extends IdScriptableObject {
  ....
  QName constructQName(XMLLibImpl lib, 
                       Context cx, 
                       Object namespace, 
                       Object name) {
    String nameString = null;
    if (name instanceof QName) {
      if (namespace == Undefined.instance) {
        return (QName) name;
      } else {
        nameString = ((QName) name).localName();          // <=
      }
    }
    if (name == Undefined.instance) {
      nameString = "";
    } else {
      nameString = ScriptRuntime.toString(name);
    }
    ....
    return newQName(lib, q_uri, q_localName, q_prefix);
  }
  ....
}

The V6021 diagnostics detected that the result of the expression is assigned to the nameString variable:

((QName) name).localName()

However, further in both branches of if...else, the nameString value is reassigned before it is used:

if (name == Undefined.instance) {
  nameString = "";
} else {
  nameString = ScriptRuntime.toString(name);
}

Most likely, this error could have occurred as a result of refactoring and the programmer did not notice it. I assume that between storing the value in nameString at the beginning of the method and changing it in if...else it was supposed to be used. Or maybe the programmer forgot to add another branch in if...else, where the nameString value was used (or at least was not changed).

Potential error and dependent arguments

V6025 Index 'sig1.length - 1' is out of bounds. NativeJavaMethod.java(461)

V6025 Index 'sig2.length - 1' is out of bounds. NativeJavaMethod.java(462)

public class NativeJavaMethod extends BaseFunction {

  private static int preferSignature(Object[] args, 
                                     Class<?>[] sig1, 
                                     boolean vararg1, 
                                     Class<?>[] sig2, 
                                     boolean vararg2) {
    int totalPreference = 0;
    for (int j = 0; j < args.length; j++) {
      Class<?> type1 = vararg1 && 
                       j >= sig1.length ? sig1[sig1.length - 1] // <=
                                        : sig1[j];             
      Class<?> type2 = vararg2 && 
                       j >= sig2.length ? sig2[sig2.length - 1] // <=
                                        : sig2[j];             
      ....
    }
    return totalPreference;
  }
}

The analyzer warns that index access in sig1[sig1.length - 1] and sig2[sig2.length - 1] expressions may cause ArrayIndexOutOfBoundsException to occur. Both of these expressions can indeed lead to the exception if the sig1 or sig2 arrays are empty, because in this case the access by the -1 (0-1 = -1) index will occur. Even checks of the j >= sig1.length and j >= sig2.length ternary operators do not protect against access by -1 index on the first iteration of the for loop.

In fact, in the current code base this method is only called in one place and the arguments that are passed to the method when the are called are dependent on each other. If vararg1 is true, the sig1 array will also have at least one element. A similar dependency exists between vararg2 and the sig2 array. This is due to Java reflection, because if calling the isVararg method of java.lang.reflect.Method or java.lang.reflect.Constructor objects returns true, then calling the getParameterTypes method will also return an array that has at least one element (the first argument, which is declared as a vararg). Objects of these classes are used in the source code as arguments for calling the preferSignature method. So in the current source code, the error the analyzer warns about will never occur. In the future, however, there is still the possibility that this method could be called incorrectly, resulting in a -1 index access.

To prevent incorrect calls to the preferSignature method in the future, I suggest that you return -1 immediately if vararg1 is true and the sig1 array is empty, or if vararg2 is true and the sig2 array is empty:

private static int preferSignature(Object[] args, 
                                     Class<?>[] sig1, 
                                     boolean vararg1, 
                                     Class<?>[] sig2, 
                                     boolean vararg2) {
    if (vararg1 && sig1.length == 0 || vararg2 && sig2.length == 0){
      return -1;
    }

    int totalPreference = 0;
    for (int j = 0; j < args.length; j++) {
      Class<?> type1 = vararg1 && 
                       j >= sig1.length ? sig1[sig1.length - 1]
                                        : sig1[j];             
      Class<?> type2 = vararg2 && 
                       j >= sig2.length ? sig2[sig2.length - 1]
                                        : sig2[j];             
      ....
    }
    return totalPreference;
  }
}

Unsafe access to an array element

V6025 Possibly index 'last - 1' is out of bounds. SuperBlock.java(50)

final class SuperBlock {
  ....
  int[] getTrimmedLocals() {
    int last = locals.length - 1;
    while (last >= 0
        && locals[last] == TypeInfo.TOP
        && !TypeInfo.isTwoWords(locals[last - 1])) {         // <=
      last--;
    }
    ....
  }
  ....
}

At the beginning of the getTrimmedLocals method, the array elements from the locals field are sorted using the while loop. However, a potential error has crept into the while loop's condition — accessing the locals array's element by index -1 can generate ArrayIndexOutOfBoundsException.

This will happen if the last loop counter is 0 and at the same time the first element of the array (index 0) is equal to the value of TypeInfo.TOP (value 0). In this case, the last expression from the while loop condition will be executed, and before the TypeInfo.isTwoWords method is called, the locals array element is accessed by index 0 - 1 = -1.

I assume that ArrayIndexOutOfBoundsException might never occur simply because somewhere in code, the first element in the locals array is always written with a value not equal to TypeInfo.TOP (value 0). In this situation, it is better to replace the index in the first check with 1 instead of 0 in the while statement to prevent the exception in the future:

while (last >= 1                          // <=
    && locals[last] == TypeInfo.TOP
    && !TypeInfo.isTwoWords(locals[last - 1])) {         
  last--;
}

Or it's possible to use the > (greater than) operator instead of the >= (greater than or equal to) operator:

while (last > 0                                  // <=
    && locals[last] == TypeInfo.TOP
    && !TypeInfo.isTwoWords(locals[last - 1])) {         
  last--;
}

Mutually annihilating operands

V6028 Identical expression 'end' to the left and to the right of compound assignment. FastDtoaBuilder.java(74)

public class FastDtoaBuilder {
  ....
  private void toFixedFormat(int firstDigit, int decPoint) {
    if (point < end) {
      ....
    } else if (point > end) {
      // large integer, add trailing zeroes
      Arrays.fill(chars, end, point, '0');
      end += point - end;                        /// <=
    }
  }
  ....
}

The analyzer found the last line in the else if block strange. If you describe the operations in the block in more detail and simplify the code, it becomes clearer what the analyzer is warning about:

end = end + point – end 
=> end = end – end + point 
=> end = + point 
=> end = point

As you can see, all that will actually happen is that the value of the point variable will be assigned to the end variable.

Most likely, there is no error in this case, and this expression remained after refactoring. However, even in this case, I would suggest replacing the use of the += operator with the usual assignment of the point value to the end variable.

Commented out code

V6032 It is odd that the body of method 'endCheckSwitch' is fully equivalent to the body of another method 'endCheckTry'. Node.java(681), Node.java(717)

public class Node implements Iterable<Node> {
  ....
  private int endCheckSwitch() {
    int rv = END_UNREACHED;

    // examine the cases
    //         for (n = first.next; n != null; n = n.next)
    //         {
    //             if (n.type == Token.CASE) {
    //                 rv |= ((Jump)n).target.endCheck();
    //             } else
    //                 break;
    //         }

    //         // we don't care how the cases drop into each other
    //         rv &= ~END_DROPS_OFF;

    //         // examine the default
    //         n = ((Jump)this).getDefault();
    //         if (n != null)
    //             rv |= n.endCheck();
    //         else
    //             rv |= END_DROPS_OFF;

    //         // remove the switch block
    //         rv |= getIntProp(CONTROL_BLOCK_PROP, END_UNREACHED);

    return rv;
  }
  ....
  private int endCheckTry() {
    int rv = END_UNREACHED;

    // a TryStatement isn't a jump - needs rewriting

    // check the finally if it exists
    //         n = ((Jump)this).getFinally();
    //         if(n != null) {
    //             rv = n.next.first.endCheck();
    //         } else {
    //             rv = END_DROPS_OFF;
    //         }

    //         // if the finally block always returns, then none of the returns
    //         // in the try or catch blocks matter
    //         if ((rv & END_DROPS_OFF) != 0) {
    //             rv &= ~END_DROPS_OFF;

    //             // examine the try block
    //             rv |= first.endCheck();

    //             // check each catch block
    //             n = ((Jump)this).target;
    //             if (n != null)
    //             {
    //                 // point to the first catch_scope
    //                 for (n = n.next.first; n != null; n = n.next.next)
    //                 {
    //                     // check the block of user code in the catch_scope
    //                     rv |= n.next.first.next.first.endCheck();
    //                 }
    //             }
    //         }

    return rv;
  }
}

This warning doesn't mean the code contains an error — the V6032 diagnostic detects methods that perform the same thing. Judging by the comments and commented out code pieces inside these functions, developers intended to fine-tune both methods in the future. Thus, the analyzer helped to detect methods with commented-out code that need to be finalized. The analyzer's warning helps the programmer not to forget to refine these methods in the future, as the code doesn't have a TODO comment here.

The hard life of Java programmers without compiler directives

V6048 This expression can be simplified. Operand 'lParenIndex' in the operation equals 0. ClassFileWriter.java(2658)

public class ClassFileWriter {
  ....
  private int[] createInitialLocals() {
    int[] initialLocals = new int[itsMaxLocals];
    ....
    // No error checking should be necessary, sizeOfParameters does this
    String type = itsCurrentMethod.getType();
    int lParenIndex = type.indexOf('(');
    int rParenIndex = type.indexOf(')');
    if (lParenIndex != 0 || rParenIndex < 0) {
      throw new IllegalArgumentException("bad method type");
    }
    int start = lParenIndex + 1;                                  // <=
    StringBuilder paramType = new StringBuilder();
    while (start < rParenIndex) {
      switch (type.charAt(start)) {
      ....
      }
      ....
    }
    return initialLocals;  
  }
  ....
}

The string returned by the itsCurrentMethod.getType method needs to be valid, namely, start with an opening parenthesis "(" and have a closing parenthesis ")". This follows from the comment before this method is called. However, someone decided to play it safe and added if with checks of the values of the lParenIndex and rParenIndex variables — if one of these checks turns out to be true, an exception will be generated. As a result, after if, the lParenIndex variable's value is now always 0. This is what the analyzer discovered.

I studied in detail all places where objects of ClassFileMethod type are created. These objects are stored in itsCurrentMethod field. I have come to the conclusion that the lParenIndex != 0 || rParenIndex < 0 condition will never be executed. In all cases, the string stored in the itsCurrentMethod.type field starts with the opening parenthesis '(' and always contains one closing parenthesis ')'.

For better code readability I would initialize the start variable with just 1:

private int[] createInitialLocals() {
  int[] initialLocals = new int[itsMaxLocals];
  ....
  // No error checking should be necessary, sizeOfParameters does this
  String type = itsCurrentMethod.getType();
  int lParenIndex = type.indexOf('(');
  int rParenIndex = type.indexOf(')');
  if (lParenIndex != 0 || rParenIndex < 0) {
      throw new IllegalArgumentException("bad method type");
  }
  int start = 1; // <=
  StringBuilder paramType = new StringBuilder();
  while (start < rParenIndex) {
    switch (type.charAt(start)) {
    ....
    }
    ....
  }
  return initialLocals;  
}

Or, you could add a static private final DEBUG boolean field, and describe what it is for in the comment above the field. Moreover, developers have already done this in some classes — the PVS-Studio analyzer warned about unreachable code (V6019). During development, to check the value of type, the DEBUG field would have to be set to true, and during the release – to false:

// set the true value if you need to check 
// currentMethodType in the createInitialLocals method
private static final boolean DEBUG = false;
....
private int[] createInitialLocals() {
  int[] initialLocals = new int[itsMaxLocals];
  ....
  // No error checking should be necessary, sizeOfParameters does this
  String type = itsCurrentMethod.getType();
  int lParenIndex = type.indexOf('(');
  int rParenIndex = type.indexOf(')');

  if (DEBUG &&                             // <=
     (lParenIndex != 0 || rParenIndex < 0)
  ) {
    throw new IllegalArgumentException("bad method type");
  }

  int start = 1; 
  StringBuilder paramType = new StringBuilder();
  while (start < rParenIndex) {
    switch (type.charAt(start)) {
    ....
    }
    ....
  }
  return initialLocals;  
}

A caller and an argument are the same object

V6100 An object is used as an argument to its own method. Consider checking the first actual argument of the 'setParentScope' method. Scope.java(123)

public class Scope extends Jump {
  ....
  public static Scope splitScope(Scope scope) {
    Scope result = new Scope(scope.getType());
    result.symbolTable = scope.symbolTable;
    scope.symbolTable = null;
    result.parent = scope.parent;
    result.setParentScope(scope.getParentScope());
    result.setParentScope(result);                   // <= 
    scope.parent = result;
    result.top = scope.top;
    return result;
  }
  ....
}

In the splitScope method, the analyzer found it strange that the same result object is passed to the setParentScope method as an argument when the result.setParentScope(result) is called. This is strange because inside the method's body of the class instance, the current instance is always accessed via this anyway. Moreover, the strangest thing is that the same method is called on the same object before the call pointed out by the analyzer. However, in this case the argument is the result of the scope.getParentScope() expression. If the setParentScope method were a normal setter method for the parentScope field, a repeated call of the setParentScope method would simply overwrite the previous value, which would 100% be an error. However, it's not that simple. Let's take a closer look at the setParentScope method:

public void setParentScope(Scope parentScope) {
  this.parentScope = parentScope;
  this.top = parentScope == null ? (ScriptNode) this 
                                 : parentScope.top;
}

This method, besides saving the passed argument to the parentScope field, also has a side effect. If the argument is null, a reference to the current instance of the object — this — is stored to the top field. If the argument is not null, the top field stores a reference to the object from the top field of the passed argument.

If you display the effect of calls to setParentScope methods in the splitScope method when calling scope.getParentScope() returns null in the form of a table, you get the result:

image.png

If the scope call.getParentScope() call doesn't not return null, you get the following result:

image.png

I cannot say for sure that the repeated call of the result.setParentScope(result) method is an error, but it looks very strange. I think, it is up to developers to review this code and fix it, if necessary.

Useful false-positive positives

V6106 Casting expression to short type before implicitly casting it to other type may be excessive or incorrect. ClassFileWriter.java(796)

public class ClassFileWriter {
  ....
  public void addInvoke(int theOpCode, 
                        String className, 
                        String methodName, 
                        String methodType) {
    if (DEBUGCODE) {
        System.out.println("Add "
                        + bytecodeStr(theOpCode)
                        + ", "
                        + className
                        + ", "
                        + methodName
                        + ", "
                        + methodType);
    }
    int parameterInfo = sizeOfParameters(methodType);
    int parameterCount = parameterInfo >>> 16;  // <= not commented
    int stackDiff = (short) parameterInfo;      // <= analyzer warning

    ....
  }
  ....
}

V6106 Casting expression to short type before implicitly casting it to other type may be excessive or incorrect. ClassFileWriter.java(847)

public class ClassFileWriter {
  ....
  public void addInvokeDynamic(String methodName, 
                               String methodType, 
                               MHandle bsm, 
                               Object... bsmArgs) {
    if (DEBUGCODE) {
      System.out.println("Add invokedynamic, " +
                         methodName +
                         ", " +
                         methodType);
    }
    // JDK 1.7 major class file version is required for invokedynamic
    if (MajorVersion < 51) {
        throw new RuntimeException(
          "Please build and run with JDK 1.7 for invokedynamic support");
    }

    int parameterInfo = sizeOfParameters(methodType);
    // int parameterCount = parameterInfo >>> 16;  // <= commented
    int stackDiff = (short) parameterInfo;         // <= analyzer warning
    ....
  }
  ....
```}
Finally, the last couple of warnings of this article. Both warnings, in my opinion, are false-positives. However, looking through them, I found a real error. The code at the beginning of the *addInvoke* and *addInvokeDynamic* methods is very similar, which means that it is likely that one of these methods was created by copying the other. However, at the beginning of the *addInvokeDynamic* method, the line with a bitwise shift of the *parameterInfo* variable is commented out, while in the *addInvoke* method an identical line is not commented out. Most likely, a developer forgot to remove the comment from the line where the *addInvokeDynamic* method is. And due to this, it seems that the behavior of the *addInvokeDynamic* method will be incorrect. This can lead to even more errors when the program is running, and these errors may potentially be overlooked.

The found error has never manifested itself yet, because the *addInvokeDynamic* method is not called anywhere in the project.

Removing this comment is all that is needed to ensure that no one will encounter incorrect behaviour in the future when the *addInvokeDynamic* public method is called:

public class ClassFileWriter { .... public void addInvokeDynamic(....) { ....

int parameterInfo = sizeOfParameters(methodType);
int parameterCount = parameterInfo >>> 16;    // <= comment removed
int stackDiff = (short) parameterInfo;          
....

} .... } ```

Conclusion

As you can see, even a 25-year-old project that is used in many other projects and utilities can still hide errors. The longer an error lives, the more resources are required to fix it. Therefore, it is best if the bugs are detected at the stage of code writing — it will greatly reduce the potential negative effect of bugs. A regular use of code analysis tools (not just one, but several at a time, preferably), such as for instance, PVS-Studio, helps in this difficult matter.

If you are interested in the PVS-Studio analyzer and you want to try it out on your project, just download the distribution and feel free to use the analyzer during the trial period.

P.S. I'm currently continuing research on projects written in Java that process JavaScript code, so stay tuned for more articles on project analysis. As an alternative to JavaScript engines, I am also reviewing TypeScript Compiler API as a tool for writing a static analyzer for JavaScript. This is due to the fact that TypeScript is a superset of JavaScript. I'd love to hear in the comments what diagnostics for JavaScript you'd like to see.

Did you find this article valuable?

Support PVS-Studio's Blog by becoming a sponsor. Any amount is appreciated!

PVS-Studiojavascript enginestatic code analysisSASTOpen Source