Top 10 most intriguing Java errors in 2024

• Foreword
• 10th place. Back to the future
• 9th place. "Does not seem to work"
• 8th place. Shot in the foot
• 7th place. Even great mathematicians make mistakes
• 6th place. This pattern doesn't work
• 5th place. Micro-optimization
• 4th place. The test won't fall if it doesn't work
• 3rd place. Happy debugging, folks
• 2nd place. When patterns failed
• 1st place. One error offset another
• Conclusion

In 2024, we've analyzed a wealth of projects, sharing our discoveries on our blog. Now it's New Year's Eve—it's time to tell festive tales! We've collected the most intriguing Java errors detected in open-source projects and now bring them to you!

Foreword

We've long upheld the tradition of posting the most interesting bugs detected with PVS-Studio, but there hasn't been a Java-related top since 2020! This time, I've tried to resurrect the vintage rubric. I hope you have a cozy blanket and a warm cup of tea at hand because I've picked over 10 entertaining bugs just for you. Here's how they were ranked:

• my personal opinion;
• an interesting background of the bug;
• diversity, credibility, and non-triviality.

Get ready for 10 entertaining stories with their own programming wisdom—New Year's Eve is coming soon after all :)

10th place. Back to the future

In the tenth place, the first code snippet welcomes us with open arms.

public Builder setPersonalisation(Date date, ....) {
  ....
  final OutputStreamWriter
    out = new OutputStreamWriter(bout, "UTF-8");
  final DateFormat
    format = new SimpleDateFormat("YYYYMMdd");
  out.write(format.format(date));
    ....
}

I couldn't help but include it in the New Year's Eve top because a bug in this code allows us to travel to the next year faster :) Guess where the bug stems from?

Let's take a look at the argument passed to the SimpleDateFormat constructor. Does it seem valid? If we pass there almost any date, like the date of writing this article (10/12/2024), the code will return the correct value, 20241210.

However, if we pass 29/12/2024, it'll return 20251229 instead, thus ingeniously shifting into the new year early. Btw, traveling back in time is also doable.

This happens because the Y character in the argument to SimpleDateFormat represents the year based on the week number. In brief, a week is considered as the first one when it includes at least four days of the new year. So, if our week starts on Sunday, we can jump into the new year three days early.

To fix it, just replace an uppercase Y with a lowercase y. Want to learn more? We've dedicated a whole post to this topic.

Here's a PVS-Studio warning for this error:

V6122 Usage of 'Y' (week year) pattern was detected: it was probably intended to use 'y' (year). SkeinParameters.java 246

Due to the specifics of the week number, so testing isn't the best pal to find this error. So why does such a topical bug come in the last place? The reason is that the warning isn't from the actual version of Bouncy Castle but from our testing base. The old sources still remain there, and this bug has been fixed for a long time. This is such a salute from the past, just a time travel again :)

9th place. "Does not seem to work"

In the ninth place, we have the warning from the GeoServer analysis:

@Test
public void testStore() {
  Properties newProps = dao.toProperties();
  
  // ....
  Assert.assertEquals(newProps.size(), props.size());
  for (Object key : newProps.keySet()) {
    Object newValue = newProps.get(key);
    Object oldValue = newProps.get(key);              // <=
    Assert.assertEquals(newValue, oldValue);
  }
}

Here's the PVS-Studio warning:

V6027 Variables 'newValue', 'oldValue' are initialized through the call to the same function. It's probably an error or un-optimized code. DataAccessRuleDAOTest.java 110, DataAccessRuleDAOTest.java 111

What's interesting about such an error? Let me reveal what's hidden behind the four dots:

@Test
public void testStore() {
  Properties newProps = dao.toProperties();

  // properties equality does not seem to work...
  Assert.assertEquals(newProps.size(), props.size());
  for (Object key : newProps.keySet()) {
    Object newValue = newProps.get(key);
    Object oldValue = newProps.get(key);
    Assert.assertEquals(newValue, oldValue);
  }
}

There's a comment that the code isn't working for some reason. Honestly, it made me chuckle the first time I saw it.

The comment is rather ambiguous, though. It's likely that the test has been written that way intentionally to prevent a failure while the comparison is down. However, the code has been in this state for over a decade, which raises some questions. That ambiguity is why I don't rank it higher.

8th place. Shot in the foot

If we can't call bugs from JBullet as shots in the foot, I don't know which ones we can call so. Here's an error from the article:

@Override
public BroadphasePair findPair(BroadphaseProxy proxy0, BroadphaseProxy proxy1) {
  BulletStats.gFindPairs++;
  if (proxy0.getUid() > proxy1.getUid()) {
    BroadphaseProxy tmp = proxy0;
    proxy0 = proxy1;
    proxy1 = proxy0;
  }
  ....
}

I think we don't even need the PVS-Studio warning to spot where the error is. Anyway, just in case, here it is:

V6026 This value is already assigned to the 'proxy1' variable. HashedOverlappingPairCache.java 233

Yes, it's an embarrassingly simple error. That simplicity makes it even more hilarious, though. Nevertheless, it has its own story.

The JBullet library is a port from the C/C++ bullet library, and there's a similar function there:

template <typename T>
B3_FORCE_INLINE void b3Swap(T &a, T &b)
{
  T tmp = a;
  a = b;
  b = tmp;
}

It's easy to see that this code is written correctly. Judging by the git blame, it's originally written correctly. It turns out that the error occured when the code was ported from one language to another.

For its striking unpretentiousness coupled with a rich history, I award this warning with the eighth place. I hope you've enjoyed that the shot-in-the-foot bug turned out to be C++-related.

7th place. Even great mathematicians make mistakes

Admittedly, the next warning warms my heart for many reasons. Here's a code snippet from the GeoGebra check:

@Override
final public void update() {
  ....
  switch (angle.getAngleStyle()) {
    ....
    case EuclidianStyleConstants.RIGHT_ANGLE_STYLE_L:
      // Belgian offset |_
      if (square == null) {
        square = AwtFactory.getPrototype().newGeneralPath();
      } else {
        square.reset();
      }
      length = arcSize * 0.7071067811865;
      double offset = length * 0.4;
      square.moveTo(
          coords[0] + length * Math.cos(angSt)
              + offset * Math.cos(angSt)
              + offset * Math.cos(angSt + Kernel.PI_HALF),
          coords[1]
              - length * Math.sin(angSt)
                  * view.getScaleRatio()
              - offset * Math.sin(angSt)
              - offset * Math.sin(angSt + Kernel.PI_HALF));
      ....
      break;
    }
}

Try to find the error yourself! To keep you from peeking, I hid the warning and explanation in a spoiler.

6th place. This pattern doesn't work

The following warning, which I've taken from the first article based on the DBeaver check, may not immediately catch our attention. Here's a code fragment:

private List<DBTTaskRun> runs;
....
private void loadRunsIfNeeded() {
    if (runs == null) {
        synchronized (this) {
            if (runs == null) {
                runs = new ArrayList<>(loadRunStatistics());
            }
        }
    }
}

Here's what the PVS-Studio analyzer detected:

V6082 Unsafe double-checked locking. The field should be declared as volatile. TaskImpl.java 59, TaskImpl.java317

While there's nothing special about this particular warning, I still find it very intriguing. The point is that the applied Double-Checked Locking pattern doesn't work. What's the trick? This was relevant 20 years ago :)

If you want to read more about the topic, I suggest reading the full article. But for now, let me give you a quick summary.

The Double-Checked Locking pattern is used to implement delayed initialization in multithreaded environments. Before the "heavyweight" check, the "lightweight" one is executed without a synchronization block. The resource is created only when both checks pass.

However, in this approach, the object creation is non-atomic and the processor and compiler can change the operation order. Thus, another thread may accidentally receive a partially created object and start working with it, which can lead to incorrect behavior. This error may rarely occur, so debugging will be a huge challenge for devs.

Here's a twist: this pattern didn't work until JDK 5. Starting with JDK 5, the volatile keyword was introduced to solve the potential issue of reordering operations thanks to the happens-before principle. The analyzer warns us that we should add this keyword.

However, it'd be better to avoid this pattern anyway. The hardware and JVM performance has progressed a long way since then, and the synchronized operations aren't so slow anymore. However, incorrectly implementing the DCL pattern remains a common pitfall, with potentially serious consequences described above. It confirms the fact that our analyzer still finds such negligent errors in old projects.

5th place. Micro-optimization

The fifth place takes another DBeaver warning about which we've dedicated an article. Let's take a look at it:

@Override
public boolean canEditObject(ExasolTableColumn object) {
    ExasolTableBase exasolTableBase = object.getParentObject();
    if (exasolTableBase != null &
        exasolTableBase.getClass().equals(ExasolTable.class)) {
        return true;
    } else {
        return false;
    }
}

Here's an explanation:

V6030 The method located to the right of the '&' operator will be called regardless of the value of the left operand. Perhaps, it is better to use '&&'. ExasolTableColumnManager.java 79, DB2TableColumnManager.java 77

The developer has mixed up the logical && with the bitwise &. They have different behavior: the conditions in the expression aren't terminated after the bitwise AND. The short-circuit evaluation doesn't work for bitwise AND. So, even though exasolTableBase != null will return false, the execution thread will reach exasolTableBase.getClass() and lead to an NPE.

Okay, it's just a typo, let's move on, shall we? DBeaver has a lot of such warnings. A lot. Many are relatively harmless, but for curious readers, I've left a few examples below:

After digging deeper, my team has assumed that the developers might have been trying to micro-optimize performance. For a full picture, you can check out our article—here I introduce a summary.

The key point is that bitwise operations don't rely on branch predictions, potentially allowing for faster execution compared to logical operations.

To our surprise, a home-grown benchmark supports this claim:

The chart illustrates the time required for each operation type. If we trust it, the bitwise operations appear to be 40% faster than logical operations.

Why do I raise this topic? To highlight the cost of potential micro-optimizations.

First, developers invented the branch prediction for a reason—it'd be too expensive to abandon it. Thus, the benchmark probably works faster because values have a normal distribution, which is unlikely to be observed in real cases.

Second, abandoning the short-circuit evaluation mechanism can lead to significantly higher costs. If we look at the third example from the spoiler above, we can see that not the fastest contains operation will execute all the times instead of stopping promptly.

Third, we give carte blanche for such errors from the start of the chapter.

Overall, I find the cautionary tale of the micro-optimization price compelling enough to open our top five.

4th place. The test won't fall if it doesn't work

Automated tests are often considered as the ultimate safeguard against various errors. However, every so often, I'm tempted to ask: "Who tests the tests themselves?" Another warning from the GeoServer check demonstrates this once again. Here's a code fragment:

@Test
public void testChallenge() throws Exception {
  Map<String, Object> raw = getWorld();
  try {
    executeGetCoverageKvp(raw);
    fail("This should have failed with a security exception");
  } catch (Throwable e) {
    // make sure we are dealing with some security exception
    Throwable se = null;
    while (e.getCause() != null && e.getCause() != e) {         // <=
      e = e.getCause();
      if (SecurityUtils.isSecurityException(e)) {
        se = e;
      }
    }
  
    if (e == null) {                                            // <=
      fail("We should have got some sort of SpringSecurityException");
    } else {
      // some mumbling about not having enough privileges
      assertTrue(se.getMessage().contains("World"));
      assertTrue(se.getMessage().contains("privileges"));
    }
  }
}

The PVS-Studio warning:

V6060 The 'e' reference was utilized before it was verified against null. ResourceAccessManagerWCSTest.java 186, ResourceAccessManagerWCSTest.java 193

At first glance, this warning might not seem like the analyzer most exciting one because V6060 is often issued for redundant code. Yet, I promised that I'll select nominations based on their appeal. So, this case is far more interesting than it appears.

Initially, the test logic may seem wrong because the e variable is obtained from the catch operator and remains unchanged further, so it'll never be null. We can just make a stray edit and remove the then branch of the if(e == nul) condition as unreachable. Yet, that would be radically wrong. Have you figured out the catch yet?

The key lies in one more variable in the code that contains the exception object, it's a se. It has a value that changes within the loop body. So, we can easily guess that in the condition, there should be the se variable instead of e.

This error will cause that the then branch will never be executed, so we won't know that there's no exception. What's worse, it's rather difficult to notice such an error on a code review because the variable names are painfully similar.

There are two wisdoms to be gleaned from this story:

• Name variables clearly, even in tests. Otherwise, it'll be easier to make such mistakes;
• Tests aren't enough to guarantee the project quality because they can also contain errors. Thus, it leaves loopholes for bugs within the app.

For delivering such valuable lessons, I award this warning with the fourth place.

3rd place. Happy debugging, folks

The top three winners belong to the warning from the NetBeans check. Previous code snippets were relatively compact, so let's look at lengthy one:

private void mergeParallelInclusions(List<IncludeDesc> inclusions,
                                     IncludeDesc original,
                                     boolean preserveOriginal) {
  IncludeDesc best = null;
  ....
  // 2nd remove incompatible inclusions, move compatible ones to same level
  for (Iterator it=inclusions.iterator(); it.hasNext(); ) {
    IncludeDesc iDesc = (IncludeDesc) it.next();
    if (iDesc != best) {
      if (!compatibleInclusions(iDesc, best, dimension)) {
        it.remove();
      } else if (iDesc.parent == best.parent &&
                 iDesc.neighbor == best.neighbor &&
                (iDesc.neighbor != null || iDesc.index == iDesc.index)) {
        it.remove(); // same inclusion twice (detect for better robustness)
      }
      ....
    }
    ....
  }
  ....
  if (unifyGaps != null) {
    // unify resizability of the border gaps collected for individual inclusions
    for (LayoutInterval[] gaps : unifyGaps) {
      int preferredFixedSide =
                         fixedSideGaps[LEADING] >= fixedSideGaps[TRAILING] ?
                                                   LEADING : TRAILING;
      for (int i=LEADING; i <= TRAILING; i++) {
        if (LayoutInterval.canResize(gaps[i]) && !anyResizingNeighbor[i]
            && (anyResizingNeighbor[i^1] || preferredFixedSide == i)) {
          operations.setIntervalResizing(gaps[i], false);
          if (!LayoutInterval.canResize(gaps[i^1])) {
            operations.setIntervalResizing(gaps[i^i], true);
          }
          break;
        }
      }
    }
  }
}

For the last time, try to find the bug yourself—I'll wait...

Still searching?

Nice! Those who found the error only in the expression iDesc.neighbor != null || iDesc.index == iDesc.index, I'm sorry to say—you lost :)

Sure, there's an issue, but it's not nearly interesting enough for the top one. Yes, there are two errors here, I've fooled you a bit. But what are the holidays without a little mischief? :)

The analyzer has detected an error in the i^i expression here and issued the following warning:

V6001 There are identical sub-expressions 'i' to the left and to the right of the '^' operator. LayoutFeeder.java 3897

The XOR operation makes no sense because the exclusive OR with two identical values will always be zero. For a quick refresher, here's the truth table for XOR:

a	b	a^b
0	0	0
0	1	1
1	0	1
1	1	0

In other words, the operation is true only when the operands are different. We'll have all the bits the same, since the value is the same.

Why did I like this bug so much? There is the i^1 operation, which looks almost identical to i^i. Thus, it'd be incredibly easy to miss this error on a code review, since we've already seen the correct i^1 above.

I don't know about you, but it reminds me of the famous:

#define true rand() % 2
// Happy debugging, folks!

Otherwise, it's hard to explain how it got into the code—unless, we dismiss the boring version with a simple typo. If you did manage to spot that bug, pat yourself on the back—or share your detective skills in the comments :)

2nd place. When patterns failed

I've already shown the errors from the first and third DBeaver articles, skipping the second one. I stand corrected—the following is just from the second article.

PVS-Studio analyzer doesn't like that isBinaryContents is called from the constructor of the TextWithOpen class, which is overridden in the subclasses:

public TextWithOpen(Composite parent, boolean multiFS, boolean secured) {
    super(parent, SWT.NONE);

    ....
    if (!useTextEditor && !isBinaryContents()) {
        ....

        editItem.setEnabled(false);
    }

    ....
}

protected boolean isBinaryContents() {
    return false;
}

So what? It's overridden—not a big deal, though. It seems like a code smell, nothing critical. At least, that's what I used to think. I've dedicated an article to my struggle with this bug.

TextWithOpen has many subclasses, and one of them is TextWithOpenFile. There, the method is actually overridden, and instead of false, it returns a field that the superclass doesn't have:

public class TextWithOpenFile extends TextWithOpen {
    private final boolean binary;
    ....

    @Override
    protected boolean isBinaryContents() {
        return binary;
    }
    ....
}

Suspicious yet? What does the constructor of this class look like?

public TextWithOpenFile(
    Composite parent,
    String title,
    String[] filterExt,
    int style,
    boolean binary,
    boolean multiFS,
    boolean secured
) {
    super(parent, multiFS, secured);
    this.title = title;
    this.filterExt = filterExt;
    this.style = style;
    this.binary = binary;
}

Notice that? The binary field is initialized after the superclass constructor is called. However, there's a call to the isBinaryContents method, which references the subclass field!

Here's the PVS-Studio warning:

V6052 Calling overridden 'isBinaryContents' method in 'TextWithOpen' parent-class constructor may lead to use of uninitialized data. Inspect field: binary. TextWithOpenFile.java(77), TextWithOpen.java 59

It's a rather entertaining picture. At first glance, the developer seemed to follow the best practices: avoid the code spaghetti, which is impossible to maintain, and try to implement canonical OOP through a template method pattern. But when implementing even such a simple pattern, we can make a mistake, which is what happened. In my opinion, such (fallacious) beauty in simplicity is a solid second place.

1st place. One error offset another

Rejoice! The first place on stage! The competition was fierce, but the choice had to be made. After much deliberation, I've decided to take the warning from the NetBeans check. Let me introduce the final code snippet:

private void changeIgnoreStatus (File f) throws IOException {
  File parent = f;
  boolean isDirectory = f.isDirectory() && (! Files.isSymbolicLink(f.toPath()));
  StringBuilder sb = new StringBuilder('/');
  if (isDirectory) {
    sb.append('/');
  }
  boolean cont = true;
  while (cont) {
    sb.insert(0, parent.getName()).insert(0, '/');
    parent = parent.getParentFile();
    String path = sb.toString();
    if (parent.equals(getRepository().getWorkTree())) {
      if (addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
          path, isDirectory, false) &&
          handleAdditionalIgnores(path, isDirectory)) {
        addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
                              path, isDirectory, true);
      }
      cont = false;
    } else {
      cont = addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
                                   path, isDirectory, false);
   }
}

I'm confident that it's impossible to spot such a bug at a glance—unless, of course, you've made this mistake yourself. I won't keep you waiting—here's the PVS-Studio warning:

V6009 Buffer capacity is set to '47' using a char value. Most likely, the '/' symbol was supposed to be placed in the buffer. IgnoreUnignoreCommand.java 107

In fact, the error is ridiculously elementary: the StringBuilder constructor has no overload that accepts char. What constructor is called then? The developer apparently thought that an overload accepting String would be called, and then the initial value of StringBuilder would be this slash.

However, an implicit type conversion occurs, and the type constructor that accepts int is called. In our case, it represents the initial size of StringBuilder. Passing char as an argument doesn't functionally affect anything because it won't be included in the final string. If the initial size is exceeded, it'll simply increase on its own without causing exceptions or other side effects.

But wait, I mentioned two errors, didn't I? Where's the second, and how are they connected? To spot this, we'll have to read into the method body and realize what this code does.

It generates an absolute path to a file or directory. Based on the code, the resulting path should look something like this:

• for a file: /folder1/file
• for a directory: /folder1/folder/.

The code looks quite correct. That's the problem. The code really works as it should :) But if we fix the error by replacing a character with a string, we'll get this instead of the correct result:

• /folder1/file/;
• /folder1/folder//

In other words, we'll get an extra slash at the end of the string. It'll be at the end because the code above adds new text to the beginning of the line every time.

Therefore, the second error is that this slash was passed to the constructor as an argument at all. However, I wouldn't underestimate such an error because if someone decides to replace a character with a string without checking, there may be issues.

That's how the first place in the top of errors goes to code that works correctly. New Year's miracles, what did you expect? :)

Conclusion

I hope you've enjoyed reading my bug stories. If any particular story stood out to you—or if you have suggestions for adjustments to the ranking—feel free to share your thoughts in the comments I'll keep them in mind for next time :)

If you're interested in other languages, I invite you to check out the top C# bugs for 2024 here—stay tuned for new tops!

All these errors were detected with PVS-Studio analyzer, and the latest version (7.34) has just been released! You can try it by this link.

To stay tuned for new articles on code quality, we invite you to subscribe:

• PVS-Studio X (Twitter);
• our monthly article digest;
• my personal blog.

Happy New Year!