Análisis del código fuente RPC del framework Apache Dubbo con el analizador estático PVS-Studio

Apache Dubbo es uno de los proyectos Java más populares en GitHub. Y esto no es sorprendente. Fue creado hace 8 años y es ampliamente utilizado como un entorno RPC de alto rendimiento. Por supuesto, la mayoría de los errores en su código se han corregido durante mucho tiempo y la calidad del código se mantiene a un alto nivel. Sin embargo, no hay razón para negarse a verificar un proyecto tan interesante utilizando el analizador de código estático PVS-Studio. Veamos qué logramos encontrar.

Sobre PVS-Studio

El analizador de código estático PVS-Studio ha existido durante más de 10 años en el mercado de TI y es una solución de software multifuncional y fácil de implementar. Por el momento, el analizador admite C, C ++, C #, lenguajes Java y funciona en plataformas Windows, Linux y macOS.

PVS-Studio es una solución B2B pagada y es utilizada por una gran cantidad de equipos en varias compañías. Si desea evaluar las capacidades del analizador, descargue el kit de distribución y solicite una clave de prueba aquí .

También hay opciones para usar PVS-Studio gratis en proyectos de código abierto o si eres un estudiante.

Apache Dubbo: ¿qué es y qué come?

Actualmente, casi todos los grandes sistemas de software están distribuidos . Si en un sistema distribuido hay una conexión interactiva entre componentes remotos con un tiempo de respuesta corto y una cantidad relativamente pequeña de datos transmitidos, esta es una buena razón para usar el entorno RPC (llamada a procedimiento remoto).

Apache Dubbo es un entorno RPC (llamada a procedimiento remoto) basado en Java de código abierto y alto rendimiento. Al igual que con muchos sistemas RPC, dubbo se basa en la idea de crear un servicio para definir métodos que se puedan llamar de forma remota con sus parámetros y tipos de datos de retorno. En el lado del servidor, se implementa una interfaz y se inicia el servidor dubbo para procesar las llamadas de los clientes. Hay un trozo en el lado del cliente que proporciona los mismos métodos que el servidor. Dubbo ofrece tres características clave que incluyen llamadas remotas frontales, tolerancia a fallas y equilibrio de carga, así como registro automático y descubrimiento de servicios.

Sobre el análisis

La secuencia de pasos para el análisis es bastante simple y no requiere mucho tiempo:

• Tengo Apache Dubbo con GitHub ;
• Usé las instrucciones para iniciar el analizador Java y comencé el análisis;
• Recibí un informe del analizador, lo analicé y destaqué casos interesantes.

Resultados del análisis: se emitieron 73 advertencias de los niveles de confianza Alto y Medio (46 y 27, respectivamente) para más de 4000 archivos, lo que es un buen indicador de la calidad del código.

No todas las advertencias son errores. Esta es una situación normal y, antes del uso regular del analizador, se requiere su configuración. Entonces podemos esperar un porcentaje bastante bajo de falsos positivos ( ejemplo ).

Entre las advertencias, no se consideraron 9 advertencias (7 altas y 2 medias) por archivos de prueba.

Como resultado, se mantuvo una pequeña cantidad de advertencias, pero entre ellas también hubo falsos positivos, ya que no configuré el analizador. Ordenar 73 advertencias en un artículo es una tarea larga, tonta y tediosa, por lo que se seleccionaron las más interesantes.

Byte firmado en Java

V6007 La expresión 'endKey [i] <0xff' siempre es verdadera. OpciónUtil.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); } 

Un valor de tipo byte (un valor de -128 a 127) se compara con un valor de 0xff (255). En esta condición, no se tiene en cuenta que el tipo de byte en Java es significativo, por lo tanto, la condición siempre se cumplirá, lo que significa que no tiene sentido.

Devuelve el mismo valor

V6007 La expresión 'isPreferIPV6Address ()' siempre es falsa. 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(); } 

IsPreferIPV6 Método de dirección.

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

El método isPreferIPV6Address devuelve falso en ambos casos, lo más probable es que uno de los casos debería haber devuelto verdadero según lo previsto por el programador, de lo contrario, el método no tiene sentido.

Verificaciones sin sentido

V6007 La expresión '! Máscara [i] .equals (ipAddress [i])' siempre es verdadera. 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; } 

En la primera de las condiciones if-else-if, se produce una comprobación "*". Igual (máscara [i]) || mask [i] .equals (ipAddress [i]) . Si no se cumple esta condición, el código pasa a la siguiente verificación en if-else-if, y nos damos cuenta de que mask [i] e ipAddress [i] no son equivalentes. Pero una de las siguientes comprobaciones en if-else-if solo comprueba que mask [i] e ipAddress [i] no son equivalentes. Como la máscara [i] y ipAddress [i] no tienen asignado ningún valor en el código del método, la segunda verificación no tiene sentido.

V6007 La expresión 'message.length> 0' siempre es verdadera. DeprecatedTelnetCodec.java (302)

V6007 Expresión 'mensaje! = Nulo' siempre es verdadero. 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; } } } .... } 

Comprobando mensaje! = Nulo && mensaje.length> 0 en la línea 302 no tiene sentido. Antes de la verificación, la línea 302 verifica:

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

Si no se cumple la condición de verificación, sabremos que el mensaje no es nulo y su longitud no es 0. De esta información se deduce que su longitud es mayor que cero (ya que la longitud de la cadena no puede ser un número negativo). El mensaje de la variable local antes de la línea 302 no tiene asignado ningún valor, lo que significa que en la línea 302 se usa el valor de la variable del mensaje , como en el código anterior. De todo esto podemos concluir que la expresión message! = Null && message.length> 0 siempre será verdadera , lo que significa que el código en el bloque else nunca se ejecutará.

Establecer el valor de un campo de referencia no inicializado

V6007 La expresión '! ShouldExport ()' siempre es falsa. ServiceConfig.java (371)

 public synchronized void export() { checkAndUpdateSubConfigs(); if (!shouldExport()) { // <= return; } if (shouldDelay()) { .... } else { doExport(); } 

El método shouldExport de la clase ServiceConfig llama al método getExport definido en la misma clase.

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

El método getExport llama al método getExport de la clase abstracta AbstractServiceConfig , que devuelve el valor del campo de exportación de tipo Boolean . También hay un método setExport para establecer el valor de un campo.

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

El campo de exportación en el código se establece solo mediante el método setExport . El método setExport se llama solo en el método de compilación de la clase abstracta AbstractServiceBuilder (que extiende AbstractServiceConfig ) y solo si el campo de exportación no es nulo.

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

Debido al hecho de que todos los campos de referencia se inicializan como nulos de forma predeterminada y que al campo de exportación no se le ha asignado ningún valor en ninguna parte del código, nunca se llamará al método setExport .

Como resultado, el método getExport de la clase ServiceConfig que extiende la clase AbstractServiceConfig siempre devolverá nulo . El valor devuelto se utiliza en el método shouldExport de la clase ServiceConfig , por lo que el método shouldExport siempre devolverá verdadero . Debido a que devuelve verdadero, el valor de la expresión ! ShouldExport () siempre será falso. Resulta que nunca habrá un retorno del método de exportación de la clase ServiceConfig antes de la ejecución del código:

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

Valor de parámetro no negativo

V6009 La función 'subcadena' podría recibir el valor '-1' mientras se espera un valor no negativo. Inspeccionar argumento: 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); } } } } 

El resultado de la función lastIndexOf se pasa por el segundo parámetro a la función de subcadena , cuyo segundo parámetro no debe ser un número negativo, aunque lastIndexOf puede devolver -1 si no encuentra el valor en la cadena. Si el segundo parámetro del método de subcadena es menor que el primero (-1 <0), se generará una StringIndexOutOfBoundsException . Para corregir el error, debe verificar el resultado devuelto por la función lastIndexOf , y si es correcto (al menos no negativo), luego pasarlo a la función de subcadena .

Contador de ciclos no utilizado

V6016 Acceso sospechoso al elemento del objeto 'tipos' por un índice constante dentro de un bucle. 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(); } 

El bucle for utiliza el índice constante 0 para referirse al elemento de la matriz de tipos . Quizás estaba destinado a usar la variable i como índice para acceder a los elementos de la matriz, pero no pasaron por alto, como dicen.

Hacer tiempo sin sentido

V6019 Código inalcanzable detectado. Es posible que haya un error presente. 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()); // <= .... } 

La expresión en la condición de bucle do - while (frame.readable ()) es un código inalcanzable, ya que la primera iteración del bucle siempre sale del método. En el cuerpo del bucle, se realizan varias comprobaciones de la variable msg utilizando if-else, y tanto en if como en else siempre devuelven un valor del método o arrojan una excepción. Es por esto que el cuerpo del ciclo se ejecutará solo una vez, como resultado, usar el ciclo do-while no tiene sentido.

Copiar y pegar en el interruptor

V6067 Dos o más ramificaciones de casos realizan las mismas acciones. 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: } } .... } 

El código de cambio para WAITING y TIMED_WAITING contiene un código de copiar y pegar. Si realmente necesita hacer lo mismo, puede simplificar el código eliminando el contenido en el bloque de casos para WAITING . Como resultado, el mismo código registrado en una sola copia se ejecutará para WAITING y TIMED_WAITING .

Conclusión

Cualquier persona interesada en usar RPC en Java probablemente haya oído hablar de Apache Dubbo. Este es un proyecto de código abierto popular con una larga historia y código escrito por muchos desarrolladores. El código para este proyecto es de excelente calidad, pero el analizador estático PVS-Studio logró encontrar una serie de errores. De esto podemos concluir que el análisis estático es muy importante al desarrollar proyectos medianos y grandes, no importa cuán perfecto sea su código.

Nota Estas comprobaciones únicas demuestran las capacidades de un analizador de código estático, pero son una forma completamente incorrecta de usarlo. Esta idea se presenta con más detalle aquí y aquí . ¡Usa el análisis regularmente!

Gracias a los desarrolladores de Apache Dubbo por esta gran herramienta. Espero que este artículo te ayude a mejorar el código. El artículo no describe todas las secciones sospechosas del código, por lo que es mejor que los desarrolladores verifiquen el proyecto por su cuenta y evalúen los resultados.

Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Valery Komarov. Análisis del marco Apache Dubbo RPC por el analizador de código estático PVS-Studio .