About PVS-Studio

Apache Dubbo: What It Is and Main Features

About the Analysis

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.

Sign Byte in Java

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

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

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

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

Setting the Value of an Uninitialized Reference Field

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

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

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

@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

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('

'); break; case WAITING: // <= sb.append("\t- waiting on " + threadInfo.getLockInfo()); // <= sb.append('

'); // <= break; // <= case TIMED_WAITING: // <= sb.append("\t- waiting on " + threadInfo.getLockInfo()); // <= sb.append('

'); // <= break; // <= default: } } .... }

Conclusion

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.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.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.The actions sequence of the analysis is quite simple and didn't require much time in my case: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. V6007 Expression 'endKey[i] < 0xff' is always true. OptionUtil.java(32)A value of thetype (the value from -128 to 127) is compared with the value 0xff (255). In this condition, it's not taken into account that thetype in Java is signed, therefore the condition will always be true, which means it's meaningless. V6007 Expression 'isPreferIPV6Address()' is always false. NetUtils.java(236)The methodThemethod returnsin both cases. Most likely, a developer wanted one case to returnotherwise the method doesn't make any sense. V6007 Expression '!mask[i].equals(ipAddress[i])' is always true. NetUtils.java(476)In the condition if-else-if, the checkis performed. If the condition isn't met, the next check in if-else-if occurs which shows us thatandaren't equal. But in one of the following checks in if-else-if it's checked thatandaren't equal. Sinceand] 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)The checkin line 302 is redundant. Before the check in line 302, the following check is performed:If the condition of the check doesn't meet, we'll know thatisn'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 variableisn't assigned any value before line 302, which means in line 302 the value of thevariable is the same as in the code above. It may be concluded that the expressionwill always be true, but the code in the else block will never be executed. V6007 Expression '!shouldExport()' is always false. ServiceConfig.java(371)Themethod of theclass calls themethod, defined in the same class.Themethod calls themethod of the abstractclass, which returns the value of thefield of thetype. There is also amethod to set the field value.The export field is set in the code only by themethod. Themethod is called only in the build method of the abstractclass (extending) only in case if the field isn't null.Due to the fact that all reference fields by default are initialized byand thefield wasn't assigned a value, themethod will never be called.As a result, themethod of theclass, expanding theclass will always return. The return value is used in themethod of theclass, therefore themethod will always return. Because of returning true, the value of theexpression will always be false. It turns out, that thefield of theclass will never be returned until the following code is executed: V6009 The 'substring' function could receive the '-1' value while non-negative value is expected. Inspect argument: 2. AbstractEtcdClient.java(169)The result of thefunction is passed as the second parameter to thefunction, whose second parameter mustn't be a negative number, even thoughcan returnif it doesn't find the sought-for value in the string. If the second parameter of themethod is less than the first (-1 < 0), the exceptionwill be thrown. To fix the error, we need to check the result, returned by thefunction. If it's correct (at least, not negative), pass it to thefunction. V6016 Suspicious access to element of 'types' object by a constant index inside a loop. RpcUtils.java(153)In theloop, aconstant index is used to access the element of the array. It may have been meant to use thevariable as an index for accessing the array elements. But authors have left that out. V6019 Unreachable code detected. It is possible that an error is present. GrizzlyCodecAdapter.java(136)The expression in the condition of the loopis unreachable code, as the method exits during the first iteration of the loop. Several checks of thevariable are performed in the body of the loop using if-else. In doing so, both inandvalues 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. V6067 Two or more case-branches perform the same actions. JVMUtil.java(67), JVMUtil.java(71)The code infor casesandcontains copy-paste code. If the same actions have to be performed, the code can be simplified by removing the content of theblock. As a result, the same code will be executed forandAnyone 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.