Geekly Articles each Day
📜 ⬆️ ⬇️

Analysis of the CUBA Platform code using PVS-Studio

Picture 2

For Java programmers, there are useful tools to help write quality code, for example, the powerful IntelliJ IDEA development environment, free SpotBugs, PMD analyzers and others. All this is already used in the development of the CUBA Platform project, and in this review of the found code defects I will tell you how else you can improve the quality of the project using the PVS-Studio static code analyzer.

About project and analyzer


The CUBA Platform is a high-level Java framework for quickly building enterprise applications with a full-fledged web interface. The platform abstracts the developer from heterogeneous technologies so that you can focus on solving business problems, at the same time, without depriving you of the opportunity to work with them directly. The source code is taken from the repository on GitHub .

PVS-Studio is a tool for detecting errors and potential vulnerabilities in the source code of programs written in C, C ++, C # and Java. Works in 64-bit systems on Windows, Linux and macOS. For the convenience of Java programmers, we have developed plug-ins for Maven, Gradle and IntelliJ IDEA. The CUBA Platform project was easily checked using the Gradle plugin.

Errors in conditions


Warning 1
')
V6007 Expression 'StringUtils.isNotEmpty ("handleTabKey")' is always true. SourceCodeEditorLoader.java (60)

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

After extracting an attribute value from an element, it does not check this value. Instead, a constant string is passed to the isNotEmpty function, but you had to pass the variable handleTabKey .

There is another similar error in the AbstractTableLoader.java file:


Warning 2

V6007 Expression 'previousMenuItemFlatIndex> = 0' is always 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; }

The indexOf function may return -1 if no item is found in the list. Then a unit is added to the index, thus hiding the situation when the required element is missing. Another potential problem is the fact that the variable previousMenuItemFlatIndex will always be greater than or equal to zero. If, for example, the menu list of menuItemWidgets is empty, then it becomes possible to go beyond the array boundary.

Warning 3

V6009 The 'delete' function for the '-1' value for non-negative value is expected. Inspect argument: 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 "); } .... }

In the orderBy character buffer, the last 2 characters are deleted if their total number is greater than zero, i.e. string contains one character or more. But the starting position for deleting characters was set with an offset of 2 characters. Thus, if orderBy suddenly consists of 1 character, an attempt to delete will result in a StringIndexOutOfBoundsException exception.

Warning 4

V6013 Objects 'masterCollection' and 'entities' are compared by reference. Possibly an equality comparison was intended. 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); } } }

In the updateMasterCollection function , the values ​​from entities are copied to the masterCollection . Before this, the collections were compared by reference, but perhaps they were planned to be compared by value.

Warning 5

V6013 Objects 'value' and 'oldValue' are compared by reference. Possibly an equality comparison was intended. WebOptionsList.java (278)

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

This example is similar to the previous one. Here isCollectionValuesChanged is designed to compare collections. Perhaps the comparison by reference is also not the way that was expected.

Excess conditions


Warning 1

V6007 Expression 'mask.charAt (i + offset)! = PlaceHolder' is always 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)))) { .... } .... }

In the second comparison, an expression that is the opposite of the first is tested. The second check can be deleted, which simplifies the code.

V6007 Expression 'connector == null' is always 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; }

After the end of the while loop , the value of the connector variable will not be null , therefore, redundant checking can be removed.

Another suspicious place that developers should pay attention to:


Unreachable code in tests


V6019 Unreachable code detected. It is possible that an error is present. 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(); } }

The throwException function throws an exception that does not make a call to the tx1.commit function . Perhaps these lines should be swapped.

A few more similar places in other tests:


Suspicious Function Arguments


Warning 1

V6023 Parameter 'salt' is always rewritten in method body before being used. BCryptEncryptionModule.java (47)

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

In cryptography, salt is a data string that is passed to a hash function along with a password. It is mainly used to protect against dictionary enumeration and attacks using rainbow tables, as well as hiding identical passwords. Read more: Salt (cryptography) .

In this function, the string is ground immediately upon entering the function. It is possible that ignoring the transmitted value is a potential vulnerability.

Warning 2

For the considered function, the analyzer generates two warnings at once:



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

Fun code. The function accepts only two variables: offsetWidth and offsetHeight , both are overwritten before use.

Warning 3

V6022 Parameter 'shortcut' is not used inside constructor body. 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); }

The value of the shortcut parameter is not used in the function. The function interface may be outdated or this warning is not an error.

Some more similar places:


Different functions with the same code


Warning 1

V6032 It is odd that the body of the method of first is oneItemId it is fully equivalent. 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(); }

The functions firstItemId and lastItemId have the same implementation. Most likely, in the latter it was necessary to get the element not with the index 0 , but to calculate the index of the last element.

Warning 2

V6032 It is an odd method. 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); }

Two more functions with a suspiciously identical implementation. I would venture to suggest that in one of them it was necessary to use a different color, different from color53 .

Contact by zero link


Warning 1

V6060 The 'descriptionPopup' reference was used before it was verified against 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); } }

The author managed to write a very suspicious code in just two lines. First, the setPopupPosition method is called on the descriptionPopup object, and then the object is compared to null . Most likely, the first call to the setPopupPosition function is redundant and dangerous. Looks like the consequences of a failed refactoring.

Warnings 2

V6060 The 'tableModel' reference was used before it was verified against null. 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); } .... }

A similar situation in this function. After numerous calls to the tableModel object, it is checked whether it is null or not.

One more example:


Perhaps a logical error


V6026 This value is assigned to the 'sortAscending' variable. 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); } } .... }

In the first condition, the sortAscending variable is true anyway, but it is still assigned the same value. Perhaps this is an error and wanted to assign false .

A similar example from another file:


Strange function return values


Warning 1

V6037 An unconditional 'return' within a 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(....)); } .... }

The analyzer detected an unconditional call to the return operator at the first iteration of the for loop. Perhaps there is an error, or you need to rewrite the code to use the if statement

Warning 2

V6014 This is one of those values . 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; }

This function returns true in all cases. But here, in the very last line, the return value is false . Perhaps there is a mistake.

The entire list of suspicious functions with similar code:


Warning 3

V6007 Expression 'needReload' is always 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; }

The function returns a needReload variable, the value of which is always false . Most likely, in one of the conditions they forgot to add a code for changing the value of a variable.

Warning 4

V6062 Possible infinite recursion inside the 'isFocused' method. GwtAceEditor.java (189), GwtAceEditor.java (190)

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

The analyzer detected a function that is called recursively without the condition of stopping recursion. There are many functions in this file that are labeled with the native keyword and contain commented out code. Most likely, the file is currently being rewritten and soon the developers will pay attention to the isFocused function.

Miscellaneous Warnings


Warning 1

V6002 The switch statement enum: 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; } }; .... }

The switch statement does not consider the value of ADD . It is the only one not considered, so it’s worth checking whether it’s a mistake or not.

Warning 2

V6021 Variable 'source' is not used. 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); } }

The code declares and does not use the source variable. Perhaps, like the other comp variable of the same type, the source forgot to add to the layout .

More functions with unused variables:


Warning 3

V6054 Classes should not be compared by their name. 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; }

The analyzer detected a situation when classes are compared by name. Such a comparison is incorrect, because, according to the specification, JVM classes have a unique name only inside the package. This may cause incorrect comparison and execution of the wrong code that was planned.

CUBA Platform Developer Feedback


Of course, in any large project there are bugs. That is why we gladly agreed to the proposal of the PVS-Studio team to check our project. The forks of some third-party OSS libraries under the Apache 2 license are included in the CUBA repository, and it seems that we need to pay more attention to this code, the analyzer has found quite a lot of problems in these sources. Now we use SpotBugs as the main analyzer, and it does not find some significant problems found by PVS-Studio. It's time to go and write additional checks yourself. Many thanks to the PVS-Studio team for the work done.

Also, the developers noted that the warnings V6013 and V6054 are false. The code was written this way. The static analyzer is designed to detect suspicious code fragments and the probability of finding errors in all inspections is different. Nevertheless, it is easy to work with such warnings using a convenient mechanism for mass suppression of analyzer warnings without modifying the source code files.

Another team PVS-Studio cannot pass by the phrase “it's time to go and write additional checks by ourselves” and not leave this picture :)

Picture 1

Conclusion


PVS-Studio will be a great addition to your project to already existing tools for improving the quality of the code. Especially if there are dozens, hundreds and thousands of employees on staff. PVS-Studio is intended not only for finding errors, but also for correcting them. And this is not about automatic code editing, but about reliable code quality control. In a large company, it is impossible to imagine a situation where absolutely all developers will independently check their code with various tools, therefore, such companies are more suitable for tools like PVS-Studio, where the quality control of the code is provided at all stages of development, not just from an ordinary programmer.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analyzing the Code of CUBA Platform with PVS-Studio

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

More articles:


All Articles