El Año Nuevo se acerca inexorablemente y, por lo tanto, es hora de hacer balance. Continuando con la tradición, este año revisamos nuestros artículos sobre la verificación de proyectos Java del mundo del código abierto e hicimos una calificación de los 10 errores más interesantes.
Durante el año pasado, nosotros (el equipo de Java de PVS-Studio) resolvimos los errores de cinco proyectos de código abierto en nuestros artículos y hablamos bastante sobre nuestro funcionamiento interno:
- NSA, Ghidra y los unicornios
- PVS-Studio para Java bajo el capó: desarrollo de diagnósticos
- Comprobando el código de XMage y por qué no están disponibles las cartas raras especiales para la colección Dragon's Maze
- Comprobación de WildFly - Servidor de aplicaciones JavaEE
- Unicornios en su seguridad: examinando el código del castillo hinchable
- Big / Bug Data: Analizando el código fuente de Apache Flink
Sugerimos que el lector primero lea estos artículos y haga su propia calificación personal, luego la compare con la nuestra y diga que estamos equivocados :).
Décimo lugar: "Igualdad engañosa"
Fuente: Big / Bug Data: Analizando el
código fuente de Apache Flink V6001 Hay subexpresiones idénticas ' ProcessingData ' a la izquierda y a la derecha del operador '=='. CheckpointStatistics.java (229)
@Override
public boolean equals(Object o)
{
....
CheckpointStatistics that = (CheckpointStatistics) o;
return id == that.id &&
savepoint == that.savepoint &&
triggerTimestamp == that.triggerTimestamp &&
latestAckTimestamp == that.latestAckTimestamp &&
stateSize == that.stateSize &&
duration == that.duration &&
alignmentBuffered == that.alignmentBuffered &&
processedData == processedData && // <=
persistedData == that.persistedData &&
numSubtasks == that.numSubtasks &&
numAckSubtasks == that.numAckSubtasks &&
status == that.status &&
Objects.equals(checkpointType, that.checkpointType) &&
Objects.equals(
checkpointStatisticsPerTask,
that.checkpointStatisticsPerTask);
}
Un simple error y muy ofensivo debido a la falta de atención: la processedData campo se comparó a sí mismo. Debido a este error, la comparación de objetos de tipo CheckpointStatistics a veces dará falsos positivos. Pero el principal peligro de este error tipográfico es que equals se utiliza de forma muy activa en las colecciones, y la implementación incorrecta de este método puede conducir a un comportamiento muy extraño, que llevará mucho tiempo depurar.
Me gustaría señalar que es común que los desarrolladores cometan errores en las funciones de comparación. Un colega mío incluso escribió un largo artículo " Vidas malvadas en funciones de comparación " con muchos ejemplos y explicaciones.
Noveno lugar: "Código inalcanzable"
Fuente: Unicornios en su seguridad: examinando el código del castillo hinchable .
V6019 Se detectó un código inalcanzable. Es posible que exista un error. XMSSTest.java (170)
public void testSignSHA256CompleteEvenHeight2() {
....
int height = 10;
....
for (int i = 0; i < (1 << height); i++) {
byte[] signature = xmss.sign(new byte[1024]);
switch (i) {
case 0x005b:
assertEquals(signatures[0], Hex.toHexString(signature));
break;
case 0x0822:
assertEquals(signatures[1], Hex.toHexString(signature));
break;
....
}
}
}
La rama de conmutación para el valor i == 0x0822 (2082) resultó ser inalcanzable. ¿Como paso?
Si presta atención a la condición del bucle 1 << altura , donde la altura siempre es 10 , entonces todo encajará en su lugar a la vez. Según la condición del bucle, el contador i en el bucle for no puede ser superior a 1024 (1 << 10). Naturalmente, la ejecución de la rama de conmutación considerada nunca sucederá.
Octavo lugar: "Método anotado"
Fuente: Under the Hood PVS-Studio para Java: Desarrollo de diagnósticos .
La colección V6009 está vacía. La llamada de la función 'clara' no tiene sentido. MetricRepositoryRule.java (90)
protected void after()
{
this.metricsById.clear();
this.metricsById.clear();
}
Algunos de nuestros diagnósticos se basan en gran medida en el mecanismo de anotación de métodos. Las anotaciones proporcionan información adicional al analizador sobre los métodos utilizados, por ejemplo:
- ¿Es este un método limpio?
- ¿Cuáles son las restricciones a los argumentos,
- Resultado devuelto,
- ... y así.
El analizador deriva algunas anotaciones del código fuente en sí, algunas las agregamos manualmente (por ejemplo, para los métodos de la biblioteca estándar). La historia de este error comenzó con el hecho de que no anotamos completamente el método Map # clear. Después de que notamos y arreglamos esto, aparecieron nuevos desencadenantes en nuestros proyectos de prueba, entre los cuales estaba nuestro caso interesante.
A primera vista, volver a borrar el diccionario no es un error. E incluso pensaríamos que se trata de una cadena duplicada aleatoriamente si no prestamos atención a los campos de la clase:
private final Map<String, Metric> metricsByKey = new HashMap<>();
private final Map<Long, Metric> metricsById = new HashMap<>();
La clase tiene dos campos con nombres similares metricsById y metricsByKey . Esto sugiere que el autor del código quería borrar ambos diccionarios, pero ... esto no sucedió. Por lo tanto, los dos diccionarios que contienen datos relacionados no estarán sincronizados después de la llamada a after .
Séptimo lugar: "Expectativa / Realidad"
Fuente: Checking WildFly - Servidor de aplicaciones JavaEE .
V6058 La función 'igual' compara objetos de tipos incompatibles: String, ModelNode. JaxrsIntegrationProcessor.java (563)
// Send value to RESTEasy only if it's not null, empty string, or the
// default value.
private boolean isTransmittable(AttributeDefinition attribute,
ModelNode modelNode) {
if (modelNode == null || ModelType
.UNDEFINED.equals(modelNode.getType())) {
return false;
}
String value = modelNode.asString();
if ("".equals(value.trim())) {
return false;
}
return !value.equals(attribute.getDefaultValue()); // <=
}
Al notar el comentario que precede al método, puede esperar que el método devuelva verdadero si:
- modelNode no es nulo,
- la representación de cadena de modelNode no está vacía,
- modelNode no es el predeterminado.
A pesar del comentario del autor y, a primera vista, la lógica correcta, el comportamiento del método será diferente. Esto se debe a que se comprueba la igualdad de modelNode con el valor predeterminado en la última línea del método.
La representación de cadena de modelNode se compara con un objeto de tipo ModelNode y, como puede adivinar, dicha comparación siempre devolverá un resultado negativo debido a la incompatibilidad de tipos.
Consecuencias del error: Permiso inesperado para enviar el valor modelNode cuando es igual al valor predeterminado ( atributo.getDefaultValue () ).
Sexto lugar: "Programación orientada a copiar y pegar"
Fuente: Verificando el código de XMage y por qué no están disponibles las tarjetas raras especiales para la colección Dragon's Maze .
V6072 Se encontraron dos fragmentos de código similares. Quizás, esto es un error tipográfico y la variable 'playerB' debería usarse en lugar de 'playerA'. SubTypeChangingEffectsTest.java (162), SubTypeChangingEffectsTest.java (158), SubTypeChangingEffectsTest.java (156), SubTypeChangingEffectsTest.java (160)
@Test
public void testArcaneAdaptationGiveType() {
addCard(Zone.HAND, playerA, "Arcane Adaptation", 1);
addCard(Zone.BATTLEFIELD, playerA, "Island", 3);
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
....
for (Card card : playerB.getGraveyard().getCards(currentGame)) {
if (card.isCreature()) {
Assert.assertEquals(card.getName() + " should not have ORC type",
false, card.getSubtype(currentGame).contains(SubType.ORC));
Assert.assertEquals(card.getName() + " should have CAT type",
true, card.getSubtype(currentGame).contains(SubType.CAT));
}
}
}
Este año, como el año pasado ( los 10 errores principales en 2019 ), el genial error de copiar y pegar de la regla de diagnóstico V6072 merece un lugar entre los diez primeros.
La naturaleza del error radica en el hecho de que cuando un desarrollador necesita realizar acciones similares para diferentes variables, entonces copia el código escrito de buena fe anteriormente y cambia el nombre de la variable. Pero no lo hace del todo de buena fe y se olvida de modificar las variables.
Esto es exactamente lo que sucedió en este fragmento de código. El autor de la prueba imitaba un juego entre los jugadores, dispersando tarjetas idénticas entre ellas a través de las zonas de juego, pero debido a copiar y pegar, playera obtuvo la misma tarjeta dos veces. Debido a esto, el área de juego es Zone.GRAVEYARDEl jugador playerB se fue sin probar. Se puede encontrar una descripción detallada del error en el propio artículo.
Quinto lugar: "Distribución anormal"
Fuente: Big / Bug Data: análisis del código fuente de Apache Flink
V6048 Esta expresión se puede simplificar. El operando 'índice' en la operación es igual a 0. CollectionUtil.java (76)
public static <T>
Collection<List<T>> partition(Collection<T> elements, int numBuckets)
{
Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);
int initialCapacity = elements.size() / numBuckets;
int index = 0;
for (T element : elements)
{
int bucket = index % numBuckets; // <=
buckets.computeIfAbsent(bucket,
key -> new ArrayList<>(initialCapacity))
.add(element);
}
return buckets.values();
}
El error se descubrió en el método de la utilidad de partición , que divide la colección de elementos pasados en colecciones numBuckets . La esencia del error es que el índice de la colección de cubos en el que se colocará cada elemento en cuestión tiene un valor constante (0). La razón de esto es que el desarrollador olvidó incrementar la variable de índice en cada iteración del ciclo.
Como resultado, el método de partición siempre devolverá una colección de elementos envuelta en otra colección. Y esto no es un comportamiento intencionado.
Cuarto lugar: "Bomba de tiempo"
Fuente: NSA, Ghidra y Unicorns .
V6008 Desreferencia nula de 'selectedNode' en la función 'setViewPanel'. OptionsPanel.java (266)
private void processSelection(OptionsTreeNode selectedNode) {
if (selectedNode == null) {
setViewPanel(defaultPanel, selectedNode); // <=
return;
}
....
}
private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {
....
setHelpLocation(component, selectedNode);
....
}
private void setHelpLocation(JComponent component, OptionsTreeNode node) {
Options options = node.getOptions();
....
}
El fragmento de código anterior está claramente arruinado. Si sigue el selectedNode de processSelection () cuando selectedNode == null , inmediatamente encontrará que nos espera una NullPointerException inminente . Esto es lo que nos advierte el analizador.
Pero, después de estudiar un poco de código, el autor del artículo llegó a la conclusión de que la ejecución del programa nunca encontrará una NullPointerException, ya que se llama a processSelection () en solo dos lugares, antes de llamar a qué selectedNode se verifica explícitamente como nulo.
Independientemente, este código es una bomba de tiempo, porque otro desarrollador podría ver que el método maneja explícitamente el caso selectedNode == null y decidir que este es un valor válido, lo que luego bloqueará la aplicación.
Tercer lugar: "Siempre falso"
Fuente: Verificando el código de XMage y por qué las cartas raras especiales para la colección Dragon's Maze no están disponibles .
V6007 Expression 'filter.getMessage (). ToLowerCase (Locale.ENGLISH) .startsWith ("Each")' siempre es falsa. SetPowerToughnessAllEffect.java (107)
@Override
public String getText(Mode mode) {
StringBuilder sb = new StringBuilder();
....
if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
sb.append(" has base power and toughness ");
} else {
sb.append(" have base power and toughness ");
}
....
return sb.toString();
}
Entonces, ¿quién compara una cadena en minúsculas con una cadena que comienza con una letra mayúscula? De ahí el resultado siempre falso de comprobar el mensaje.
El resultado del defecto no es crítico, sino también desagradable: en alguna parte aparecerá un texto compuesto de forma analfabeta.
Segundo lugar: "2 en 1"
Fuente: NSA, Ghidra y Unicorns .
V6007 La expresión 'índice> = 0' siempre es verdadera. ExternalNamesTableModel.java (105)
V6019 Se detectó un código inalcanzable. Es posible que exista un error. ExternalNamesTableModel.java (109)
public void setValueAt(Object aValue, int row, int column) {
....
int index = indexOf(newName);
if (index >= 0) { // <=
Window window = tool.getActiveWindow();
Msg.showInfo(getClass(), window, "Duplicate Name",
"Name already exists: " + newName);
return;
}
ExternalPath path = paths.get(row); // <=
....
}
private int indexOf(String name) {
for (int i = 0; i < paths.size(); i++) {
ExternalPath path = paths.get(i);
if (path.getName().equals(name)) {
return i;
}
}
return 0;
}
El método indexOf siempre devuelve un número no negativo. Y todo porque el autor del método, en ausencia del newName deseado , devuelve por error 0, no -1. Tal error lleva al hecho de que el flujo de ejecución del programa siempre ingresará a la rama then de la declaración condicional if (índice> = 0) , en la que emitirá un mensaje sobre el nombre nuevo existente y saldrá exitosamente del método, incluso cuando en realidad el nombre nuevo no fue encontró.
Pero eso no es todo. Dado que la rama then de la declaración condicional termina la ejecución del método, las cosas nunca llegarán al código después de la declaración condicional.
El analizador nos advierte sobre esto.
Primer lugar: "¿Lo hemos comprobado?"
Fuente: Under the Hood PVS-Studio para Java: Desarrollo de diagnósticos .
V6080 Considere la posibilidad de buscar errores de impresión. Es posible que una variable asignada deba comprobarse en la siguiente condición. Menu.java (40)
public class Menu
{
private Map<String, List<String>> menus = new HashMap<String, List<String>>();
public void putMenuItem(String menu, String item)
{
List<String> items = menus.get(menu);
if (item == null) // <=
{
items = new ArrayList<String>();
menus.put(menu, items);
}
items.add(item);
}
....
}
Según la idea del autor, se suponía que debía crear una colección mediante la tecla de menú , si aún no existía. Pero comprobar la variable incorrecta arruinó toda la idea, dejando un vacío para la NullPointerException. El método generará una excepción cuando la tecla de menú no esté en el diccionario y el valor del elemento que deseaban agregar no sea nulo .
Conclusión
Las comprobaciones de los proyectos de código abierto que utilizan PVS-Studio año tras año demuestran que una línea de protección como el análisis de código estático debe estar presente en el desarrollo. No importa lo experto que seas, los errores seguramente encontrarán una laguna en tu proyecto, y hay muchas razones para esto: estás cansado, bloqueado en el trabajo o incluso distraído por los gatos. Y si trabaja en equipo, la cantidad de oportunidades de obtener errores en el código aumenta en proporción al número de colegas.
Si le gustó nuestra revisión, no espere hasta el final del próximo año. Los artículos sobre comprobaciones comenzarán inmediatamente a partir del primer mes de 2021 y, si está impaciente, descargue el analizador y compruebe los proyectos de código abierto usted mismo.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Maxim Stefanov. Los 10 errores principales en proyectos Java en 2020 .