# 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\.

![0663_ApacheDubbo/image1.png](https://import.viva64.com/docx/blog/0663_ApacheDubbo/image1.png)

## About PVS\-Studio

The [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) 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](https://pvs-studio.com/en/pvs-studio/download/)\.  

Also there are [options](https://pvs-studio.com/en/blog/posts/0614/) 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](https://computersciencewiki.org/index.php/Distributed_systems)\. 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](https://en.wikipedia.org/wiki/Remote_procedure_call) \(remote procedure call\) environment\.

Apache Dubbo is a high\-performance [RPC](https://en.wikipedia.org/wiki/Remote_procedure_call) \(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](https://github.com/apache/dubbo);
* Used start\-up Java analyzer [instructions](https://pvs-studio.com/en/docs/manual/6703/) 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](https://pvs-studio.com/en/blog/posts/cpp/0523/)\)\.

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](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'endKey\[i\] < 0xff' is always true\. OptionUtil\.java\(32\)

```cpp
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](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'isPreferIPV6Address\(\)' is always false\. NetUtils\.java\(236\)

```cpp
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_\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v6007/) Expression '\!mask\[i\]\.equals\(ipAddress\[i\]\)' is always true\. NetUtils\.java\(476\)

```cpp
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](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'message\.length \> 0' is always true\. DeprecatedTelnetCodec\.java\(302\)

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'message \!\= null' is always true\. DeprecatedTelnetCodec\.java\(302\)

```cpp
protected Object decode(.... , byte[] message) throws IOException {
  ....
  if (message == null || message.length == 0) {
    return NEED_MORE_INPUT;
  }
  ....
  // Here the variable message 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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v6007/) Expression '\!shouldExport\(\)' is always false\. ServiceConfig\.java\(371\)

```cpp
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\.

```cpp
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\.

```cpp
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\.

```cpp
@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: 

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

## Non\-negative Parameter's Value

[V6009](https://pvs-studio.com/en/docs/warnings/v6009/) The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. AbstractEtcdClient\.java\(169\)

```cpp
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](https://pvs-studio.com/en/docs/warnings/v6016/) Suspicious access to element of 'types' object by a constant index inside a loop\. RpcUtils\.java\(153\)

```cpp
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](https://pvs-studio.com/en/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. GrizzlyCodecAdapter\.java\(136\)

```cpp
@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](https://pvs-studio.com/en/docs/warnings/v6067/) Two or more case\-branches perform the same actions\. JVMUtil\.java\(67\), JVMUtil\.java\(71\)

```cpp
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](https://en.wikipedia.org/wiki/Remote_procedure_call) 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](https://pvs-studio.com/en/blog/posts/cpp/0594/) and [here](https://pvs-studio.com/en/blog/posts/cpp/0639/)\. 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\.