El equipo de PVS-Studio ha mantenido el blog sobre los controles de proyectos de código abierto por el analizador de código estático del mismo nombre durante muchos años. Hasta la fecha, se han verificado más de 300 proyectos, la base de errores contiene más de 12000 casos. Inicialmente, el analizador se implementó para verificar el código C y C ++, luego se agregó soporte de C #. Por lo tanto, de todos los proyectos verificados, la mayoría (> 80%) representa C y C ++. Recientemente, se agregó Java a la lista de lenguajes compatibles, lo que significa que ahora hay un mundo abierto completamente nuevo para PVS-Studio, por lo que es hora de complementar la base con errores de proyectos Java.
El mundo de Java es vasto y variado, por lo que uno ni siquiera sabe dónde buscar primero al elegir un proyecto para probar el nuevo analizador. Finalmente, la elección recayó en el motor de búsqueda y análisis analítico de texto completo Elasticsearch. Es un proyecto bastante exitoso, e incluso es especialmente agradable encontrar errores en proyectos importantes. Entonces, ¿qué defectos logró detectar PVS-Studio para Java? Se hablará más sobre los resultados de la verificación.
Brevemente sobre elasticsearch
Elasticsearch es un motor de búsqueda y análisis RESTful distribuido con código fuente abierto, capaz de resolver un número creciente de casos de uso. Le permite almacenar grandes cantidades de datos, realizar una búsqueda rápida y análisis (casi en modo de tiempo real). Por lo general, se utiliza como mecanismo / tecnología subyacente, que proporciona aplicaciones con funciones complejas y requisitos de búsqueda.
Entre los principales sitios que utilizan Elasticsearch se encuentran Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox.
Bien, suficiente de introducción.
Toda la historia de cómo eran las cosas.
No hubo problemas con el cheque en sí. La secuencia de acciones es bastante simple y no tomó mucho tiempo:
- Elasticsearch descargado de GitHub ;
- Seguí las instrucciones de cómo ejecutar el analizador Java y ejecuté el análisis;
- Recibió el informe del analizador, profundizó en él y señaló casos interesantes.
Ahora pasemos al punto principal.
Cuidado Posible NullPointerException
V6008 Desreferencia nula de 'línea'. GoogleCloudStorageFixture.java (451)
private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { ....
El error en este fragmento de código es que si no se leyó la cadena del búfer, la llamada del método
beginWith en la condición de la instrucción
if dará como resultado la excepción
NullPointerException . Lo más probable es que sea un error tipográfico y, al escribir una condición, los desarrolladores se
referían al operador
&& en lugar de
|| .
V6008 Desreferencia potencial nula de 'followIndexMetadata'. TransportResumeFollowAction.java (171), TransportResumeFollowAction.java (170), TransportResumeFollowAction.java (194)
void start( ResumeFollowAction.Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ....) throws IOException { MapperService mapperService = followIndexMetadata != null
Otra advertencia del diagnóstico
V6008 . El objeto
followIndexMetadata despertó mi interés. El método de
inicio acepta varios argumentos como entrada, nuestro sospechoso tiene razón entre ellos. Después de eso, en función de la comprobación de nuestro objeto para
nulo, se crea un nuevo objeto, que participa en la lógica del método adicional. Verificar
nulo nos muestra que
followIndexMetadata aún puede venir del exterior como un objeto nulo. Bueno, veamos más allá.
Luego, se envían múltiples argumentos al método de
validación (de nuevo, existe nuestro objeto considerado entre ellos). Si observamos la implementación del método de validación, todo encaja. Nuestro potencial objeto nulo se pasa al método de
validación como un tercer argumento, donde se desreferencia incondicionalmente. Posible
NullPointerException como resultado.
static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex,
No sabemos con certeza con qué argumentos se llama el método de
inicio . Es muy posible que todos los argumentos se verifiquen en algún lugar antes de llamar al método y que no ocurra ninguna desreferencia de objeto nulo. De todos modos, debemos admitir que dicha implementación de código todavía parece poco confiable y merece atención.
V6060 La referencia 'nodo' se utilizó antes de que se verificara contra nulo. RestTasksAction.java (152), RestTasksAction.java (151)
private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); ....
Aquí se activa otra regla de diagnóstico con el mismo problema.
NullPointerException . La regla grita a los desarrolladores: "Chicos, ¿qué están haciendo? ¿Cómo pudiste hacer eso? ¡Oh, es horrible! ¿Por qué usa primero el objeto y verifica si es
nulo en la siguiente línea? Así es como ocurre la desreferencia de objeto nulo. Por desgracia, incluso el comentario de un desarrollador no ayudó.
V6060 La referencia de 'causa' se utilizó antes de que se verificara como nula. StartupException.java (76), StartupException.java (73)
private void printStackTrace(Consumer<String> consumer) { Throwable originalCause = getCause(); Throwable cause = originalCause; if (cause instanceof CreationException) { cause = getFirstGuiceCause((CreationException)cause); } String message = cause.toString();
En este caso, debemos tener en cuenta que el método
getCause de la clase
Throwable podría devolver
nulo . El problema anterior se repite aún más, por lo que su explicación es innecesaria.
Condiciones sin sentido
V6007 La expresión 's.charAt (i)! =' \ T '' siempre es verdadera. Cron.java (1223)
private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) {
La función considerada devuelve el índice del primer carácter de espacio, comenzando desde el índice
i . Que pasa Tenemos el analizador advirtiendo que
s.charAt (i)! = '\ T' siempre es verdadero, lo que significa que la expresión
(s.charAt (i)! = '' || s.charAt (i)! = '\ T ') siempre será cierto también. ¿Es esto cierto? Creo que puedes asegurarte de esto fácilmente sustituyendo cualquier personaje.
Como resultado, este método siempre devolverá el índice, igual a
s.length () , lo cual es incorrecto. Me aventuraría a sugerir que el método anterior es el culpable aquí:
private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) {
Los desarrolladores implementaron el método, luego copiaron y obtuvieron nuestro método erróneo
findNextWhiteSpace, después de haber realizado algunas ediciones. Siguieron arreglando y arreglando el método pero no lo han arreglado. Para hacerlo bien, uno debe usar el operador
&& en lugar de
|| .
V6007 La expresión 'restante == 0' siempre es falsa. PemUtils.java (439)
private static byte[] generateOpenSslKey(char[] password, byte[] salt, int keyLength) { .... int copied = 0; int remaining; while (copied < keyLength) { remaining = keyLength - copied; .... copied += bytesToCopy; if (remaining == 0) {
Por la condición del bucle
<keyLength copiado podemos notar que la
copia siempre será menor que
keyLength . Por lo tanto, no tiene sentido comparar la variable restante con 0, y siempre será falsa, en cuyo punto el ciclo no saldrá por una condición. ¿Eliminar el código o reconsiderar la lógica de comportamiento? Creo que solo los desarrolladores podrán poner todos los puntos sobre la i.
V6007 La expresión 'healthCheckDn.indexOf (' = ')> 0' siempre es falsa. ActiveDirectorySessionFactory.java (73)
ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(...., () -> { if (....) { final String healthCheckDn = ....; if (healthCheckDn.isEmpty() && healthCheckDn.indexOf('=') > 0) { return healthCheckDn; } } return ....; }, ....); .... }
Expresión sin sentido de nuevo. Según la condición, la cadena
healthCheckDn debe estar vacía y contener el carácter '=' no en la primera posición, de modo que la expresión lambda devuelva la variable de cadena
healthCheckDn . ¡Uf, eso es todo! Como probablemente entendiste, es imposible. No vamos a profundizar en el código, dejémoslo a discreción de los desarrolladores.
Cité solo algunos de los ejemplos erróneos, pero más allá de eso había muchos
disparos de diagnóstico
V6007 , que deberían considerarse uno por uno, sacando conclusiones relevantes.
Pequeño método puede recorrer un largo camino
private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; }
Así que aquí tenemos un método diminuto de varias líneas. Pero los errores están en el reloj! El análisis de este método dio el siguiente resultado:
- V6007 La expresión '(int) x <0' siempre es falsa. BCrypt.java (429)
- V6025 Posiblemente el índice '(int) x' está fuera de límites. BCrypt.java (431)
Problema N1. La expresión
(int) x <0 siempre es falsa (Sí,
V6007 nuevamente). La variable
x no puede ser negativa, ya que es del tipo
char . El tipo
char es un entero sin signo. No se puede llamar un error real, pero, no obstante, la verificación es redundante y se puede eliminar.
Problema N2. Posible índice de matriz fuera de límites, lo que resulta en la excepción
ArrayIndexOutOfBoundsException . Luego surge la pregunta, que se encuentra en la superficie: "Espera, ¿qué tal la verificación del índice?"
Entonces, tenemos una matriz de tamaño fijo de 128 elementos:
private static final byte index_64[] = { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, -1, -1, -1, -1, -1, -1, -1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, -1, -1, -1, -1, -1, -1, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, -1, -1, -1, -1, -1 };
Cuando el método
char64 recibe la variable
x , se verifica la validez del índice. ¿Dónde está la falla? ¿Por qué todavía es posible el índice de matriz fuera de los límites?
El check (int) x> index_64.length no es del todo correcto. Si el método char64 recibe x con el valor 128, la comprobación no protegerá contra ArrayIndexOutOfBoundsException. Quizás esto nunca suceda en la realidad. Sin embargo, la verificación se escribe incorrectamente, y uno tiene que cambiar el operador "mayor que" (>) con "mayor o igual que (> =).
Comparaciones, que hicieron su mejor esfuerzo
Los números
V6013 'displaySize' y 'that.displaySize' se comparan por referencia. Posiblemente se pretendía una comparación de igualdad. ColumnInfo.java (122)
.... private final String table; private final String name; private final String esType; private final Integer displaySize; .... @Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } ColumnInfo that = (ColumnInfo) o; return displaySize == that.displaySize &&
Lo incorrecto aquí es que los objetos
displaySize del tipo
Integer se comparan utilizando el operador
== , es decir, por referencia. Es muy posible que se
comparen los objetos
ColumnInfo , cuyos campos
displaySize tienen referencias diferentes pero el mismo contenido. En este caso, la comparación nos dará el resultado negativo, cuando esperábamos que fuera cierto.
Me aventuraría a adivinar que tal comparación podría ser el resultado de una refactorización fallida e inicialmente el campo
displaySize era del tipo
int .
V6058 La función 'igual' compara objetos de tipos incompatibles: Integer, TimeValue. DatafeedUpdate.java (375)
.... private final TimeValue queryDelay; private final TimeValue frequency; .... private final Integer scrollSize; .... boolean isNoop(DatafeedConfig datafeed) { return (frequency == null || Objects.equals(frequency, datafeed.getFrequency())) && (queryDelay == null || Objects.equals(queryDelay, datafeed.getQueryDelay())) && (scrollSize == null || Objects.equals(scrollSize, datafeed.getQueryDelay()))
Comparación de objetos incorrectos de nuevo. Esta vez se comparan objetos con tipos incompatibles (
Integer y
TimeValue ). El resultado de esta comparación es obvio, y siempre es falso. Puede ver que los campos de clase se comparan entre sí, solo se deben cambiar los nombres de los campos. Aquí está el punto: un desarrollador decidió acelerar el proceso mediante el uso de copiar y pegar y consiguió un error en el negocio. La clase implementa un captador para el campo
scrollSize , por lo que para corregir el error se debe usar el método
datafeed .getScrollSize ().Veamos un par de ejemplos erróneos sin ninguna explicación. Los problemas son bastante obvios.
V6001 Hay
subexpresiones idénticas 'takenInMillis' a la izquierda y a la derecha del operador '=='. TermVectorsResponse.java (152)
@Override public boolean equals(Object obj) { .... return index.equals(other.index) && type.equals(other.type) && Objects.equals(id, other.id) && docVersion == other.docVersion && found == other.found && tookInMillis == tookInMillis
V6009 La función 'igual' recibe un argumento extraño. Un objeto 'shardId.getIndexName ()' se usa como argumento para su propio método. SnapshotShardFailure.java (208)
@Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) &&
Misceláneo
V6006 El objeto fue creado pero no se está utilizando. La palabra clave 'throw' podría faltar. JdbcConnection.java (88)
@Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } }
El error es obvio y no requiere explicación. Un desarrollador creó una excepción, pero no la lanzó a ningún otro lado. Dicha excepción anónima se crea con éxito y, lo que es más importante, se eliminará sin problemas. La razón es la falta del operador de
lanzamiento .
V6003 Se
detectó el uso del
patrón 'if (A) {....} else if (A) {....}'. Hay una probabilidad de presencia de error lógico. MockScriptEngine.java (94), MockScriptEngine.java (105)
@Override public <T> T compile(....) { .... if (context.instanceClazz.equals(FieldScript.class)) { .... } else if (context.instanceClazz.equals(FieldScript.class)) { .... } else if(context.instanceClazz.equals(TermsSetQueryScript.class)) { .... } else if (context.instanceClazz.equals(NumberSortScript.class)) .... }
En caso
contrario, una de las condiciones se repite dos veces, por lo que la situación requiere una revisión de código competente.
V6039 Hay dos declaraciones 'if' con expresiones condicionales idénticas. La primera instrucción 'if' contiene el método return. Esto significa que la segunda declaración 'si' no tiene sentido. SearchAfterBuilder.java (94), SearchAfterBuilder.java (93)
public SearchAfterBuilder setSortValues(Object[] values) { .... for (int i = 0; i < values.length; i++) { if (values[i] == null) continue; if (values[i] instanceof String) continue; if (values[i] instanceof Text) continue; if (values[i] instanceof Long) continue; if (values[i] instanceof Integer) continue; if (values[i] instanceof Short) continue; if (values[i] instanceof Byte) continue; if (values[i] instanceof Double) continue; if (values[i] instanceof Float) continue; if (values[i] instanceof Boolean) continue;
La misma condición se usa dos veces seguidas. ¿Es la segunda condición superflua o debería usarse otro tipo en lugar de
booleano ?
V6009 La función 'subcadena' recibe argumentos extraños. El argumento 'queryStringIndex + 1' no debe ser mayor que 'queryStringLength'. LoggingAuditTrail.java (660)
LogEntryBuilder withRestUriAndMethod(RestRequest request) { final int queryStringIndex = request.uri().indexOf('?'); int queryStringLength = request.uri().indexOf('#'); if (queryStringLength < 0) { queryStringLength = request.uri().length(); } if (queryStringIndex < 0) { logEntry.with(....); } else { logEntry.with(....); } if (queryStringIndex > -1) { logEntry.with(...., request.uri().substring(queryStringIndex + 1,
Consideremos aquí el escenario erróneo que puede causar la excepción
StringIndexOutOfBoundsException . La excepción ocurrirá cuando
request.uri () devuelve una cadena que contiene el carácter '#' antes de '?'. No hay controles en el método, por lo que en caso de que ocurra, el problema se está gestando. Quizás, esto nunca sucederá debido a varias comprobaciones del objeto fuera del método, pero establecer esperanzas en esto no es la mejor idea.
Conclusión
Durante muchos años, PVS-Studio ha estado ayudando a encontrar defectos en el código de proyectos comerciales y de código abierto gratuitos. Recientemente, Java se ha unido a la lista de idiomas admitidos para el análisis. Elasticsearch se convirtió en una de las primeras pruebas para nuestro recién llegado. Esperamos que esta verificación sea útil para el proyecto e interesante para los lectores.
PVS-Studio para Java necesita nuevos desafíos, nuevos usuarios, comentarios activos y clientes para adaptarse rápidamente al nuevo mundo :). ¡Así que lo invito a
descargar y probar nuestro analizador en su proyecto de trabajo de inmediato!