Geekly articles weekly

Análisis de código de la plataforma CUBA usando PVS-Studio

Imagen 2

Para los programadores de Java, existen herramientas útiles para ayudarlo a escribir código de calidad, por ejemplo, el poderoso entorno de desarrollo IntelliJ IDEA, SpotBugs gratuitos, analizadores PMD y otros. Todo esto ya se usa en el desarrollo del proyecto CUBA Platform, y en esta revisión de los defectos de código encontrados, le diré cómo puede mejorar la calidad del proyecto utilizando el analizador de código estático PVS-Studio.

Sobre el proyecto y el analizador.


CUBA Platform es un marco Java de alto nivel para crear rápidamente aplicaciones empresariales con una interfaz web completa. La plataforma abstrae al desarrollador de tecnologías heterogéneas para que pueda concentrarse en resolver problemas comerciales, al mismo tiempo, sin que sea imposible trabajar con ellos directamente. El código fuente se toma del repositorio en GitHub .

PVS-Studio es una herramienta para detectar errores y vulnerabilidades potenciales en el código fuente de programas escritos en C, C ++, C # y Java. Funciona en sistemas de 64 bits en Windows, Linux y macOS. Para la comodidad de los programadores de Java, hemos desarrollado complementos para Maven, Gradle e IntelliJ IDEA. El proyecto de la plataforma CUBA se verificó fácilmente utilizando el complemento Gradle.

Errores en las condiciones.


Advertencia 1

La expresión V6007 'StringUtils.isNotEmpty ("handleTabKey") siempre es verdadera. SourceCodeEditorLoader.java (60)

@Override public void loadComponent() { .... String handleTabKey = element.attributeValue("handleTabKey"); if (StringUtils.isNotEmpty("handleTabKey")) { resultComponent.setHandleTabKey(Boolean.parseBoolean(handleTabKey)); } .... }

Después de recuperar el valor del atributo de un elemento, no se realiza la verificación de este valor. En cambio, se pasa una cadena constante a la función isNotEmpty , pero tuvimos que pasar la variable handleTabKey .

Hay otro error similar en el archivo AbstractTableLoader.java:

  • V6007 La expresión 'StringUtils.isNotEmpty ("editable")' siempre es verdadera. AbstractTableLoader.java (596)

Advertencia 2

V6007 La expresión 'previousMenuItemFlatIndex> = 0' siempre es verdadera. CubaSideMenuWidget.java (328)

 protected MenuItemWidget findNextMenuItem(MenuItemWidget currentItem) { List<MenuTreeNode> menuTree = buildVisibleTree(this); List<MenuItemWidget> menuItemWidgets = menuTreeToList(menuTree); int menuItemFlatIndex = menuItemWidgets.indexOf(currentItem); int previousMenuItemFlatIndex = menuItemFlatIndex + 1; if (previousMenuItemFlatIndex >= 0) { return menuItemWidgets.get(previousMenuItemFlatIndex); } return null; }

La función indexOf puede devolver -1 si el elemento no se encuentra en la lista. Luego se agrega uno al índice, ocultando así la situación cuando falta el elemento deseado. Otro problema potencial puede ser el hecho de que la variable previousMenuItemFlatIndex siempre será mayor o igual que cero. Si, por ejemplo, la lista menuItemWidgets está vacía, entonces es posible ir más allá del límite de la matriz.

Advertencia 3

V6009 La función 'eliminar' podría recibir el valor '-1' mientras se espera un valor no negativo. Inspeccionar argumento: 1. AbstractCollectionDatasource.java (556)

 protected DataLoadContextQuery createDataQuery(....) { .... StringBuilder orderBy = new StringBuilder(); .... if (orderBy.length() > 0) { orderBy.delete(orderBy.length() - 2, orderBy.length()); orderBy.insert(0, " order by "); } .... }

En el orden de almacenamiento intermedio de caracteres Por, los últimos 2 caracteres se eliminan si su número total es mayor que cero, es decir una cadena contiene un caracter o más. Pero la posición inicial para eliminar caracteres se estableció con un desplazamiento de 2 caracteres. Por lo tanto, si orderBy de repente consta de 1 carácter, un intento de eliminación arrojará una StringIndexOutOfBoundsException .

Advertencia 4

Los objetos V6013 'masterCollection' y 'entidades' se comparan por referencia. Posiblemente se pretendía una comparación de igualdad. CollectionPropertyContainerImpl.java (81)

 @Override public void setItems(@Nullable Collection<E> entities) { super.setItems(entities); Entity masterItem = master.getItemOrNull(); if (masterItem != null) { MetaProperty masterProperty = getMasterProperty(); Collection masterCollection = masterItem.getValue(masterProperty.getName()); if (masterCollection != entities) { updateMasterCollection(masterProperty, masterCollection, entities); } } }

En la función updateMasterCollection, los valores de las entidades se copian en masterCollection . Antes de esto, las colecciones se compararon por referencia, pero tal vez se planificó que se compararan por valor.

Advertencia 5

V6013 Los objetos 'valor' y 'oldValue' se comparan por referencia. Posiblemente se pretendía una comparación de igualdad. WebOptionsList.java (278)

 protected boolean isCollectionValuesChanged(Collection<I> value, Collection<I> oldValue) { return value != oldValue; }

Este ejemplo es similar al anterior. Aquí isCollectionValuesChanged es para comparar colecciones. Quizás la comparación por referencia tampoco sea la forma esperada.

Condiciones de exceso


Advertencia 1

V6007 La expresión 'mask.charAt (i + offset)! = PlaceHolder' siempre es verdadera. DatePickerDocument.java (238)

 private String calculateFormattedString(int offset, String text) .... { .... if ((mask.charAt(i + offset) == placeHolder)) { // <= .... } else if ((mask.charAt(i + offset) != placeHolder) && // <= (Character.isDigit(text.charAt(i)))) { .... } .... }

En la segunda comparación, se verifica una expresión que es opuesta a la primera. La segunda verificación se puede eliminar para simplificar el código.

V6007 La expresión 'conector == nulo' siempre es falsa. HTML5Support.java (169)

 private boolean validate(NativeEvent event) { .... while (connector == null) { widget = widget.getParent(); connector = Util.findConnectorFor(widget); } if (this.connector == connector) { return true; } else if (connector == null) { // <= return false; } else if (connector.getWidget() instanceof VDDHasDropHandler) { return false; } return true; }

Una vez que se completa el ciclo while , el valor de la variable del conector no será nulo , por lo tanto, se puede eliminar la verificación redundante.

Otro lugar sospechoso al que los desarrolladores deben prestar atención:

  • V6007 La expresión 'StringUtils.isBlank (strValue)' siempre es verdadera. Param.java (818)

Código inalcanzable en pruebas


V6019 Código inalcanzable detectado. Es posible que haya un error presente. TransactionTest.java (283)

 private void throwException() { throw new RuntimeException(TEST_EXCEPTION_MSG); } @Test public void testSuspendRollback() { Transaction tx = cont.persistence().createTransaction(); try { .... Transaction tx1 = cont.persistence().createTransaction(); try { EntityManager em1 = cont.persistence().getEntityManager(); assertTrue(em != em1); Server server1 = em1.find(Server.class, server.getId()); assertNull(server1); throwException(); // <= tx1.commit(); // <= } catch (Exception e) { // } finally { tx1.end(); } tx.commit(); } finally { tx.end(); } }

La función throwException produce una excepción que impide la llamada a la función tx1.commit. Quizás estas líneas deberían intercambiarse.

Algunos lugares más similares en otras pruebas:

  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TransactionTest.java (218)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TransactionTest.java (163)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TransactionTest.java (203)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TransactionTest.java (137)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. UpdateDetachedTest.java (153)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. EclipseLinkDetachedTest.java (132)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. PersistenceTest.java (223)

Argumentos sospechosos de funciones


Advertencia 1

V6023 El parámetro 'sal' siempre se reescribe en el cuerpo del método antes de usarse. BCryptEncryptionModule.java (47)

 @Override public String getHash(String content, String salt) { salt = BCrypt.gensalt(); return BCrypt.hashpw(content, salt); }

En criptografía, la sal es una cadena de datos que se pasa a una función hash junto con una contraseña. Se utiliza principalmente para la protección contra búsquedas y ataques de diccionario mediante tablas de arco iris, así como para ocultar las mismas contraseñas. Leer más: Sal (criptografía) .

En esta función, la línea se frota inmediatamente al ingresar a la función. Quizás ignorar el valor pasado es una vulnerabilidad potencial.

Advertencia 2

Para la función considerada, el analizador da dos advertencias a la vez:

  • V6023 El parámetro 'offsetWidth' siempre se reescribe en el cuerpo del método antes de usarse. CubaSuggestionFieldWidget.java (433)
  • V6023 El parámetro 'offsetHeight' siempre se reescribe en el cuerpo del método antes de usarse. CubaSuggestionFieldWidget.java (433)


 @Override public void setPosition(int offsetWidth, int offsetHeight) { offsetHeight = getOffsetHeight(); .... if (offsetHeight + getPopupTop() > ....)) { .... } .... offsetWidth = containerFirstChild.getOffsetWidth(); if (offsetWidth + getPopupLeft() > ....)) { .... } else { left = getPopupLeft(); } setPopupPosition(left, top); }

Código divertido La función solo toma dos variables: offsetWidth y offsetHeight , ambas se sobrescriben antes de su uso.

Advertencia 3

V6022 El parámetro 'acceso directo' no se usa dentro del cuerpo del constructor. DeclarativeTrackingAction.java (47)

 public DeclarativeTrackingAction(String id, String caption, String description, String icon, String enable, String visible, String methodName, @Nullable String shortcut, ActionsHolder holder) { super(id); this.caption = caption; this.description = description; this.icon = icon; setEnabled(enable == null || Boolean.parseBoolean(enable)); setVisible(visible == null || Boolean.parseBoolean(visible)); this.methodName = methodName; checkActionsHolder(holder); }

El valor del parámetro de acceso directo no se utiliza en la función. La interfaz de la función puede estar desactualizada o esta advertencia no es un error.

Algunos lugares más similares:

  • V6022 El parámetro 'tipo' no se usa dentro del cuerpo del constructor. QueryNode.java (36)
  • V6022 El parámetro 'text2' no se usa dentro del cuerpo del constructor. MarkerAddition.java (22)
  • El parámetro 'selección' V6022 no se usa dentro del cuerpo del constructor. AceEditor.java (114)
  • El parámetro V6022 'opciones' no se usa dentro del cuerpo del constructor. EntitySerialization.java (379)

Diferentes funciones con el mismo código.


Advertencia 1

V6032 Es extraño que el cuerpo del método 'firstItemId' sea completamente equivalente al cuerpo de otro método 'lastItemId'. ContainerTableItems.java (213), ContainerTableItems.java (219)

 @Override public Object firstItemId() { List<E> items = container.getItems(); return items.isEmpty() ? null : items.get(0).getId(); } @Override public Object lastItemId() { List<E> items = container.getItems(); return items.isEmpty() ? null : items.get(0).getId(); }

Las funciones firstItemId y lastItemId tienen la misma implementación. Lo más probable es que en este último fuera necesario obtener un elemento no con índice 0 , sino calcular el índice del último elemento.

Advertencia 2

V6032 Es extraño que el cuerpo del método sea totalmente equivalente al cuerpo de otro método. SearchComboBoxPainter.java (495), SearchComboBoxPainter.java (501)

 private void paintBackgroundDisabledAndEditable(Graphics2D g) { rect = decodeRect1(); g.setPaint(color53); g.fill(rect); } private void paintBackgroundEnabledAndEditable(Graphics2D g) { rect = decodeRect1(); g.setPaint(color53); g.fill(rect); }

Dos funciones más con implementación sospechosamente idéntica. Me atrevería a sugerir que en uno de ellos era necesario usar un color diferente al color53 .

Referencia nula


Advertencia 1

V6060 La referencia 'descriptionPopup' se utilizó antes de que se verificara contra nulo. SugerirPopup.java (252), SugerirPopup.java (251)

 protected void updateDescriptionPopupPosition() { int x = getAbsoluteLeft() + WIDTH; int y = getAbsoluteTop(); descriptionPopup.setPopupPosition(x, y); if (descriptionPopup!=null) { descriptionPopup.setPopupPosition(x, y); } }

En solo dos líneas, el autor logró escribir un código muy sospechoso. Primero, se llama al método setPopupPosition en el objeto descriptionPopup , y luego el objeto se compara con nulo . Lo más probable es que la primera llamada a la función setPopupPosition sea ​​redundante y peligrosa. Parecen las consecuencias de una refactorización fallida.

Advertencias 2

V6060 La referencia 'tableModel' se utilizó antes de que se verificara contra nulo. DesktopAbstractTable.java (1580), DesktopAbstractTable.java (1564)

 protected Column addRuntimeGeneratedColumn(String columnId) { // store old cell editors / renderers TableCellEditor[] cellEditors = new TableCellEditor[tableModel.getColumnCount() + 1]; // <= TableCellRenderer[] cellRenderers = new TableCellRenderer[tableModel.getColumnCount() + 1]; // <= for (int i = 0; i < tableModel.getColumnCount(); i++) { // <= Column tableModelColumn = tableModel.getColumn(i); if (tableModel.isGeneratedColumn(tableModelColumn)) { // <= TableColumn tableColumn = getColumn(tableModelColumn); cellEditors[i] = tableColumn.getCellEditor(); cellRenderers[i] = tableColumn.getCellRenderer(); } } Column col = new Column(columnId, columnId); col.setEditable(false); columns.put(col.getId(), col); if (tableModel != null) { // <= tableModel.addColumn(col); } .... }

Una situación similar existe en esta función. Después de numerosas llamadas al objeto tableModel , se verifica si es nulo o no.

Otro ejemplo:

  • V6060 La referencia 'tableModel' se utilizó antes de que se verificara contra nulo. DesktopAbstractTable.java (596), DesktopAbstractTable.java (579)

Quizás un error lógico


V6026 Este valor ya está asignado a la variable 'sortAscending'. CubaScrollTableWidget.java (488)

 @Override protected void sortColumn() { .... if (sortAscending) { if (sortClickCounter < 2) { // special case for initial revert sorting instead of reset sort order if (sortClickCounter == 0) { client.updateVariable(paintableId, "sortascending", false, false); } else { reloadDataFromServer = false; sortClickCounter = 0; sortColumn = null; sortAscending = true; // <= client.updateVariable(paintableId, "resetsortorder", "", true); } } else { client.updateVariable(paintableId, "sortascending", false, false); } } else { if (sortClickCounter < 2) { // special case for initial revert sorting instead of reset sort order if (sortClickCounter == 0) { client.updateVariable(paintableId, "sortascending", true, false); } else { reloadDataFromServer = false; sortClickCounter = 0; sortColumn = null; sortAscending = true; client.updateVariable(paintableId, "resetsortorder", "", true); } } else { reloadDataFromServer = false; sortClickCounter = 0; sortColumn = null; sortAscending = true; client.updateVariable(paintableId, "resetsortorder", "", true); } } .... }

En la primera condición, la variable sortAscending ya es verdadera , pero aún se le asigna el mismo valor. Quizás esto es un error y quería asignar falso .

Un ejemplo similar de otro archivo:

  • V6026 Este valor ya está asignado a la variable 'sortAscending'. CubaTreeTableWidget.java (444)

Extraños valores de retorno de funciones


Advertencia 1

V6037 Un 'retorno' incondicional dentro de un bucle. QueryCacheManager.java (128)

 public <T> T getSingleResultFromCache(QueryKey queryKey, List<View> views) { .... for (Object id : queryResult.getResult()) { return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....)); } .... }

El analizador detectó una llamada incondicional a la declaración de retorno en la primera iteración del bucle for . Quizás esto sea un error, o necesite reescribir el código para usar la instrucción if .

Advertencia 2

V6014 Es extraño que este método siempre devuelva el mismo valor. DefaultExceptionHandler.java (40)

 @Override public boolean handle(ErrorEvent event, App app) { Throwable t = event.getThrowable(); if (t instanceof SocketException || ExceptionUtils.getRootCause(t) instanceof SocketException) { return true; } if (ExceptionUtils.getThrowableList(t).stream() .anyMatch(o -> o.getClass().getName().equals("...."))) { return true; } if (StringUtils.contains(ExceptionUtils.getMessage(t), "....")) { return true; } AppUI ui = AppUI.getCurrent(); if (ui == null) { return true; } if (t != null) { if (app.getConnection().getSession() != null) { showDialog(app, t); } else { showNotification(app, t); } } return true; }

Esta función en todos los casos devuelve verdadero . Pero aquí, en la última línea, se pide el retorno de falso . Quizás haya un error.

La lista completa de funciones sospechosas con código similar:

  • V6014 Es extraño que este método siempre devuelva el mismo valor. ErrorNodesFinder.java (31)
  • V6014 Es extraño que este método siempre devuelva el mismo valor. FileDownloadController.java (69)
  • V6014 Es extraño que este método siempre devuelva el mismo valor. IdVarSelector.java (73)
  • V6014 Es extraño que este método siempre devuelva el mismo valor. IdVarSelector.java (48)
  • V6014 Es extraño que este método siempre devuelva el mismo valor. IdVarSelector.java (67)
  • V6014 Es extraño que este método siempre devuelva el mismo valor. IdVarSelector.java (46)
  • V6014 Es extraño que este método siempre devuelva el mismo valor. JoinVariableNode.java (57)

Advertencia 3

V6007 La expresión 'needReload' siempre es falsa. WebAbstractTable.java (2702)

 protected boolean handleSpecificVariables(Map<String, Object> variables) { boolean needReload = false; if (isUsePresentations() && presentations != null) { Presentations p = getPresentations(); if (p.getCurrent() != null && p.isAutoSave(p.getCurrent()) && needUpdatePresentation(variables)) { Element e = p.getSettings(p.getCurrent()); saveSettings(e); p.setSettings(p.getCurrent(), e); } } return needReload; }

La función devuelve la variable needReload , cuyo valor siempre es falso . Lo más probable es que, en una de las condiciones, olvidaron agregar el código para cambiar el valor de la variable.

Advertencia 4

V6062 Posible recursión infinita dentro del método 'isFocused'. GwtAceEditor.java (189), GwtAceEditor.java (190)

 public final native void focus() /*-{ this.focus(); }-*/; public final boolean isFocused() { return this.isFocused(); }

El analizador detectó una función que se llama recursivamente sin una condición para detener la recursividad. Este archivo tiene muchas funciones que están etiquetadas con la palabra clave nativa y contienen código comentado. Lo más probable es que el archivo se sobrescriba actualmente y pronto los desarrolladores prestarán atención a la función isFocused .

Advertencias varias


Advertencia 1

V6002 La sentencia switch no cubre todos los valores de la enumeración 'Operation': ADD. DesktopAbstractTable.java (665)

 /** * Operation which caused the datasource change. */ enum Operation { REFRESH, CLEAR, ADD, REMOVE, UPDATE } @Override public void setDatasource(final CollectionDatasource datasource) { .... collectionChangeListener = e -> { switch (e.getOperation()) { case CLEAR: case REFRESH: fieldDatasources.clear(); break; case UPDATE: case REMOVE: for (Object entity : e.getItems()) { fieldDatasources.remove(entity); } break; } }; .... }

La instrucción switch no aborda el valor de ADD . Es el único no revisado, por lo que vale la pena verificar si es un error o no.

Advertencia 2

V6021 La variable 'fuente' no se utiliza. DefaultHorizontalLayoutDropHandler.java (177)

 @Override protected void handleHTML5Drop(DragAndDropEvent event) { LayoutBoundTransferable transferable = (LayoutBoundTransferable) event .getTransferable(); HorizontalLayoutTargetDetails details = (HorizontalLayoutTargetDetails) event .getTargetDetails(); AbstractOrderedLayout layout = (AbstractOrderedLayout) details .getTarget(); Component source = event.getTransferable().getSourceComponent(); // <= int idx = (details).getOverIndex(); HorizontalDropLocation loc = (details).getDropLocation(); if (loc == HorizontalDropLocation.CENTER || loc == HorizontalDropLocation.RIGHT) { idx++; } Component comp = resolveComponentFromHTML5Drop(event); if (idx >= 0) { layout.addComponent(comp, idx); } else { layout.addComponent(comp); } if (dropAlignment != null) { layout.setComponentAlignment(comp, dropAlignment); } }

La variable fuente se declara y no se usa en el código. Tal vez, como otra variable comp del mismo tipo, se olvidaron de agregar la fuente al diseño .

Más funciones con variables no utilizadas:

  • V6021 La variable 'fuente' no se utiliza. DefaultHorizontalLayoutDropHandler.java (175)
  • V6021 El valor se asigna a la variable 'r' pero no se usa. ExcelExporter.java (262)
  • V6021 La variable 'sobre' no se usa. DefaultCssLayoutDropHandler.java (49)
  • V6021 Variable 'transferible' no se utiliza. DefaultHorizontalLayoutDropHandler.java (171)
  • V6021 Variable 'transferible' no se utiliza. DefaultHorizontalLayoutDropHandler.java (169)
  • V6021 La variable 'beanLocator' no se usa. ScreenEventMixin.java (28)

Advertencia 3

Las clases V6054 no deben compararse por su nombre. MessageTools.java (283)

 public boolean hasPropertyCaption(MetaProperty property) { Class<?> declaringClass = property.getDeclaringClass(); if (declaringClass == null) return false; String caption = getPropertyCaption(property); int i = caption.indexOf('.'); if (i > 0 && declaringClass.getSimpleName().equals(caption.substring(0, i))) return false; else return true; }

El analizador detectó una situación cuando la comparación de clases se realiza por nombre. Dicha comparación es incorrecta porque, según la especificación, las clases JVM tienen un nombre único solo dentro del paquete. Esto puede causar una comparación y ejecución incorrecta del código incorrecto que se planeó.

Revisión del desarrollador de la plataforma CUBA


Por supuesto, en cualquier proyecto importante hay errores. Es por eso que aceptamos con gusto la propuesta del equipo de PVS-Studio para verificar nuestro proyecto. El repositorio CUBA incluye bifurcaciones de algunas bibliotecas OSS de terceros bajo la licencia Apache 2, y parece que debemos prestar más atención a este código, el analizador encontró bastantes problemas en estas fuentes. Ahora usamos SpotBugs como el analizador principal, y no encuentra algunos problemas significativos encontrados por PVS-Studio. Es hora de ir y escribir cheques adicionales usted mismo. Muchas gracias al equipo de PVS-Studio por el trabajo realizado.

Los desarrolladores también notaron que las advertencias V6013 y V6054 son falsas. El código fue escrito de manera intencional. El analizador estático está diseñado para detectar fragmentos de código sospechosos y la probabilidad de encontrar un error es diferente para todas las inspecciones. Sin embargo, es fácil trabajar con tales advertencias utilizando el mecanismo conveniente para la supresión masiva de advertencias del analizador sin modificar los archivos de código fuente.

Otro equipo de PVS-Studio no puede ignorar la frase "es hora de ir a escribir cheques adicionales nosotros mismos" y no dejar esta imagen :)

Imagen 1

Conclusión


PVS-Studio será una gran adición a su proyecto existente para mejorar las herramientas de calidad de código. Especialmente si hay docenas, cientos y miles de empleados. PVS-Studio está diseñado no solo para encontrar errores, sino también para corregirlos. Además, no estamos hablando de la edición automática de código, sino de un control de calidad de código confiable. En una empresa grande, es imposible imaginar una situación en la que absolutamente todos los desarrolladores verifiquen independientemente su código con varias herramientas, por lo que herramientas como PVS-Studio son más adecuadas para dichas empresas, donde el control de calidad del código se proporciona en todas las etapas de desarrollo, no solo para un programador ordinario.



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Svyatoslav Razmyslov. Analizando el Código de la Plataforma CUBA con PVS-Studio

Source: https://habr.com/ru/post/449022/

More articles:


All Articles