Geekly articles weekly

Analisando o código da plataforma CUBA com o PVS-Studio


Os desenvolvedores de Java têm acesso a várias ferramentas úteis que ajudam a escrever códigos de alta qualidade, como o poderoso IDE IntelliJ IDEA, analisadores gratuitos SpotBugs, PMD e similares. Os desenvolvedores que trabalham na Plataforma CUBA já estão usando tudo isso, e esta revisão mostrará como o projeto pode se beneficiar ainda mais do uso do analisador de código estático PVS-Studio.

Algumas palavras sobre o projeto e o analisador


A Plataforma CUBA é uma estrutura de alto nível para o desenvolvimento de aplicativos corporativos. A plataforma abstrai os desenvolvedores das tecnologias subjacentes, para que possam se concentrar nas tarefas de negócios, mantendo total flexibilidade, fornecendo acesso irrestrito ao código de baixo nível. O código fonte foi baixado do GitHub .

O PVS-Studio é uma ferramenta para detectar bugs e possíveis vulnerabilidades de segurança no código fonte dos programas escritos em C, C ++, C # e Java. O analisador é executado em sistemas Windows, Linux e macOS de 64 bits. Para facilitar as coisas para programadores Java, desenvolvemos plugins para Maven, Gradle e IntelliJ IDEA. Eu verifiquei o projeto usando o plug-in Gradle, e ele saiu sem problemas.

Erros nas condições


Aviso 1

A expressão V6007 'StringUtils.isNotEmpty ("handleTabKey")' sempre é verdadeira. SourceCodeEditorLoader.java (60)

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

O valor do atributo extraído do elemento não é verificado. Em vez disso, a função isNotEmpty obtém uma string literal como argumento, em vez da variável handleTabKey .

Um erro semelhante foi encontrado no arquivo AbstractTableLoader.java:

  • A expressão V6007 'StringUtils.isNotEmpty ("editable")' sempre é verdadeira. AbstractTableLoader.java (596)

Aviso 2

A expressão V6007 'previousMenuItemFlatIndex> = 0' sempre é verdadeira. 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; }

A função indexOf retornará -1 se o elemento não for encontrado na lista. O valor 1 é então adicionado ao índice, o que disfarça o problema com o elemento ausente. Outro problema em potencial tem a ver com o fato de que a variável previousMenuItemFlatIndex sempre será maior ou igual a zero. Por exemplo, se a lista menuItemWidgets estiver vazia, o programa terminará com uma saturação de matriz.

Aviso 3

V6009 A função 'delete' pode receber o valor '-1' enquanto se espera um valor não negativo. Argumento de inspeção: 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 "); } .... }

Os dois últimos caracteres do buffer orderBy serão excluídos se o número total de elementos for maior que zero, ou seja, se a sequência contiver pelo menos um caractere. No entanto, a posição inicial de onde a exclusão começa é deslocada por 2. Portanto, se orderBy contiver um caractere, tentar excluí-lo aumentará uma StringIndexOutOfBoundsException .

Aviso 4

V6013 Os objetos 'masterCollection' e 'entidades' são comparados por referência. Possivelmente uma comparação de igualdade foi planejada. 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); } } }

Na função updateMasterCollection , os valores das entidades são copiados para masterCollection . Uma linha anterior, as coleções foram comparadas por referência, mas o programador provavelmente pretendia que fosse uma comparação por valor.

Aviso 5

V6013 Os objetos 'value' e 'oldValue' são comparados por referência. Possivelmente uma comparação de igualdade foi planejada. WebOptionsList.java (278)

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

Este caso é semelhante ao anterior. As coleções são comparadas na função isCollectionValuesChanged e a comparação de referência talvez também não seja o que foi planejado aqui.

Condições redundantes


Aviso 1

A expressão V6007 'mask.charAt (i + deslocamento)! = PlaceHolder' sempre é verdadeira. 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)))) { .... } .... }

A segunda condição verifica uma expressão que é oposta à verificada na primeira condição. O último pode, portanto, ser removido com segurança para encurtar o código.

A expressão V6007 'conector == nulo' é sempre 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; }

Depois de sair do loop while, o valor da variável do conector não será igual a nulo ; portanto, a verificação redundante pode ser excluída.

Outro aviso suspeito desse tipo que precisa ser examinado:

  • A expressão V6007 'StringUtils.isBlank (strValue)' sempre é verdadeira. Param.java (818)

Código inacessível em testes


V6019 Código inacessível detectado. É possível que haja um erro. 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(); } }

A função throwException lança uma exceção que impede a execução da chamada de tx1.commit . Essas duas linhas devem ser trocadas para que o código funcione corretamente.

Também houve alguns problemas semelhantes em outros testes:

  • V6019 Código inacessível detectado. É possível que haja um erro. TransactionTest.java (218)
  • V6019 Código inacessível detectado. É possível que haja um erro. TransactionTest.java (163)
  • V6019 Código inacessível detectado. É possível que haja um erro. TransactionTest.java (203)
  • V6019 Código inacessível detectado. É possível que haja um erro. TransactionTest.java (137)
  • V6019 Código inacessível detectado. É possível que haja um erro. UpdateDetachedTest.java (153)
  • V6019 Código inacessível detectado. É possível que haja um erro. EclipseLinkDetachedTest.java (132)
  • V6019 Código inacessível detectado. É possível que haja um erro. PersistenceTest.java (223)

Argumentos suspeitos


Aviso 1

V6023 O parâmetro 'salt' é sempre reescrito no corpo do método antes de ser usado. BCryptEncryptionModule.java (47)

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

Na criptografia, salt é uma cadeia de dados que você passa junto com a senha para uma função hash. É usado principalmente para proteger o programa contra ataques de dicionário e ataques de tabelas arco-íris, além de ocultar senhas idênticas. Mais aqui: Sal (criptografia) .

Nesta função, a sequência passada é substituída logo após a entrada. Ignorar o valor passado para a função é uma vulnerabilidade em potencial.

Aviso 2

Esta função disparou dois avisos ao mesmo tempo:

  • V6023 O parâmetro 'offsetWidth' é sempre reescrito no corpo do método antes de ser usado. CubaSuggestionFieldWidget.java (433)
  • V6023 O parâmetro 'offsetHeight' é sempre reescrito no corpo do método antes de ser usado. 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); }

Esse é um trecho bastante curioso. A função é chamada com apenas duas variáveis ​​como argumentos, offsetWidth e offsetHeight , e ambas são substituídas antes do uso.

Aviso 3

V6022 O parâmetro 'atalho' não é usado dentro do corpo do construtor. 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); }

A função não utiliza o valor passado como parâmetro de atalho . Talvez a interface da função tenha se tornado obsoleta ou esse aviso seja apenas um falso positivo.

Mais alguns defeitos desse tipo:

  • V6022 O parâmetro 'type' não é usado dentro do corpo do construtor. QueryNode.java (36)
  • V6022 O parâmetro 'text2' não é usado dentro do corpo do construtor. MarkerAddition.java (22)
  • V6022 O parâmetro 'seleção' não é usado dentro do corpo do construtor. AceEditor.java (114)
  • V6022 O parâmetro 'options' não é usado dentro do corpo do construtor. EntitySerialization.java (379)

Funções diferentes, mesmo código


Aviso 1

V6032 É estranho que o corpo do método 'firstItemId' seja totalmente equivalente ao corpo de outro 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(); }

As funções firstItemId e lastItemId têm as mesmas implementações. O último provavelmente foi criado para obter o índice do último elemento em vez de obtê-lo no índice 0.

Aviso 2

V6032 É estranho que o corpo do método seja totalmente equivalente ao corpo de outro 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); }

Mais duas funções com corpos suspeitosamente idênticos. Meu palpite é que um deles foi criado para funcionar com alguma outra cor em vez de com a cor53 .

Desreferência nula


Aviso 1

V6060 A referência 'descriptionPopup' foi utilizada antes de ser verificada com relação a nulo. SuggestPopup.java (252), SuggestPopup.java (251)

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

Em apenas duas linhas, o programador conseguiu escrever um pedaço de código altamente suspeito. Primeiro, o método setPopupPosition do objeto descriptionPopup é chamado e, em seguida, o objeto é verificado como nulo . A primeira chamada para setPopupPosition provavelmente é redundante e potencialmente perigosa. Eu acho que resulta de uma refatoração ruim.

Aviso 2

V6060 A referência 'tableModel' foi utilizada antes de ser verificada com relação a 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 é semelhante ao anterior. Quando o objeto tableModel é verificado como nulo , ele já foi acessado várias vezes.

Outro exemplo:

  • V6060 A referência 'tableModel' foi utilizada antes de ser verificada com relação a nulo. DesktopAbstractTable.java (596), DesktopAbstractTable.java (579)

Provavelmente um erro lógico


V6026 Este valor já está atribuído à variável '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); } } .... }

Na primeira condição, a variável sortAscending já recebeu o valor true , mas ainda assim, o mesmo valor novamente. Isso deve ser um erro, e o autor provavelmente quis dizer o valor falso .

Um exemplo semelhante de um arquivo diferente:

  • V6026 Este valor já está atribuído à variável 'sortAscending'. CubaTreeTableWidget.java (444)

Valores de retorno estranhos


Aviso 1

V6037 Um 'retorno' incondicional dentro de um loop. 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(....)); } .... }

O analisador detectou uma chamada incondicional para retornar na primeira iteração de um loop for . Essa linha está incorreta ou o loop deve ser reescrito como uma declaração if .

Aviso 2

V6014 É estranho que esse método sempre retorne um e o mesmo 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; }

Essa função retorna true em cada caso, enquanto a última linha obviamente exige false . Parece um erro.

Aqui está uma lista completa de outras funções suspeitas semelhantes:

  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. ErrorNodesFinder.java (31)
  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. FileDownloadController.java (69)
  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. IdVarSelector.java (73)
  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. IdVarSelector.java (48)
  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. IdVarSelector.java (67)
  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. IdVarSelector.java (46)
  • V6014 É estranho que esse método sempre retorne um e o mesmo valor. JoinVariableNode.java (57)

Aviso 3

A expressão V6007 'needReload' é sempre 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; }

A função retorna a variável needReload cujo valor é sempre falso . Algum código para alterar esse valor provavelmente está ausente de uma das condições.

Aviso 4

V6062 Possível recursão infinita dentro do método 'isFocused'. GwtAceEditor.java (189), GwtAceEditor.java (190)

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

O analisador detectou uma função recursiva sem condição de parada. Este arquivo contém muitas funções marcadas com a palavra-chave nativa e contendo código comentado. Os desenvolvedores provavelmente estão reescrevendo esse arquivo agora e em breve notarão a função isFocused também.

Diversos


Aviso 1

V6002 A instrução switch não cobre todos os valores da enumeração '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; } }; .... }

A instrução switch não tem nenhum argumento para o valor ADD . É o único valor que não está sendo verificado, portanto, os desenvolvedores devem dar uma olhada neste código.

Aviso 2

V6021 A 'fonte' variável não é usada. 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); } }

A fonte da variável é declarada, mas não usada. Talvez os autores tenham esquecido de adicionar fonte ao layout , assim como aconteceu com outra variável desse tipo, comp .

Outras funções com variáveis ​​não utilizadas:

  • V6021 A 'fonte' variável não é usada. DefaultHorizontalLayoutDropHandler.java (175)
  • V6021 O valor é atribuído à variável 'r', mas não é usado. ExcelExporter.java (262)
  • V6021 A variável 'over' não é usada. DefaultCssLayoutDropHandler.java (49)
  • V6021 A variável 'transferível' não é usada. DefaultHorizontalLayoutDropHandler.java (171)
  • V6021 A variável 'transferível' não é usada. DefaultHorizontalLayoutDropHandler.java (169)
  • V6021 A variável 'beanLocator' não é usada. ScreenEventMixin.java (28)

Aviso 3

As classes V6054 não devem ser comparadas pelo nome. 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; }

O analisador detectou uma comparação de classe por nome. É incorreto comparar as classes pelo nome, pois, de acordo com a especificação, os nomes das classes da JVM devem ser exclusivos apenas dentro de um pacote. Essa comparação gera resultados incorretos e leva à execução do código errado.

Comentado por desenvolvedores da plataforma CUBA


Qualquer projeto grande certamente possui bugs. Sabendo disso, estamos de bom grado quando a equipe do PVS-Studio se ofereceu para verificar nosso projeto. O repositório CUBA contém garfos de algumas das bibliotecas OSS de terceiros licenciadas sob o Apache 2, e parece que deveríamos prestar mais atenção a esse código, pois o analisador encontrou vários problemas nessas fontes. Atualmente, usamos o SpotBugs como nosso analisador principal e ele não percebe alguns dos grandes erros relatados pelo PVS-Studio. Parece que deveríamos escrever alguns diagnósticos adicionais. Muito obrigado à equipe PVS-Studio pelo trabalho.

Os desenvolvedores também nos disseram que os avisos V6013 e V6054 eram falsos positivos; foi a decisão consciente deles de escrever esse código da maneira que eles escreveram. O analisador foi projetado para detectar fragmentos de código suspeitos, e a probabilidade de encontrar erros genuínos varia de acordo com diferentes diagnósticos. Esses avisos, no entanto, podem ser facilmente manipulados usando o mecanismo especial de supressão de avisos em massa sem a necessidade de modificar os arquivos de origem.

Além disso, a equipe do PVS-Studio não pode ignorar a frase "parece que deveríamos escrever alguns diagnósticos adicionais" e ficar sem essa imagem :)

Quadro 3

Conclusão


O PVS-Studio pode ser um complemento perfeito para as ferramentas de controle de qualidade existentes usadas em seu processo de desenvolvimento. É especialmente verdade para empresas com dezenas, centenas ou milhares de desenvolvedores. O PVS-Studio foi projetado não apenas para detectar bugs, mas também para ajudá-lo a corrigi-los, e o que quero dizer com isso não é edição automática de código, mas meios confiáveis ​​de controle de qualidade de código. Em uma grande empresa, é impossível para todo desenvolvedor verificar suas respectivas partes do código com ferramentas diferentes; portanto, uma solução melhor para essas empresas seria adotar ferramentas como o PVS-Studio, que fornece controle de qualidade do código em todas as etapas do desenvolvimento, não somente no lado do programador.

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

More articles:


All Articles