Analysis of the Apache Dubbo RPC Framework by the PVS-Studio Static Code Analyzer

Apache Dubbo is one of the most popular Java projects on GitHub. It's not surprising. It was created 8 years ago and is widely applied as a high-performance RPC environment. Of course, most of the bugs in its code have long been fixed and the quality of the code is maintained at a high level. However, there is no reason to opt out of checking such an interesting project using the PVS-Studio static code analyzer. Let's see how it turned out.

About PVS-Studio

The PVS-Studio static code analyzer has been around for more than 10 years on the IT market and is a multifunctional and easy-to-introduce software solution. At the moment, the analyzer supports C, C++, C#, Java and works on Windows, Linux and macOS.

PVS-Studio is a B2B-solution and is used by a large number of teams in various companies. If you'd like to estimate the analyzer capabilities, you are welcome to download the distributive and request a trial key here.

Also there are options how you can use PVS-Studio for free in open source projects or if you're a student.

Apache Dubbo: What It Is and Main Features

Nowadays, almost all large software systems are distributed. If in a distributed system there is an interactive connection between remote components with low latency and relatively little data to be passed, it's a strong reason to use an RPC (remote procedure call) environment.

Apache Dubbo is a high-performance RPC (remote procedure call) environment with open source code based on Java. The same as many other RPC systems, dubbo is based on the idea of creating a service defining some methods which can be remotely called with their parameters and types of return values. On the server side, an interface is implemented and the dubbo's server is launched to handle customer enquiries. On the client side, there is a stub that provides the same methods as the server does. Dubbo suggests three key functions, which include interface remote calling, fail-tolerance and load balancing, as well as automatic registration and service discovery.

About the Analysis

The actions sequence of the analysis is quite simple and didn't require much time in my case:

• Downloaded Apache Dubbo from GitHub;
• Used start-up Java analyzer instructions and ran the analysis;
• Got the analyzer report, reviewed it and highlighted interesting cases.

The analysis results: 73 warnings of High and Medium levels of certainty (46 and 27, respectively) were issued for 4000+ files, which is a nice indicator of the code quality.

Not all warnings are errors. It's a usual outcome, as before directly using the analyzer, one has to configure it. After that, you can expect a fairly low percent of false positives (example).

I didn't consider 9 warnings (7 High and 2 Medium), related to files with tests.

As a result, I had a small number of warnings, which also included false positives, because I haven't configured the analyzer. Delving into all 73 warnings with further description in the article is long, ridiculous and tedious, so I chose the most sapid ones.

Sign Byte in Java

V6007 Expression 'endKey[i] < 0xff' is always true. OptionUtil.java(32)

public static final ByteSequence prefixEndOf(ByteSequence prefix) {
  byte[] endKey = prefix.getBytes().clone();
  for (int i = endKey.length - 1; i >= 0; i--) {
    if (endKey[i] < 0xff) {                                           // <=
      endKey[i] = (byte) (endKey[i] + 1);
      return ByteSequence.from(Arrays.copyOf(endKey, i + 1));
    }
  }
  return ByteSequence.from(NO_PREFIX_END);
}

A value of the byte type (the value from -128 to 127) is compared with the value 0xff (255). In this condition, it's not taken into account that the byte type in Java is signed, therefore the condition will always be true, which means it's meaningless.

Return of the Same Values

V6007 Expression 'isPreferIPV6Address()' is always false. NetUtils.java(236)

private static Optional<InetAddress> toValidAddress(InetAddress address) {
  if (address instanceof Inet6Address) {
    Inet6Address v6Address = (Inet6Address) address;
    if (isPreferIPV6Address()) {                               // <= 
      return Optional.ofNullable(normalizeV6Address(v6Address));
    }
  }
  if (isValidV4Address(address)) {
    return Optional.of(address);
  }
  return Optional.empty();
}

The method isPreferIPV6Address.

static boolean isPreferIPV6Address() {
  boolean preferIpv6 = Boolean.getBoolean("java.net.preferIPv6Addresses");
  if (!preferIpv6) {
    return false;                                                     // <=
  }
  return false;                                                         // <=
}

The isPreferIPV6Address method returns false in both cases. Most likely, a developer wanted one case to return true, otherwise the method doesn't make any sense.

Pointless Checks

V6007 Expression '!mask[i].equals(ipAddress[i])' is always true. NetUtils.java(476)

public static boolean matchIpRange(....) throws UnknownHostException {
  ....
  for (int i = 0; i < mask.length; i++) {
    if ("*".equals(mask[i]) || mask[i].equals(ipAddress[i])) {
      continue;
    } else if (mask[i].contains("-")) {
       ....
    } else if (....) {
      continue;
    } else if (!mask[i].equals(ipAddress[i])) {                 // <=
      return false;
    }
  }
  return true;
}

In the condition if-else-if, the check "*".equals(mask[i]) || mask[i].equals(ipAddress[i]) is performed. If the condition isn't met, the next check in if-else-if occurs which shows us that mask[i] and ipAddress[i] aren't equal. But in one of the following checks in if-else-if it's checked that mask[i] and ipAddress[i] aren't equal. Since mask[i] and ipAddress[i] aren't assigned any values, the second check is pointless.

V6007 Expression 'message.length > 0' is always true. DeprecatedTelnetCodec.java(302)

V6007 Expression 'message != null' is always true. DeprecatedTelnetCodec.java(302)

protected Object decode(.... , byte[] message) throws IOException {
  ....
  if (message == null || message.length == 0) {
    return NEED_MORE_INPUT;
  }
  ....
  // Here the variable <i>message </i> doesn't change!
  ....
  if (....) {
    String value = history.get(index);
    if (value != null) {
      byte[] b1 = value.getBytes();
      if (message != null && message.length > 0) {                         // <=
        byte[] b2 = new byte[b1.length + message.length];
        System.arraycopy(b1, 0, b2, 0, b1.length);
        System.arraycopy(message, 0, b2, b1.length, message.length);
        message = b2;
      } else {
        message = b1;
      }
    }
  }
  ....
}

The check message != null && message.length > 0 in line 302 is redundant. Before the check in line 302, the following check is performed:

if (message == null || message.length == 0) {
  return NEED_MORE_INPUT;
}

If the condition of the check doesn't meet, we'll know that message isn't null and its length isn't equal to 0. It follows from this that its length is more than 0 (as a string's length cannot be a negative number). The local variable message isn't assigned any value before line 302, which means in line 302 the value of the message variable is the same as in the code above. It may be concluded that the expression message != null && message.length > 0 will always be true, but the code in the else block will never be executed.

Setting the Value of an Uninitialized Reference Field

V6007 Expression '!shouldExport()' is always false. ServiceConfig.java(371)

public synchronized void export() {
  checkAndUpdateSubConfigs();

  if (!shouldExport()) {                                 // <=
    return;
  }

  if (shouldDelay()) {
    ....
  } else {
    doExport();
}

The shouldExport method of the ServiceConfig class calls the getExport method, defined in the same class.

private boolean shouldExport() {
  Boolean export = getExport();
  // default value is true
  return export == null ? true : export; 
}
....
@Override
public Boolean getExport() {
  return (export == null && provider != null) ? provider.getExport() : export;
}

The getExport method calls the getExport method of the abstract AbstractServiceConfig class, which returns the value of the export field of the Boolean type. There is also a setExport method to set the field value.

protected Boolean export;
....
public Boolean getExport() {
  return export;
}
....
public void setExport(Boolean export) {
  this.export = export;
}

The export field is set in the code only by the setExport method. The setExport method is called only in the build method of the abstract AbstractServiceBuilder class (extending AbstractServiceConfig) only in case if the field isn't null.

@Override
public void build(T instance) {
....
  if (export != null) {
    instance.setExport(export);
  }
....
}

Due to the fact that all reference fields by default are initialized by null and the export field wasn't assigned a value, the setExport method will never be called.

As a result, the getExport method of the ServiceConfig class, expanding the AbstractServiceConfig class will always return null. The return value is used in the shouldExport method of the ServiceConfig class, therefore the shouldExport method will always return true. Because of returning true, the value of the !shouldExport() expression will always be false. It turns out, that the export field of the ServiceConfig class will never be returned until the following code is executed:

if (shouldDelay()) {
  DELAY_EXPORT_EXECUTOR.schedule(this::doExport, getDelay(), ....);
} else {
  doExport();
}

Non-negative Parameter's Value

V6009 The 'substring' function could receive the '-1' value while non-negative value is expected. Inspect argument: 2. AbstractEtcdClient.java(169)

protected void createParentIfAbsent(String fixedPath) {
  int i = fixedPath.lastIndexOf('/');
  if (i > 0) {
    String parentPath = fixedPath.substring(0, i);
    if (categories.stream().anyMatch(c -> fixedPath.endsWith(c))) {
      if (!checkExists(parentPath)) {
        this.doCreatePersistent(parentPath);
      }
    } else if (categories.stream().anyMatch(c -> parentPath.endsWith(c))) {
      String grandfather = parentPath
        .substring(0, parentPath.lastIndexOf('/'));            // <=
        if (!checkExists(grandfather)) {
          this.doCreatePersistent(grandfather);
        }
    }
  }
}

The result of the lastIndexOf function is passed as the second parameter to the substring function, whose second parameter mustn't be a negative number, even though lastIndexOf can return -1 if it doesn't find the sought-for value in the string. If the second parameter of the substring method is less than the first (-1 < 0), the exception StringIndexOutOfBoundsException will be thrown. To fix the error, we need to check the result, returned by the lastIndexOf function. If it's correct (at least, not negative), pass it to the substring function.

Unused Loop Counter

V6016 Suspicious access to element of 'types' object by a constant index inside a loop. RpcUtils.java(153)

public static Class<?>[] getParameterTypes(Invocation invocation) {
  if ($INVOKE.equals(invocation.getMethodName())
            && invocation.getArguments() != null
            && invocation.getArguments().length > 1
            && invocation.getArguments()[1] instanceof String[]) {
    String[] types = (String[]) invocation.getArguments()[1];
    if (types == null) {
      return new Class<?>[0];
    }
    Class<?>[] parameterTypes = new Class<?>[types.length];
    for (int i = 0; i < types.length; i++) {
      parameterTypes[i] = ReflectUtils.forName(types[0]);   // <= 
    }
    return parameterTypes;
  }
  return invocation.getParameterTypes();
}

In the for loop, a 0 constant index is used to access the element of the array types. It may have been meant to use the i variable as an index for accessing the array elements. But authors have left that out.

Pointless Do-While

V6019 Unreachable code detected. It is possible that an error is present. GrizzlyCodecAdapter.java(136)

@Override
public NextAction handleRead(FilterChainContext context) throws IOException {
  .... 
  do {
    savedReadIndex = frame.readerIndex();
    try {
      msg = codec.decode(channel, frame);
    } catch (Exception e) {
      previousData = ChannelBuffers.EMPTY_BUFFER;
      throw new IOException(e.getMessage(), e);
    }
    if (msg == Codec2.DecodeResult.NEED_MORE_INPUT) {
      frame.readerIndex(savedReadIndex);
        return context.getStopAction();
    } else {
      if (savedReadIndex == frame.readerIndex()) {
        previousData = ChannelBuffers.EMPTY_BUFFER;
        throw new IOException("Decode without read data.");
      }
      if (msg != null) {
        context.setMessage(msg);
        return context.getInvokeAction();
      } else {
        return context.getInvokeAction();
      }
    }
  } while (frame.readable());                                    // <=
  ....
}

The expression in the condition of the loop do — while (frame.readable()) is unreachable code, as the method exits during the first iteration of the loop. Several checks of the msg variable are performed in the body of the loop using if-else. In doing so, both in if and else values are returned from the method or exceptions are thrown. That's the reason why the body of loop executes only once, so usage of this loop has no point.

Copy-Paste in Switch

V6067 Two or more case-branches perform the same actions. JVMUtil.java(67), JVMUtil.java(71)

private static String getThreadDumpString(ThreadInfo threadInfo) {
  ....
  if (i == 0 && threadInfo.getLockInfo() != null) {
    Thread.State ts = threadInfo.getThreadState();
    switch (ts) {
      case BLOCKED:
        sb.append("t-  blocked on " + threadInfo.getLockInfo());
        sb.append('n');
        break;
      case WAITING:                                               // <=
        sb.append("t-  waiting on " + threadInfo.getLockInfo()); // <=
        sb.append('n');                                          // <=
        break;                                                    // <=
      case TIMED_WAITING:                                         // <=
        sb.append("t-  waiting on " + threadInfo.getLockInfo()); // <=
        sb.append('n');                                          // <=
        break;                                                    // <=
      default:
    }
  }
  ....
}

The code in switch for cases WAITING and TIMED_WAITING contains copy-paste code. If the same actions have to be performed, the code can be simplified by removing the content of the WAITING case block. As a result, the same code will be executed for WAITING and TIMED_WAITING.

Conclusion

Anyone interested in using RPC in Java, has probably heard about Apache Dubbo. It's a popular open source project with a long story and code, written by many developers. This project's code is of great quality, still the PVS-Studio static code analyzer managed to find a certain number of errors. This leads to the fact that static analysis is very important when developing middle and large sized projects, no matter how perfect your code is.

Note. Such one-time checks demonstrate capabilities of a static code analyzer, but represent a completely wrong way of its usage. More details of this idea are outlined here and here. Use the analysis regularly!

Thanks to Apache Dubbo developers for such a wonderful tool. I hope this article will help you improve the code. The article doesn't describe all suspicious pieces of code, so it's best for developers to check the project themselves and evaluate the results.

Автор: ValeryKomarov

Источник