Geekly articles weekly

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

图片2

对于Java程序员，有一些有用的工具可以帮助您编写高质量的代码，例如强大的IntelliJ IDEA开发环境，免费的SpotBugs，PMD分析器等。 所有这些都已在CUBA Platform项目的开发中使用，并且在对发现的代码缺陷的回顾中，我将告诉您如何使用PVS-Studio静态代码分析器来改善项目的质量。

关于项目和分析仪


CUBA平台是一个高级Java框架，用于通过完整的Web界面快速创建企业应用程序。 该平台使开发人员从异构技术中抽象出来，这样您就可以同时专注于解决业务问题，而又不可能直接与他们合作。 源代码来自GitHub上的存储库。

PVS-Studio是用于检测用C，C ++，C＃和Java编写的程序的源代码中的错误和潜在漏洞的工具。 它适用于Windows，Linux和macOS上的64位系统。 为了方便Java程序员，我们为Maven，Gradle和IntelliJ IDEA开发了插件。 使用Gradle插件可以轻松地验证CUBA Platform项目。

条件错误


警告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 。 然后将一个添加到索引，从而在缺少所需元素时隐藏情况。 另一个潜在的问题可能是这样的事实，即变量previousMenuItemFlatIndex将始终大于或等于零。 例如，如果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个字符的总数大于零（即大于2），则将其删除。 字符串包含一个或多个字符。 但是删除字符的起始位置设置为偏移2个字符。 因此，如果orderBy突然由1个字符组成，则尝试删除将抛出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循环的第一次迭代中检测到对return语句的无条件调用。 也许这是一个错误，或者您需要重写代码以使用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); } }

源变量已声明且未在代码中使用。 也许，就像另一个相同类型的comp变量一样，他们忘记了将源代码添加到layout 。

具有未使用变量的更多函数：

  • 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团队不能忽略“现在该去自己写其他支票了”这个短语了，不要离开这张图片:)

图片1

结论


PVS-Studio将是您现有项目的重要补充，以改进代码质量工具。 尤其是如果有数十，数十万和数千名员工。 PVS-Studio不仅可以发现错误，还可以对其进行修复。 此外，我们不是在谈论自动代码编辑，而是在谈论可靠的代码质量控制。 在大型公司中，无法想象会有绝对的所有开发人员都将使用各种工具独立检查其代码的情况，因此PVS-Studio之类的工具更适合此类公司，因为这些公司在开发的各个阶段都对代码进行质量控制，而不仅仅是普通程序员。



如果您想与说英语的读者分享这篇文章，请使用以下链接：Svyatoslav Razmyslov。 使用PVS-Studio分析CUBA平台的代码

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

More articles:


All Articles