# Top 10 bugs found in Java projects in 2019

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\.

![0699_Best_10_Java_errors_2019/image1.png](https://import.viva64.com/docx/blog/0699_Best_10_Java_errors_2019/image1.png)

## No\. 10: Signed byte

Source: [Analysis of the Apache Dubbo RPC framework by the PVS\-Studio static code analyzer](https://pvs-studio.com/en/blog/posts/java/0663/)

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'endKey\[i\] < 0xff' is always true\. OptionUtil\.java\(32\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0621/)

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression '\(int\)x < 0' is always false\. BCrypt\.java\(429\)

[V6025](https://pvs-studio.com/en/docs/warnings/v6025/) Possibly index '\(int\) x' is out of bounds\. BCrypt\.java\(431\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0626/)

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'previousMenuItemFlatIndex \>\= 0' is always true\. CubaSideMenuWidget\.java\(328\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0688/)

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'dataTmpFile'\. CacheManager\.java\(91\)

```cpp
@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: 

```cpp
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:

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

## No\. 6: A very weird logic error

Source: [PVS\-Studio for Java](https://pvs-studio.com/en/blog/posts/java/0603/)

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) \[CWE\-570\] Expression '"0"\.equals\(text\)' is always false\. ConvertIntegerToDecimalPredicate\.java 46

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0657/)

[V6034](https://pvs-studio.com/en/docs/warnings/v6034/) Shift by the value of 'bitShiftsInWord — 1' could be inconsistent with the size of type: 'bitShiftsInWord — 1' \= \[\-1\.\.\. 30\]\. UnsignedInt128\.java\(1791\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0657/)

[V6051](https://pvs-studio.com/en/docs/warnings/v6051/) The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions\. ObjectStore\.java\(9080\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0657/)

[V6034](https://pvs-studio.com/en/docs/warnings/v6034/) Shift by the value of 'j' could be inconsistent with the size of type: 'j' \= \[0\.\.\.63\]\. IoTrace\.java\(272\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0688/)

[V6050](https://pvs-studio.com/en/docs/warnings/v6050/) Class initialization cycle is present\. Initialization of 'INSTANCE' appears before the initialization of 'LOG'\. UntrustedSSL\.java\(32\), UntrustedSSL\.java\(59\), UntrustedSSL\.java\(33\)

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0697/)

[V6072](https://pvs-studio.com/en/docs/warnings/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\)

```cpp
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](https://pvs-studio.com/en/about/customers/) 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](https://pvs-studio.com/en/pvs-studio/download/)\.

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 C\# project in 2019](https://pvs-studio.com/en/blog/posts/csharp/0698/) and now you coped with Java as well\! Welcome to the next level \- the article about [best errors in C\+\+ projects in 2019](https://pvs-studio.com/en/blog/posts/cpp/0700/)\.

![0699_Best_10_Java_errors_2019/image2.png](https://import.viva64.com/docx/blog/0699_Best_10_Java_errors_2019/image2.png)