Geekly articles weekly

Análise de código da plataforma CUBA usando o PVS-Studio

Quadro 2

Para programadores Java, existem ferramentas úteis para ajudá-lo a escrever código de alta qualidade, por exemplo, o poderoso ambiente de desenvolvimento IntelliJ IDEA, SpotBugs gratuitos, analisadores PMD e outros. Tudo isso já é usado no desenvolvimento do projeto CUBA Platform, e nesta revisão dos defeitos de código encontrados, mostrarei de que outra maneira você pode melhorar a qualidade do projeto usando o analisador de código estático PVS-Studio.

Sobre o projeto e o analisador


A Plataforma CUBA é uma estrutura Java de alto nível para criar rapidamente aplicativos corporativos com uma interface da Web completa. A plataforma abstrai o desenvolvedor de tecnologias heterogêneas para que você possa se concentrar na solução de problemas de negócios, ao mesmo tempo, sem tornar impossível trabalhar diretamente com eles. O código fonte é retirado do repositório no GitHub .

O PVS-Studio é uma ferramenta para detectar erros e possíveis vulnerabilidades no código fonte de programas escritos em C, C ++, C # e Java. Funciona em sistemas de 64 bits no Windows, Linux e macOS. Para conveniência dos programadores Java, desenvolvemos plugins para Maven, Gradle e IntelliJ IDEA. O projeto da plataforma CUBA foi facilmente verificado usando o plugin Gradle.

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

Após recuperar o valor do atributo de um elemento, a verificação desse valor não é executada. Em vez disso, uma cadeia constante é passada para a função isNotEmpty , mas tivemos que passar a variável handleTabKey .

Há outro erro semelhante 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 pode retornar -1 se o item não for encontrado na lista. Em seguida, um é adicionado ao índice, ocultando a situação quando o elemento desejado está ausente. Outro problema em potencial pode ser o fato de a variável previousMenuItemFlatIndex sempre ser maior ou igual a zero. Se, por exemplo, a lista menuItemWidgets estiver vazia, será possível ir além do limite da 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 "); } .... }

No buffer de caracteres orderBy, os dois últimos caracteres serão excluídos se o número total for maior que zero, ou seja, uma sequência contém um caractere ou mais. Mas a posição inicial para excluir caracteres foi definida com um deslocamento de 2 caracteres. Portanto, se orderBy consistir repentinamente em 1 caractere, uma tentativa de exclusão lançará 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 . Antes disso, as coleções eram comparadas por referência, mas talvez elas fossem planejadas para serem comparadas 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 exemplo é semelhante ao anterior. Aqui éCollectionValuesChanged é para comparar coleções. Talvez a comparação por referência também não seja o esperado.

Excesso de condições


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

Na segunda comparação, uma expressão que é oposta à primeira é verificada. A segunda verificação pode ser removida para simplificar 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; }

Após a conclusão do loop while , o valor da variável do conector não será nulo ; portanto, a verificação redundante pode ser removida.

Outro lugar suspeito em que os desenvolvedores devem prestar atenção:

  • 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 chamada para a função tx1.commit. Talvez essas linhas devam ser trocadas.

Mais alguns lugares 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 de funções suspeitas


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 sequência de dados que são passados ​​para uma função de hash junto com uma senha. É usado principalmente para proteção contra pesquisas e ataques de dicionário usando tabelas arco-íris, além de ocultar as mesmas senhas. Leia mais: Sal (criptografia) .

Nesta função, a linha é esfregada imediatamente após a entrada na função. Talvez ignorar o valor passado seja uma vulnerabilidade em potencial.

Aviso 2

Para a função considerada, o analisador emite 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); }

Código divertido. A função aceita apenas duas variáveis: offsetWidth e offsetHeight , 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); }

O valor do parâmetro de atalho não é usado na função. A interface da função pode estar desatualizada ou este aviso não é um erro.

Mais alguns lugares semelhantes:

  • 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 com o 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 a mesma implementação. Provavelmente, no último, foi necessário obter um elemento não com o índice 0 , mas calcular o índice do último elemento.

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 implementação suspeitamente idêntica. Atrevo-me a sugerir que em um deles era necessário usar uma cor diferente da cor53 .

Referê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 autor conseguiu escrever um código muito suspeito. Primeiro, o método setPopupPosition é chamado no objeto descriptionPopup e, em seguida, o objeto é comparado a nulo . Provavelmente, a primeira chamada para a função setPopupPosition é redundante e perigosa. Parece que as consequências da refatoração com falha.

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

Uma situação semelhante existe nesta função. Após inúmeras chamadas para o objeto tableModel , é feita uma verificação para ver se é nulo ou não.

Outro exemplo:

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

Talvez 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á é verdadeira , mas ainda recebe o mesmo valor. Talvez isso seja um erro e deseje atribuir falso .

Um exemplo semelhante de outro arquivo:

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

Valores de retorno de funções estranhas


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 à instrução de retorno na primeira iteração do loop for . Talvez isso seja um erro ou você precise reescrever o código para usar a instruçã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; }

Esta função em todos os casos retorna verdadeira . Mas aqui, na última linha, implora o retorno do falso . Talvez haja um erro.

A lista inteira de funções suspeitas com código semelhante:

  • 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 . Provavelmente, em uma das condições, eles esqueceram de adicionar o código para alterar o valor da variável.

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 chamada recursivamente sem uma condição para interromper a recursão. Esse arquivo tem muitas funções que são marcadas com a palavra-chave nativa e contêm código comentado. Provavelmente, o arquivo está sendo substituído atualmente e em breve os desenvolvedores prestarão atenção à função isFocused .

Avisos 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 aborda o valor de ADD . É o único não revisado, por isso vale a pena verificar se é um erro ou não.

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 variável de origem é declarada e não usada no código. Talvez, como outra variável comp do mesmo tipo, eles tenham esquecido de adicionar fonte ao layout .

Mais 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 situação ao comparar classes por nome. Essa comparação está incorreta porque, de acordo com a especificação, as classes da JVM têm um nome exclusivo apenas dentro do pacote. Isso pode causar comparação e execução incorretas do código errado que foi planejado.

Feedback do desenvolvedor de plataforma CUBA


Obviamente, em qualquer projeto importante, há bugs. Por isso, concordamos com satisfação com a proposta da equipe do PVS-Studio de verificar nosso projeto. O repositório CUBA inclui garfos de algumas bibliotecas OSS de terceiros sob a licença Apache 2, e parece que precisamos prestar mais atenção a esse código, o analisador encontrou alguns problemas nessas fontes. Agora usamos o SpotBugs como o analisador principal e ele não encontra alguns problemas significativos encontrados pelo PVS-Studio. Está na hora de escrever os cheques adicionais. Muito obrigado à equipe PVS-Studio pelo trabalho realizado.

Os desenvolvedores também observaram que os avisos V6013 e V6054 são falsos. O código foi escrito de forma intencional. O analisador estático foi projetado para detectar fragmentos de código suspeitos e a probabilidade de encontrar um erro é diferente para todas as inspeções. No entanto, é fácil trabalhar com esses avisos usando o mecanismo conveniente para supressão em massa de avisos do analisador sem modificar os arquivos de código-fonte.

Outra equipe do PVS-Studio não pode ignorar a frase “é hora de escrevermos verificações adicionais” e não deixar essa imagem :)

Quadro 1

Conclusão


O PVS-Studio será um ótimo complemento para o seu projeto existente para melhorar as ferramentas de qualidade de código. Especialmente se houver dezenas, centenas e milhares de funcionários. O PVS-Studio foi projetado não apenas para encontrar erros, mas também para corrigi-los. Além disso, não estamos falando de edição automática de código, mas de controle confiável da qualidade do código. Em uma grande empresa, é impossível imaginar uma situação em que absolutamente todos os desenvolvedores verifiquem independentemente seu código com várias ferramentas, portanto, ferramentas como o PVS-Studio são mais adequadas para essas empresas, onde o controle de qualidade do código é fornecido em todas as etapas do desenvolvimento, não apenas para um programador comum.



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Svyatoslav Razmyslov. Analisando o código da plataforma CUBA com o PVS-Studio

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

More articles:


All Articles