Analysis of the RPC source code of the Apache Dubbo framework with the PVS-Studio static analyzer

Apache Dubbo is one of the most popular Java projects on GitHub. And this is not surprising. It was created 8 years ago and is widely used as a high-performance RPC environment. Of course, most of the errors in his code have long been fixed and the quality of the code is maintained at a high level. However, there is no reason to refuse to check such an interesting project using the PVS-Studio static code analyzer. Let's see what we managed to find.

About PVS-Studio

PVS-Studio static code analyzer has existed for more than 10 years in the IT market and is a multifunctional and easily implemented software solution. At the moment, the analyzer supports C, C ++, C #, Java languages ​​and works on Windows, Linux and macOS platforms.



PVS-Studio is a paid B2B solution and is used by a large number of teams in various companies. If you want to evaluate the capabilities of the analyzer, then download the distribution kit and request a trial key here .



There are also options to use PVS-Studio for free in open source projects or if you are a student.

Apache Dubbo: what is it and what does it eat?

Currently, almost all large software systems are distributed . If in a distributed system there is an interactive connection between remote components with a short response time and a relatively small amount of transmitted data, then this is a good reason to use the RPC (remote procedure call) environment.



Apache Dubbo is a high-performance, open-source Java-based RPC (remote procedure call) environment. As with many RPC systems, dubbo is based on the idea of ​​creating a service for defining methods that can be called remotely with their parameters and return data types. On the server side, an interface is implemented and the dubbo server is launched to process client calls. There is a stub on the client side that provides the same methods as the server. Dubbo offers three key features that include front-end remote calling, fault tolerance and load balancing, as well as automatic registration and discovery of services.

About analysis

The sequence of steps for the analysis is quite simple and does not require much time:

• Got Apache Dubbo with GitHub ;
• I used the instructions for starting the Java analyzer and started the analysis;
• I got the analyzer report, analyzed it and highlighted interesting cases.

Analysis results: 73 warnings of the High and Medium confidence level (46 and 27, respectively) were issued for 4000+ files, which is a good indicator of the quality of the code.



Not all warnings are errors. This is a normal situation, and before regular use of the analyzer, its configuration is required. Then we can expect a fairly low percentage of false positives ( example ).



Among the warnings, 9 warnings (7 High and 2 Medium) per test files were not considered.



As a result, a small number of warnings remained, but among them there were also false positives, since I did not configure the analyzer. To sort 73 warnings in an article is a long, silly and tedious task, so the most interesting ones were selected.

Signed 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 type byte (value from -128 to 127) is compared with a value of 0xff (255). In this condition, it is not taken into account that the byte type in Java is significant, therefore the condition will always be fulfilled, which means that it does not make sense.

Return the same value

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

IsPreferIPV6Address Method.

 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 that one of the cases should have returned true as intended by the programmer, otherwise the method does not make sense.

Meaningless 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 first of the if-else-if conditions, a "*" check occurs . Equals (mask [i]) || mask [i] .equals (ipAddress [i]) . If this condition is not met, the code proceeds to the next check in if-else-if, and we become aware that mask [i] and ipAddress [i] are not equivalent. But in one of the following checks in if-else-if it is just checked that mask [i] and ipAddress [i] are not equivalent. Since mask [i] and ipAddress [i] are not assigned any values ​​in the method code, the second check does not make sense.



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; } .... //   message  ! .... 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; } } } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Checking message! = Null && message.length> 0 on line 302 makes no sense. Before the check, line 302 checks:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

If the verification condition is not fulfilled, then we will know that message is not null and its length is not 0. It follows from this information that its length is greater than zero (since the length of the string cannot be a negative number). The local message variable is not assigned any values ​​to line 302, which means that line 302 uses the value of the message variable, as in the code above. From all this we can conclude that the expression message! = Null && message.length> 0 will always be true , which means that 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 AbstractServiceConfig abstract class, which returns the value of the export field of type Boolean . There is also a setExport method for setting the value of a field.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

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

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Due to the fact that all reference fields are initialized to null by default and that the export field has not been assigned any value anywhere in the code, the setExport method will never be called.



As a result, the getExport method of the ServiceConfig class extending the AbstractServiceConfig class will always return null . The returned value is used in the shouldExport method of the ServiceConfig class, so the shouldExport method will always return true . Due to returning true, the value of the expression ! ShouldExport () will always be false. It turns out that there will never be a return from the export method of the ServiceConfig class before code execution:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Non-negative parameter 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 by the second parameter to the substring function, the second parameter of which should not be a negative number, although lastIndexOf can return -1 if it does not find the value in the string. If the second parameter of the substring method is less than the first (-1 <0), then a StringIndexOutOfBoundsException will be thrown . To fix the error, you need to check the result returned by the lastIndexOf function, and if it is correct (at least not negative), then pass it to the substring function.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The for loop uses the constant index 0 to refer to the element of the types array. Perhaps it was meant to use the variable i as an index to access the elements of the array, but they did not overlook, as they say.

Meaningless 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 loop condition do - while (frame.readable ()) is unreachable code, since the first iteration of the loop always exits the method. In the body of the loop, several checks of the msg variable are performed using if-else, and both in if and in else always return a value from the method or throw an exception. It is because of this that the body of the cycle will be executed only once, as a result, using the do-while loop does not make sense.

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 switch code for WAITING and TIMED_WAITING contains a copy-paste code. If you really need to do the same things, you can simplify the code by deleting the contents in the case block for WAITING . As a result, the same code recorded in a single copy will be executed for WAITING and TIMED_WAITING .

Conclusion

Those interested in using RPC in Java have probably heard of Apache Dubbo. This is a popular open source project with a long history and code written by many developers. The code for this project is of excellent quality, but the PVS-Studio static analyzer managed to find a number of errors. From this we can conclude that static analysis is very important when developing medium and large projects, no matter how perfect your code is.



Note. Such one-time checks demonstrate the capabilities of a static code analyzer, but are a completely wrong way to use it. This idea is presented in more detail here and here . Use the analysis regularly!



Thanks to the Apache Dubbo developers for such a great tool. I hope this article helps you improve the code. The article describes not all the suspicious sections of code, so developers are better off checking the project on their own and evaluating the results.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Valery Komarov. Analysis of the Apache Dubbo RPC Framework by the PVS-Studio Static Code Analyzer .