Geekly articles weekly

使用PVS-Studio分析CUBA平台的代码


Java开发人员可以使用许多有用的工具来帮助编写高质量的代码，例如功能强大的IDE IntelliJ IDEA，免费的分析器SpotBugs，PMD等。 在CUBA平台上工作的开发人员已经在使用所有这些工具，本次审查将展示该项目如何从静态代码分析器PVS-Studio的使用中进一步受益。

关于项目和分析器的几句话


CUBA平台是用于企业应用程序开发的高级框架。 该平台使开发人员从基础技术中抽象出来，使他们可以专注于业务任务，同时通过提供对低级代码的无限制访问来保留完全的灵活性。 源代码是从GitHub下载的。

PVS-Studio是一种工具，用于检测用C，C ++，C＃和Java编写的程序的源代码中的错误和潜在的安全漏洞。 该分析仪可在64位Windows，Linux和macOS系统上运行。 为了使Java程序员更轻松，我们为Maven，Gradle和IntelliJ IDEA开发了插件。 我使用Gradle插件检查了该项目，结果一切顺利。

条件错误


警告1

V6007表达式'StringUtils.isNotEmpty（“ handleTabKey”）'始终为true。 SourceCodeEditorLoader.java（60）

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

不检查从元素提取的属性值。 相反， isNotEmpty函数获取字符串文字作为其参数，而不是变量handleTabKey 。

在文件AbstractTableLoader.java中发现类似的错误：

  • V6007表达式'StringUtils.isNotEmpty（“ editable”）'始终为true。 AbstractTableLoader.java（596）

警告2

V6007表达式'previousMenuItemFlatIndex> = 0'始终为true。 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; }

如果在列表中找不到元素，则indexOf函数将返回-1 。 然后将值1添加到索引中，从而掩盖了元素缺失的问题。 另一个潜在的问题与上一个MenuItemFlatIndex变量将始终大于或等于零有关。 例如，如果发现menuItemWidgets列表为空，则该程序将以数组溢出结束。

警告3

V6009 “删除”功能可以接收“ -1”值，而预期为非负值。 检查参数：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 "); } .... }

如果元素总数大于零（即，字符串包含至少一个字符），则删除orderBy缓冲区的最后两个字符。 但是，删除开始的起始位置偏移了2。因此，如果orderBy碰巧包含一个字符，则尝试删除它会引发StringIndexOutOfBoundsException 。

警告4

V6013通过引用比较了对象“ masterCollection ”和“实体”。 可能希望进行平等比较。 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); } } }

在updateMasterCollection函数中，来自实体的值被复制到masterCollection 。 前面一行，已经通过引用比较了集合，但是程序员可能希望将其作为按值比较。

警告5

V6013通过引用比较对象“值”和“ 旧值”。 可能希望进行平等比较。 WebOptionsList.java（278）

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

这种情况类似于前一种情况。 在isCollectionValuesChanged函数中对集合进行比较，引用比较可能也不是这里想要的。

冗余条件


警告1

V6007表达式'mask.charAt（i + offset）！= PlaceHolder'始终为true。 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)))) { .... } .... }

第二个条件检查与第一个条件中检查的表达式相反的表达式。 因此，可以安全地删除后者以缩短代码。

V6007表达式'connector == null'始终为false。 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; }

退出while循环后， 连接器变量的值将不等于null ，因此可以删除冗余检查。

需要检查的另一种此类可疑警告：

  • V6007表达式'StringUtils.isBlank（strValue）'始终为true。 Param.java（818）

测试中无法访问的代码


V6019检测不到代码。 可能存在错误。 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(); } }

throwException函数引发一个异常，该异常阻止执行tx1.commit的调用。 应当交换这两行以使代码正常工作。

其他测试也有一些类似的问题：

  • V6019检测不到代码。 可能存在错误。 TransactionTest.java（218）
  • V6019检测不到代码。 可能存在错误。 TransactionTest.java（163）
  • V6019检测不到代码。 可能存在错误。 TransactionTest.java（203）
  • V6019检测不到代码。 可能存在错误。 TransactionTest.java（137）
  • V6019检测不到代码。 可能存在错误。 UpdateDetachedTest.java（153）
  • V6019检测不到代码。 可能存在错误。 EclipseLinkDetachedTest.java（132）
  • V6019检测不到代码。 可能存在错误。 PersistenceTest.java（223）

可疑论点


警告1

V6023参数“ salt”在使用前总是在方法体内被重写。 BCryptEncryptionModule.java（47）

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

在密码术中， salt是一个数据字符串，您将其与密码一起传递给哈希函数。 它主要用于保护程序免受字典攻击和彩虹表攻击，以及掩盖相同的密码。 更多信息： Salt（密码学） 。

在此函数中，输入后立即将传递的字符串覆盖。 忽略传递给函数的值是潜在的漏洞。

警告2

此功能立即触发两个警告：

  • V6023参数'offsetWidth'始终在使用前在方法主体中重写。 CubaSuggestionFieldWidget.java（433）
  • V6023参数'offsetHeight'总是在使用前在方法主体中重写。 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); }

那是一个很好奇的片段。 仅使用两个变量作为参数调用该函数offsetWidth和offsetHeight ，并且在使用前都将其覆盖。

警告3

V6022构造函数体内未使用参数“快捷方式”。 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); }

该函数不使用作为快捷方式参数传递的值。 函数的界面可能已过时，或者此警告只是误报。

这种类型的一些缺陷：

  • V6022构造函数体内未使用参数'type'。 QueryNode.java（36）
  • V6022构造函数体内未使用参数'text2'。 MarkerAddition.java（22）
  • V6022构造函数体内未使用参数“选择”。 AceEditor.java（114）
  • V6022参数'options'不在构造函数体内使用。 EntitySerialization.java（379）

功能不同，代码相同


警告1

V6032方法'firstItemId'的主体完全等效于另一个方法'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(); }

函数firstItemId和lastItemId具有相同的实现。 后者可能意味着获取最后一个元素的索引，而不是获取索引0处的元素。

警告2

V6032方法的主体完全等同于另一种方法的主体，这很奇怪。 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); }

具有可疑相同主体的另外两个功能。 我的猜测是，其中一个应该使用其他颜色而不是color53 。

空解除引用


警告1

V6060已使用'descriptionPopup'参考，然后将其对照null进行验证。 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); } }

仅用两行，程序员就编写了一个高度可疑的代码。 首先，调用对象descriptionPopup的 setPopupPosition方法，然后检查该对象是否为null 。 对setPopupPosition的第一次调用可能是多余的，并且有潜在危险。 我猜这是由于不良的重构造成的。

警告2

V6060在对null进行验证之前，已使用'tableModel'引用。 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); } .... }

这种情况类似于前一种情况。 到tableModel对象检查为null时 ，它已经被多次访问。

另一个例子：

  • V6060在对null进行验证之前，已使用'tableModel'引用。 DesktopAbstractTable.java（596），DesktopAbstractTable.java（579）

可能是逻辑错误


V6026该值已经分配给'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); } } .... }

在第一种情况下，已经为变量sortAscending分配了值true ，但稍后仍会再次为其分配相同的值。 这肯定是一个错误，并且作者可能将其值设为false 。

来自不同文件的类似示例：

  • V6026该值已经分配给'sortAscending'变量。 CubaTreeTableWidget.java（444）

返回值奇怪


警告1

V6037循环内无条件的“返回”。 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(....)); } .... }

分析器检测到无条件调用，以在for循环的第一次迭代中返回 。 该行不正确，或者循环应重写为if语句。

警告2

V6014这种方法总是返回一个相同的值，这很奇怪。 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; }

在每种情况下，此函数均返回true ，而最后一行显然要求false 。 看起来像个错误。

以下是其他类似可疑功能的完整列表：

  • V6014这种方法总是返回一个相同的值，这很奇怪。 ErrorNodesFinder.java（31）
  • V6014这种方法总是返回一个相同的值，这很奇怪。 FileDownloadController.java（69）
  • V6014这种方法总是返回一个相同的值，这很奇怪。 IdVarSelector.java（73）
  • V6014这种方法总是返回一个相同的值，这很奇怪。 IdVarSelector.java（48）
  • V6014这种方法总是返回一个相同的值，这很奇怪。 IdVarSelector.java（67）
  • V6014这种方法总是返回一个相同的值，这很奇怪。 IdVarSelector.java（46）
  • V6014这种方法总是返回一个相同的值，这很奇怪。 JoinVariableNode.java（57）

警告3

V6007表达式“ needReload”始终为false。 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; }

该函数返回NeedReload变量，其值始终为false 。 其中一个条件可能缺少一些用于更改该值的代码。

警告4

V6062 “ isFocused”方法内可能进行无限递归。 GwtAceEditor.java（189），GwtAceEditor.java（190）

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

分析仪检测到没有停止条件的递归功能。 该文件包含许多功能，这些功能标有关键字native并包含注释掉的代码。 开发人员现在可能正在重写此文件，并且很快也会注意到isFocused函数。

杂项


警告1

V6002 switch语句未涵盖“ 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; } }; .... }

switch语句没有大小写ADD的情况 。 这是唯一未检查的值，因此开发人员应查看此代码。

警告2

V6021不使用变量“源”。 DefaultHorizo​​ntalLayoutDropHandler.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); } }

声明了变量source ，但未使用。 也许作者忘记了将源代码添加到布局中 ，就像发生在这种类型的另一个变量comp上一样 。

其他具有未使用变量的函数：

  • V6021不使用变量“源”。 DefaultHorizo​​ntalLayoutDropHandler.java（175）
  • V6021该值已分配给'r'变量，但未使用。 ExcelExporter.java（262）
  • V6021不使用变量“ over”。 DefaultCssLayoutDropHandler.java（49）
  • V6021不使用变量“可转移”。 DefaultHorizo​​ntalLayoutDropHandler.java（171）
  • V6021不使用变量“可转移”。 DefaultHorizo​​ntalLayoutDropHandler.java（169）
  • V6021未使用变量'beanLocator'。 ScreenEventMixin.java（28）

警告3

V6054不应按名称对类进行比较。 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; }

分析器已按名称检测到类别比较。 按名称比较类是不正确的，因为根据规范，JVM类的名称仅在包内必须唯一。 这样的比较会产生错误的结果，并导致执行错误的代码。

CUBA平台开发人员的评论


任何大型项目肯定都存在错误。 知道这一点，当PVS-Studio团队提出检查我们的项目时，我们感到非常高兴。 CUBA存储库包含Apache 2许可下的一些第三方OSS库的分支，并且由于分析器在这些源中发现了许多问题，因此我们似乎应该更加注意该代码。 我们目前使用SpotBugs作为我们的主要分析器，它无法注意到PVS-Studio报告的一些大错误。 看来我们应该自己编写一些其他诊断程序。 非常感谢PVS-Studio团队的工作。

开发商还告诉我们，警告V6013和V6054是误报； 他们有意识地决定以自己的方式编写该代码。 该分析仪用于检测可疑的代码片段，发现真正错误的可能性在不同的诊断程序中有所不同。 但是，可以使用特殊的批量警告抑制机制轻松处理此类警告，而无需修改源文件。

此外，PVS-Studio团队无法注意到“似乎我们应该自己编写一些其他诊断信息”这一短语，并且没有这张图片:)

图片3

结论


PVS-Studio可以完美地补充开发过程中使用的现有质量控制工具。 对于拥有数十，数百或数千开发人员的公司而言尤其如此。 PVS-Studio不仅旨在检测错误，而且还可以帮助您修复错误，我的意思不是自动代码编辑，而是可靠的代码质量控制手段。 在大型公司中，不可能每个开发人员都使用不同的工具来检查代码的各个部分，因此对于此类公司来说，更好的解决方案是采用PVS-Studio之类的工具，该工具在每个开发阶段都提供代码质量控制，而不是仅在程序员方面。

Source: https://habr.com/ru/post/zh-CN449020/

More articles:


All Articles