Geekly articles weekly

Analizando el Código de la Plataforma CUBA con PVS-Studio


Los desarrolladores de Java tienen acceso a una serie de herramientas útiles que ayudan a escribir código de alta calidad, como el poderoso IDE IntelliJ IDEA, analizadores gratuitos SpotBugs, PMD y similares. Los desarrolladores que trabajan en la Plataforma CUBA ya han estado usando todo esto, y esta revisión mostrará cómo el proyecto puede beneficiarse aún más del uso del analizador de código estático PVS-Studio.

Algunas palabras sobre el proyecto y el analizador.


CUBA Platform es un marco de alto nivel para el desarrollo de aplicaciones empresariales. La plataforma abstrae a los desarrolladores de las tecnologías subyacentes para que puedan centrarse en las tareas comerciales, al tiempo que retiene la flexibilidad total al proporcionar acceso sin restricciones al código de bajo nivel. El código fuente se descargó de GitHub .

PVS-Studio es una herramienta para detectar errores y posibles vulnerabilidades de seguridad en el código fuente de programas escritos en C, C ++, C # y Java. El analizador se ejecuta en sistemas Windows, Linux y macOS de 64 bits. Para facilitar las cosas a los programadores de Java, desarrollamos complementos para Maven, Gradle e IntelliJ IDEA. Revisé el proyecto usando el complemento Gradle, y salió sin problemas.

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

El valor del atributo extraído del elemento no está marcado. En cambio, la función isNotEmpty obtiene un literal de cadena como argumento en lugar de la variable handleTabKey .

Se encontró un 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 devolverá -1 si el elemento no se encuentra en la lista. El valor 1 se agrega al índice, lo que oculta el problema con el elemento ausente. Otro problema potencial tiene que ver con el hecho de que la variable anteriorMenuItemFlatIndex siempre será mayor o igual que cero. Por ejemplo, si se encuentra que la lista menuItemWidgets está vacía, el programa terminará con una matriz desbordada.

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 "); } .... }

Los dos últimos caracteres del búfer orderBy se eliminan si el número total de elementos es mayor que cero, es decir, si la cadena contiene al menos un carácter. Sin embargo, la posición de inicio desde donde comienza la eliminación está compensada por 2. Por lo tanto, si orderBy contiene un carácter, al intentar eliminarlo se generará 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 . Una línea antes, las colecciones se han comparado por referencia, pero el programador probablemente pretendía que fuera una comparación 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 caso es similar al anterior. Las colecciones se comparan en la función isCollectionValuesChanged , y la comparación de referencias quizás tampoco sea lo que se pretendía aquí.

Condiciones redundantes


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)))) { .... } .... }

La segunda condición verifica una expresión que es opuesta a la marcada en la primera condición. Este último puede, por lo tanto, eliminarse de forma segura para acortar 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; }

Después de abandonar el bucle while, el valor de la variable del conector no será igual a nulo , por lo que se puede eliminar la verificación redundante.

Otra advertencia sospechosa de este tipo que debe ser examinada:

  • 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 ejecución de la llamada de tx1.commit . Esas dos líneas deben intercambiarse para que el código funcione correctamente.

Hubo algunos problemas similares en otras pruebas también:

  • 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


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, salt es una cadena de datos que se pasa junto con la contraseña a una función hash. Se utiliza principalmente para proteger el programa contra ataques de diccionario y ataques de tabla de arco iris, así como para ocultar contraseñas idénticas. Más aquí: Sal (criptografía) .

En esta función, la cadena pasada se sobrescribe justo después de ingresar. Ignorar el valor pasado a la función es una vulnerabilidad potencial.

Advertencia 2

Esta función activó 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); }

Ese es un fragmento bastante curioso. La función se llama con solo dos variables como argumentos, offsetWidth y offsetHeight , y 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); }

La función no utiliza el valor pasado como parámetro de acceso directo . Tal vez la interfaz de la función se ha vuelto obsoleta, o esta advertencia es solo un falso positivo.

Algunos defectos más de este tipo:

  • 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, 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 las mismas implementaciones. Este último probablemente tenía la intención de obtener el índice del último elemento en lugar de obtener el elemento en el índice 0.

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 cuerpos sospechosamente idénticos. Supongo que uno de ellos estaba destinado a trabajar con otro color en lugar de color53 .

Desreferencia 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 programador logró escribir un fragmento de código altamente sospechoso. Primero se llama al método setPopupPosition del objeto descriptionPopup , y luego se verifica que el objeto sea nulo . La primera llamada a setPopupPosition es probablemente redundante y potencialmente peligrosa. Supongo que resulta de una mala refactorización.

Advertencia 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); } .... }

Este caso es similar al anterior. Cuando el objeto tableModel se verifica como nulo , ya se ha accedido varias veces.

Otro ejemplo:

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

Probablemente 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, a la variable sortAscending ya se le ha asignado el valor verdadero , pero aún se le asigna el mismo valor más adelante. Esto debe ser un error, y el autor probablemente quiso decir el valor falso .

Un ejemplo similar de un archivo diferente:

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

Extraños valores de retorno


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 ha detectado una llamada incondicional para regresar en la primera iteración de un bucle for . O esa línea es incorrecta o el ciclo debe reescribirse como una declaració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 devuelve verdadero en cada caso, mientras que la última línea obviamente llama a falso . Parece un error

Aquí hay una lista completa de otras funciones sospechosas similares:

  • 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 . Probablemente falte algún código para cambiar ese valor en una de las condiciones.

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 ha detectado una función recursiva sin condición de parada. Este archivo contiene muchas funciones marcadas con la palabra clave nativa y con código comentado. Los desarrolladores probablemente están reescribiendo este archivo ahora y pronto notarán también la función isFocused .

Misceláneo


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 tiene ningún caso para el valor ADD . Es el único valor que no se está comprobando, por lo que los desarrolladores deberían echar un vistazo a este código.

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 fuente variable se declara pero no se utiliza. Quizás los autores olvidaron agregar la fuente al diseño , tal como sucedió con otra variable de este tipo, comp .

Otras 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 ha detectado una comparación de clase por nombre. Es incorrecto comparar clases por nombre ya que, de acuerdo con la especificación, los nombres de las clases JVM deben ser únicos solo dentro de un paquete. Dicha comparación arroja resultados incorrectos y lleva a ejecutar el código incorrecto.

Comentario de los desarrolladores de la plataforma CUBA


Cualquier proyecto grande seguramente tiene errores. Sabiendo eso, estamos de acuerdo cuando el equipo de PVS-Studio se ofreció a revisar nuestro proyecto. El repositorio CUBA contiene bifurcaciones de algunas de las bibliotecas OSS de terceros con licencia de Apache 2, y parece que deberíamos prestar más atención a ese código ya que el analizador encontró bastantes problemas en esas fuentes. Actualmente utilizamos SpotBugs como nuestro analizador principal, y no se da cuenta de algunos de los grandes errores reportados por PVS-Studio. Parece que deberíamos escribir algunos diagnósticos adicionales nosotros mismos. Muchas gracias al equipo de PVS-Studio por el trabajo.

Los desarrolladores también nos dijeron que las advertencias V6013 y V6054 eran falsos positivos; fue su decisión consciente de escribir ese código de la manera en que lo hicieron. El analizador está diseñado para detectar fragmentos de código sospechosos, y la probabilidad de encontrar errores genuinos varía según los diferentes diagnósticos. Sin embargo, tales advertencias se pueden manejar fácilmente utilizando el mecanismo especial de supresión de advertencia masiva sin tener que modificar los archivos fuente.

Además, el equipo de PVS-Studio no puede darse cuenta de la frase "parece que deberíamos escribir algunos diagnósticos adicionales nosotros mismos" y prescindir de esta imagen :)

Cuadro 3

Conclusión


PVS-Studio puede ser un complemento perfecto para las herramientas de control de calidad existentes utilizadas en su proceso de desarrollo. Es especialmente cierto para empresas con docenas, cientos o miles de desarrolladores. PVS-Studio está diseñado no solo para detectar errores, sino también para ayudarlo a solucionarlos, y lo que quiero decir con eso no es la edición automática de códigos, sino medios confiables de control de calidad de código. En una compañía grande, es imposible que cada desarrollador verifique sus partes respectivas del código con diferentes herramientas, por lo que una mejor solución para esas compañías sería adoptar herramientas como PVS-Studio, que proporcionan control de calidad del código en cada etapa de desarrollo, no solo en el lado del programador.

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

More articles:


All Articles