Java developers have access to a number of useful tools that help to write high-quality code such as the powerful IDE IntelliJ IDEA, free analyzers SpotBugs, PMD, and the like. The developers working on CUBA Platform have already been using all of these, and this review will show how the project can benefit even more from the use of the static code analyzer PVS-Studio.
CUBA Platform is a high-level framework for enterprise application development. The platform abstracts developers from underlying technologies so they can focus on the business tasks, while retaining full flexibility by providing unrestricted access to low-level code. The source code was downloaded from GitHub.
PVS-Studio is a tool for detecting bugs and potential security vulnerabilities in the source code of programs written in C, C++, C#, and Java. The analyzer runs on 64-bit Windows, Linux, and macOS systems. To make things easier for Java programmers, we developed plugins for Maven, Gradle, and IntelliJ IDEA. I checked the project using the Gradle plugin, and it went off without a hitch.
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));
}
....
}The attribute value extracted from the element is not checked. Instead, the isNotEmpty function gets a string literal as its argument rather than the variable handleTabKey.
A similar error found in the file AbstractTableLoader.java:
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 will return -1 if the element is not found in the list. The value 1 is then added to the index, which disguises the problem with the absent element. Another potential problem has to do with the fact that the previousMenuItemFlatIndex variable will always be greater than or equal to zero. For example, if the menuItemWidgets list is found to be empty, the program will end up with an array overrun.
Warning 3
V6009 The 'delete' function could receive the '-1' value while 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 ");
}
....
}The last two characters of the orderBy buffer are deleted if the total number of elements is greater than zero, i.e. if the string contains at least one character. However, the start position from where the deletion begins is offset by 2. So, if orderBy happens to contain one character, attempting to delete it will raise a StringIndexOutOfBoundsException.
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 masterCollection. One line earlier, the collections have been compared by reference, but the programmer probably intended it to be a comparison 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 case is similar to the previous one. The collections are compared in the isCollectionValuesChanged function, and reference comparison is perhaps not what was intended here either.
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)))) {
....
}
....
}The second condition checks an expression that is opposite to the one checked in the first condition. The latter can, therefore, be safely removed to shorten 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 leaving the while loop, the value of the connector variable won't be equal to null, so the redundant check can be deleted.
Another suspicious warning of this type that needs to be examined:
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 prevents execution of the call of tx1.commit. Those two lines should be swapped for the code to work properly.
There were a few similar problems in other tests too:
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 you pass along with the password to a hash function. It is mainly used to protect the program against dictionary attacks and rainbow table attacks, as well as to obscure identical passwords. More here: Salt (cryptography).
In this function, the passed string is overwritten right after entering. Ignoring the value passed to the function is a potential vulnerability.
Warning 2
This function triggered 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);
}That's quite a curious snippet. The function is called with only two variables as arguments, offsetWidth and offsetHeight, and 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 function doesn't make use of the value passed as the shortcut parameter. Maybe the function's interface has become obsolete, or this warning is just a false positive.
A few more defects of this type:
Warning 1
V6032 It is odd that the body of method 'firstItemId' is fully equivalent to the body of another method '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();
}The functions firstItemId and lastItemId have the same implementations. The latter was probably meant to get the index of the last element rather than get the element at index 0.
Warning 2
V6032 It is odd that the body of method is fully equivalent to the body of another 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 suspiciously identical bodies. My guess is that one of them was meant to work with some other color instead of color53.
Warning 1
V6060 The 'descriptionPopup' reference was utilized 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);
}
}In just two lines, the programmer managed to write a highly suspicious piece of code. First the method setPopupPosition of the object descriptionPopup is called, and then the object is checked for null. The first call to setPopupPosition is probably redundant and potentially dangerous. I guess it results from bad refactoring.
Warning 2
V6060 The 'tableModel' reference was utilized 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);
}
....
}This case is similar to the previous one. By the time the tableModel object is checked for null, it has already been accessed multiple times.
Another example:
V6026 This value is already 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 variable sortAscending has already been assigned the value true, but it's still assigned the same value again later on. This must be a mistake, and the author probably meant the value false.
A similar example from a different file:
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 has detected an unconditional call to return at the very first iteration of a for loop. Either that line is incorrect or the loop should be rewritten as an if statement.
Warning 2
V6014 It's odd that this method always returns one and the same value. 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 each case, while the last line obviously calls for false. It looks like a mistake.
Here's a full list of other similar suspicious functions:
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 the needReload variable whose value is always false. Some code for changing that value is probably missing from one of the conditions.
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 has detected a recursive function with no stop condition. This file contains a lot of functions marked with the keyword native and containing commented-out code. The developers are probably rewriting this file now and will soon notice the isFocused function too.
Warning 1
V6002 The switch statement does not cover all values of the 'Operation' 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 has no case for the value ADD. It's the only value that's not being checked, so the developers should take a look at this code.
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 variable source is declared but not used. Perhaps the authors forgot to add source to layout, just like it happened with another variable of this type, comp.
Other 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 has detected a class comparison by name. It's incorrect to compare classes by name as, according to the specification, the names of JVM classes must be unique only within a package. Such a comparison yields incorrect results and leads to executing the wrong code.
Any large project surely has bugs in it. Knowing that, we gladly agreed when the PVS-Studio team offered to check our project. The CUBA repository contains forks of some of the third-party OSS libraries licensed under Apache 2, and it looks like we should pay more attention to that code as the analyzer found quite a number of problems in those sources. We currently use SpotBugs as our primary analyzer, and it fails to notice some of the big bugs reported by PVS-Studio. It seems we should write some additional diagnostics ourselves. Many thanks to the PVS-Studio team for the job.
The developers also told us that the warnings V6013 and V6054 were false positives; it was their conscious decision to write that code the way they did. The analyzer is designed for detecting suspicious code fragments, and the probability of finding genuine bugs varies across different diagnostics. Such warnings, however, can be easily handled using the special mass warning suppression mechanism without having to modify the source files.
Also, the PVS-Studio team cannot take no notice of the phrase "it seems we should write some additional diagnostics ourselves" and do without this picture :)
PVS-Studio can be a perfect complement to existing quality-control tools used in your development process. It's especially true for companies with dozens, hundreds, or thousands of developers. PVS-Studio is designed not only to detect bugs but also to help you fix them, and what I mean by that is not automatic code editing but reliable means of code quality control. In a large company, it's impossible for every developer to check their respective parts of the code with different tools, so a better solution for such companies would be to adopt tools like PVS-Studio, which provide code quality control at every development stage, not only on the programmer side.