Lejos del primer año, el equipo de PVS-Studio ha estado blogueando sobre controles de proyectos de código abierto por el analizador de código estático del mismo nombre. Hasta la fecha, se han verificado más de 300 proyectos y se han escrito más de 12,000 casos en la base de datos de errores encontrados. Inicialmente, el analizador se implementó para probar el código C y C ++, luego apareció el soporte de lenguaje C #. Por lo tanto, entre los proyectos probados, la mayoría (> 80%) corresponde a C y C ++. Más recientemente, Java se ha agregado a los lenguajes compatibles, lo que significa que PVS-Studio abre las puertas a un nuevo mundo, y es hora de complementar la base de datos con errores de proyectos Java.
El mundo de Java es enorme y diverso, por lo que mis ojos se abren al elegir un proyecto para probar un nuevo analizador. En última instancia, la elección recayó en el motor de búsqueda de texto completo y el análisis Elasticsearch. Este es un proyecto bastante exitoso, y en proyectos exitosos, encontrar errores es doble, o incluso triple, más placentero. Entonces, ¿qué defectos detectó PVS-Studio para Java? El resultado de la verificación se discutirá en el artículo.
Conocimiento superficial con Elasticsearch
Elasticsearch es un motor de análisis y búsqueda de texto completo escalable de código abierto. Le permite almacenar grandes cantidades de datos, realizar entre ellos una búsqueda rápida y análisis (casi en tiempo real). Por lo general, se usa como mecanismo / tecnología subyacente que proporciona a las aplicaciones funciones complejas y requisitos de búsqueda.
Entre los principales sitios que utilizan Elasticsearch, Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox se mencionan.
Creo que con el conocido es suficiente.
Como fue
No hubo problemas con la verificación. La secuencia de acciones es bastante simple y no tomó mucho tiempo:
- Elasticsearch descargado de GitHub ;
- Utilicé las instrucciones para iniciar el analizador de Java e inicié el análisis;
- Recibí un informe del analizador, lo analicé y destaqué casos interesantes.
Ahora vamos al grano.
Precaución 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 podían leer la línea desde el búfer, llamar al método
beginWith en la condición de la
instrucción if arrojará una
NullPointerException . Lo más probable es que se trate de un error tipográfico y, al escribir la condición, se quiso decir el 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 . Ahora, una mirada más cercana ha clavado el objeto
followIndexMetadata . El método de
inicio toma varios argumentos de entrada, incluido nuestro sospechoso. Luego, sobre la base de verificar que nuestro objeto sea
nulo , se forma un nuevo objeto, que participa en la lógica adicional del método. La comprobación de
nulo nos dice que
followIndexMetadata aún puede provenir del exterior con un objeto nulo. Ok, mira más allá.
A continuación, se llama al método de validación empujando muchos argumentos (de nuevo, entre los cuales se encuentra el objeto en cuestión). Y si observa la implementación del método de validación, entonces todo encaja. Nuestro potencial objeto nulo se pasa por el tercer argumento al método de
validación , donde se desreferencia incondicionalmente. Como resultado, una potencial
NullPointerException. static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex,
No se sabe con qué argumentos se llama realmente el método de
inicio . Es posible que la comprobación de todos los argumentos se realice en algún lugar antes de la llamada al método, y no enfrentamos ninguna desreferenciación del objeto nulo. Pero, debe 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í funcionó otra regla de diagnóstico, pero el problema es el mismo:
NullPointerException . La regla dice: "Chicos, ¿qué están haciendo? ¿Cómo es eso? Oh problema! ¿Por qué usas el objeto primero y luego en la siguiente línea de código verifica que sea
nulo ? ” Entonces resulta que aquí se desreferencia un objeto nulo. Por desgracia, incluso el comentario de uno de los desarrolladores 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();
Cabe señalar aquí que el método
getCause de la clase
Throwable puede devolver
nulo . Además, el problema considerado anteriormente se repite y no tiene sentido explicar algo en detalle.
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 espacio, comenzando desde el índice
i . Que esta mal Tenemos una advertencia del analizador de que
s.charAt (i)! = '\ T' siempre
es verdadero, lo que significa que siempre habrá verdad y la expresión
(s.charAt (i)! = '' || s.charAt (i)! = '\ t') . Es asi? Creo que usted mismo puede verificar esto fácilmente sustituyendo cualquier personaje.
Como resultado, este método siempre devolverá un índice igual a
s.length () , lo cual no es cierto. Me atrevo a suponer que este método localizado ligeramente superior es el culpable:
private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) {
Implementamos este método, luego lo
copiamos e hicimos algunas correcciones menores para obtener nuestro método erróneo
findNextWhiteSpace . El método fue ajustado, ajustado, pero no ajustado. Para corregir la situación, 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 ciclo
copiado <keyLength, se puede observar que la
copia siempre será menor que
keyLength . Por lo tanto, una comparación sobre la igualdad de la variable
restante con 0 no tiene sentido y siempre dará un resultado falso, y por lo tanto, la condición no se saldrá del bucle. ¿Vale la pena eliminar este código o todavía es necesario reconsiderar la lógica del comportamiento? Creo que solo los desarrolladores podrán puntear todo 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 ....; }, ....); .... }
De nuevo una expresión sin sentido. Según la condición, para que la expresión lambda devuelva la variable de cadena
healthCheckDn , la cadena
healthCheckDn debe estar vacía y la cadena que contiene el carácter '=' no está en la primera posición. Fuh, más o menos resuelto. Y como entendiste correctamente, esto es imposible. No entenderemos la lógica del código, se lo dejamos a los desarrolladores.
Solo di algunos ejemplos erróneos, pero además de esto, hubo muchos casos en los que se
activaron los diagnósticos
V6007 , que deben considerarse por separado y sacar conclusiones.
El método es pequeño, pero udalenky
private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; }
Entonces, tenemos un pequeño método de varias líneas. ¡Pero los insectos no duermen! Un 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í, sí, nuevamente
V6007 ). La variable
x no puede ser negativa, ya que es de tipo
char . El tipo
char es un entero sin signo. Esto no se puede llamar un error real, pero, sin embargo, la verificación es redundante y se puede eliminar.
Problema N2. Posible desbordamiento de la matriz que conduce a una
ArrayIndexOutOfBoundsException . Entonces surge la pregunta, que yace en la superficie: "Espera, ¿qué hay de verificar el í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 la variable
x ingresa la entrada del método
char64 , verifica la validez del índice. ¿Dónde está la brecha? ¿Por qué todavía es posible salir de la matriz?
Marcar
(int) x> index_64.length no
es del todo correcto. Si
x con un valor de 128
llega a la entrada del método
char64 , entonces la comprobación no protegerá contra
ArrayIndexOutOfBoundsException . Quizás esto nunca suceda en la realidad. Sin embargo, la verificación se escribe incorrectamente y debe reemplazar el operador "mayor que" (>) con "mayor o igual que" (> =).
Comparaciones que intentaron
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 &&
La incorrección aquí es que los objetos
displaySize del tipo
Integer se comparan mediante el operador
== , es decir, se comparan por referencia. Es muy posible que se
comparen los objetos
ColumnInfo , para lo cual los campos
displaySize tienen enlaces diferentes pero el mismo contenido. Y en este caso, la comparación nos dará un resultado negativo, mientras esperábamos la verdad.
Me atrevería a sugerir que tal comparación podría ser el resultado de una refactorización fallida, e inicialmente el campo
displaySize era de 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()))
Y de nuevo la comparación incorrecta de objetos. Ahora compare objetos cuyos tipos son incompatibles (
Integer y
TimeValue ). El resultado de esta comparación es obvio, y siempre es falso. Se puede ver que los campos de la clase se comparan de la misma manera entre sí, solo es necesario cambiar los nombres de los campos. Entonces, el desarrollador decidió acelerar el proceso de escribir código con copiar y pegar, pero se adjudicó el mismo error. La clase implementa un captador para el campo
scrollSize , por lo que para corregir el error, la solución correcta sería utilizar el método apropiado:
datafeed .getScrollSize () .
Veamos un par de ejemplos más de errores sin ninguna explicación. El problema ya es obvio.
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. El desarrollador lanzó una excepción, pero de ninguna manera la lanza más lejos. Dicha excepción anónima se creará con éxito, y también con éxito y, lo más importante, se destruirá sin dejar rastro. La razón es la falta de una declaración 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 una
construcción if-else múltiple
, una de las condiciones se repite dos veces, por lo que la situación requiere una revisión competente del código.
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. La segunda condición es superflua, ¿o es necesario usar un tipo diferente 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,
Considere de inmediato un escenario de error que podría
generar una
StringIndexOutOfBoundsException . Se producirá una excepción cuando
request.uri () devuelve una cadena que contiene el carácter '#' anterior a '?'. Para tal caso, no hay controles en el método, y si esto todavía sucede, no se evitará el desastre. Quizás esto nunca suceda debido a varias comprobaciones del objeto de
solicitud fuera del método, pero en mi opinión, esperar que esto no sea la mejor idea.
Conclusión
Con los años, PVS-Studio ha ayudado a encontrar fallas en el código de proyectos comerciales y de código abierto gratuitos. Más recientemente, Java se ha agregado para admitir lenguajes analizados. Y una de las primeras pruebas para nuestro recién llegado fue el desarrollo activo de Elasticsearch. Esperamos que esta verificación sea útil para el proyecto e interesante para los lectores.
Para que PVS-Studio para Java se adapte rápidamente a un mundo nuevo por sí mismo, necesitamos nuevas pruebas, nuevos usuarios, comentarios activos y clientes :). Por lo tanto, sugiero, sin demora, ¡
descargue y pruebe nuestro analizador en su borrador de trabajo!
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Maxim Stefanov.
PVS-Studio para Java sale a la carretera. La siguiente parada es Elasticsearch