2019 is drawing to an end, and the PVS-Studio team is looking back at the accomplishments of this year. In the beginning of 2019, we enhanced our analyzer's diagnostic capabilities by adding Java support, which enabled us to check and review Java projects as well. We have found lots of bugs over this year, and here's our Top 10 bugs found in Java projects.

No. 10: Signed byte

Source: Analysis of the Apache Dubbo RPC Framework by the PVS-Studio Static Code Analyzer

V6007 Expression 'endKey[i] < 0xff' is always true. OptionUtil.java(32)

public static final ByteSequence prefixEndOf(ByteSequence prefix) {
  byte[] endKey = prefix.getBytes().clone();
  for (int i = endKey.length - 1; i >= 0; i--) {
    if (endKey[i] < 0xff) {                                           // <=
      endKey[i] = (byte) (endKey[i] + 1);
      return ByteSequence.from(Arrays.copyOf(endKey, i + 1));
    }
  }
  return ByteSequence.from(NO_PREFIX_END);
}

Many programmers believe that the byte type is unsigned. This is indeed the case in many programming languages. For example, this is true for C#. But that's not so in Java.

In the endKey[i] < 0xff condition, a variable of type byte is compared with the number 255(0xff) represented in hexadecimal form. The developer probably forgot that the range of the Java byte type is [-128, 127]. This condition will be always true, and the for loop will be always processing only the last element of the endKey array.

No. 9: Two in one

Source: PVS-Studio for Java hits the road. Next stop is Elasticsearch

V6007 Expression '(int)x < 0' is always false. BCrypt.java(429)

V6025 Possibly index '(int) x' is out of bounds. BCrypt.java(431)

private static byte char64(char x) {
  if ((int)x < 0 || (int)x > index_64.length)
    return -1;
  return index_64[(int)x];
}

Discount! One method – two bugs! The first has to do with the char type: since it's unsigned in Java, the (int)x < 0 condition will be always false. The second bug is the ordinary indexing beyond the bounds of the index_64 array when (int)x == index_64.length. This happens because of the condition (int)x > index_64.length. The bug can be fixed by changing the '>' operator to '>=': (int)x >= index_64.length.

No. 8: A solution and its implications

Source: Analyzing the Code of CUBA Platform with PVS-Studio

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 author of the findNextMenuItem method wanted to get rid of the value -1 returned by the indexOf method when the menuItemWidgets list doesn't contain currentItem. To do that, the programmer is adding 1 to the result of indexOf (the menuItemFlatIndex variable) and writing the resulting value to the variable previousMenuItemFlatIndex, which is then used in the method. The adding of -1 proves to be a poor solution because it leads to several errors at once:

The return null statement will never be executed because the previousMenuItemFlatIndex >= 0 expression will be always true; therefore, the findNextMenuItem method will always return from within the if statement;
an IndexOutOfBoundsException will be raised when the menuItemWidgets list has become empty since the program will attempt to access the first element of the empty list;
an IndexOutOfBoundsException will be raised when the currentItem argument has become the last element of the menuItemWidget list.

No. 7: Making a file out of nothing

Source: Huawei Cloud: It's Cloudy in PVS-Studio Today

V6008 Potential null dereference of 'dataTmpFile'. CacheManager.java(91)

@Override
public void putToCache(PutRecordsRequest putRecordsRequest)
{
  .... 
  if (dataTmpFile == null || !dataTmpFile.exists())
  {
    try
    {
      dataTmpFile.createNewFile();  // <=
    }
    catch (IOException e)
    {
      LOGGER.error("Failed to create cache tmp file, return.", e);
      return;
    }
  }
  ....
}

When writing the putToCache method, the programmer made a typo in the condition dataTmpFile == null || !dataTmpFile.exists() before creating a new file dataTmpFile.createNewFile(): they wrote the '==' operator instead of '!='. This typo will lead to throwing a NullPointerException when calling the createNewFile method. This is what the condition looks like with the typo fixed:

if (dataTmpFile != null || !dataTmpFile.exists())

You might think, «Well, okay, we can relax now.» Not yet!

Now that one bug has been fixed, another one showed up. A NullPointerException may be thrown when calling the dataTmpFile.exists() method. To fix this, we need to replace the '||' operator with '&&'. This is the final version of the condition, after all the fixes:

if (dataTmpFile != null && !dataTmpFile.exists())

No. 6: A very weird logic error

Source: PVS-Studio for Java

V6007 [CWE-570] Expression '«0».equals(text)' is always false. ConvertIntegerToDecimalPredicate.java 46

public boolean satisfiedBy(@NotNull PsiElement element) {
  ....
  @NonNls final String text = expression.getText().replaceAll("_", "");
  if (text == null || text.length() < 2) {
    return false;
  }
  if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {// <=
    return false;
  }
  return text.charAt(0) == '0';
}

The interesting thing about this method is that it contains an obvious logic error. If the satisfiedBy method doesn't return after the first if statement, it will mean that the text string is at least two characters long. This also means that the very first check «0».equals(text) in the next if statement is meaningless. What the programmer actually meant by that remains a mystery.

No. 5: What a twist!

Source: PVS-Studio Visits Apache Hive

V6034 Shift by the value of 'bitShiftsInWord — 1' could be inconsistent with the size of type: 'bitShiftsInWord — 1' = [-1… 30]. UnsignedInt128.java(1791)

private void shiftRightDestructive(int wordShifts,
                                   int bitShiftsInWord,
                                   boolean roundUp) 
{
  if (wordShifts == 0 && bitShiftsInWord == 0) {
    return;
  }

  assert (wordShifts >= 0);
  assert (bitShiftsInWord >= 0);
  assert (bitShiftsInWord < 32);
  if (wordShifts >= 4) {
    zeroClear();
    return;
  }

  final int shiftRestore = 32 - bitShiftsInWord;

  // check this because "123 << 32" will be 123.
  final boolean noRestore = bitShiftsInWord == 0;
  final int roundCarryNoRestoreMask = 1 << 31;
  final int roundCarryMask = (1 << (bitShiftsInWord - 1));  // <=
  ....
}

With the input arguments wordShifts = 3 and bitShiftsInWord = 0, the roundCarryMask variable, which stores the result of the bitwise shift (1 << (bitShiftsInWord — 1)), will become a negative number. The developer didn't probably expect that.

No. 4: Can we see the exceptions please?

Source: PVS-Studio Visits Apache Hive

V6051 The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions. ObjectStore.java(9080)

private List<MPartitionColumnStatistics> 
getMPartitionColumnStatistics(....)
throws NoSuchObjectException, MetaException 
{
  boolean committed = false;

  try {
    .... /*some actions*/
    
    committed = commitTransaction();
    
    return result;
  } 
  catch (Exception ex) 
  {
    LOG.error("Error retrieving statistics via jdo", ex);
    if (ex instanceof MetaException) {
      throw (MetaException) ex;
    }
    throw new MetaException(ex.getMessage());
  } 
  finally 
  {
    if (!committed) {
      rollbackTransaction();
      return Lists.newArrayList();
    }
  }
}

The declaration of the getMPartitionColumnStatistics method is misleading as it says it could throw an exception. In reality, whatever exception is generated in the try block, the committed variable will be always false, so the return statement in the finally block will return a value, while all the thrown exceptions will be lost, unable to be processed outside the method. So, none of the exceptions thrown in this method will be able to leave it.

No. 3: Hocus-pocus, or trying to get a new mask

Source: PVS-Studio Visits Apache Hive

V6034 Shift by the value of 'j' could be inconsistent with the size of type: 'j' = [0...63]. IoTrace.java(272)

public void logSargResult(int stripeIx, boolean[] rgsToRead)
{
  ....
  for (int i = 0, valOffset = 0; i < elements; ++i, valOffset += 64) {
    long val = 0;
    for (int j = 0; j < 64; ++j) {
      int ix = valOffset + j;
      if (rgsToRead.length == ix) break;
      if (!rgsToRead[ix]) continue;
      val = val | (1 << j);                // <=
    }
    ....
  }
  ....
}

This bug, too, has to do with a bitwise shift, but not only that. The j variable is used as a counter over the range [0...63] in the inner for loop. This counter participates in a bitwise shift 1 << j. Everything seems OK, but here's where the integer literal '1' of type int (a 32-bit value) comes into play. Because of it, the bitwise shift will start returning the previously returned values when the j variable's value has exceeded 31. If this behavior is not what the programmer wanted, the value 1 must be represented as long: 1L << j or (long)1 << j.

No. 2: Initialization order

Source: Huawei Cloud: It's Cloudy in PVS-Studio Today

V6050 Class initialization cycle is present. Initialization of 'INSTANCE' appears before the initialization of 'LOG'. UntrustedSSL.java(32), UntrustedSSL.java(59), UntrustedSSL.java(33)

public class UntrustedSSL {
  private static final UntrustedSSL INSTANCE = new UntrustedSSL();
  private static final Logger LOG = LoggerFactory.getLogger(UntrustedSSL.class);
  .... 
  private UntrustedSSL() 
  {
    try
    {
      ....
    }
    catch (Throwable t) {
      LOG.error(t.getMessage(), t);           // <=
    }
  }
}

The order in which fields are declared in a class makes a difference since they are initialized in the same order they are declared. Forgetting this fact leads to elusive bugs like the one above.

The analyzer points out that the static field LOG is dereferenced in the constructor at the moment when it is initialized to the value null, which leads to throwing a series of exceptions NullPointerException -> ExceptionInInitializerError.

But why does the static field LOG have the value null at the moment of calling the constructor?

The ExceptionInInitializerError is the clue. The constructor is initializing the static field INSTANCE, which is declared before the LOG field. That's why the LOG field is not initialized yet when the constructor is called. To make this code work properly, the LOG field must be initialized before the call.

First: copy-paste oriented programming

Source: Apache Hadoop Code Quality: Production VS Test

V6072 Two similar code fragments were found. Perhaps, this is a typo and 'localFiles' variable should be used instead of 'localArchives'. LocalDistributedCacheManager.java(183), LocalDistributedCacheManager.java(178), LocalDistributedCacheManager.java(176), LocalDistributedCacheManager.java(181)

public synchronized void setup(JobConf conf, JobID jobId) throws IOException {
  ....
  // Update the configuration object with localized data.
  if (!localArchives.isEmpty()) {
    conf.set(MRJobConfig.CACHE_LOCALARCHIVES, StringUtils
        .arrayToString(localArchives.toArray(new String[localArchives  // <=
            .size()])));
  }
  if (!localFiles.isEmpty()) {
    conf.set(MRJobConfig.CACHE_LOCALFILES, StringUtils
        .arrayToString(localFiles.toArray(new String[localArchives     // <=
            .size()])));
  }
  ....
}

The first place on our Top 10 list is awarded to copy-paste, or, rather, a bug that stems from the careless use of this technique. The second if statement looks very much like a copy of the first, with some of the variables modified:

localArchives was changed to localFiles;
MRJobConfig.CACHE_LOCALARCHIVES was changed to MRJobConfig.CACHE_LOCALFILES.

But the programmer managed to make a mistake even in this simple operation: the if statement in the line pointed out by the analyzer is still using the localArchives variable instead of the apparently intended localFiles variable.

Conclusion

Fixing bugs found at the later development stages or after the release takes a good deal of resources. The static analyzer PVS-Studio allows you to detect bugs at the coding stage, thus making it much easier and cheaper. Many companies have already made their developers' lives easier by starting to use the analyzer on a regular basis. If you want to really enjoy your job, try PVS-Studio.

We are not going to stop at that and are planning to keep improving and enhancing our tool. Stay tuned for new diagnostics and articles with even more interesting bugs in the next year.

I see you like adventures! First, you defeated top 10 bugs in С# project in 2019 and now you coped with Java as well! Welcome to the next level — the article about best errors in C++ projects in 2019.

Top 10 Bugs Found in Java Projects in 2019