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

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.

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.

About the Analysis

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

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

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);
}

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();
}
static boolean isPreferIPV6Address() {
boolean preferIpv6 = Boolean.getBoolean("java.net.preferIPv6Addresses");
if (!preferIpv6) {
return false; // <=
}
return false; // <=
}

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;
}
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;
}
}
}
....
}
if (message == null || message.length == 0) {
return NEED_MORE_INPUT;
}

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();
}
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;
}
protected Boolean export;
....
public Boolean getExport() {
return export;
}
....
public void setExport(Boolean export) {
this.export = export;
}
@Override
public void build(T instance) {
....
if (export != null) {
instance.setExport(export);
}
....
}
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);
}
}
}
}

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();
}

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()); // <=
....
}

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:
}
}
....
}

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.

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

610 Followers

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.