Our website uses cookies to enhance your browsing experience.
Java keeps finding itself on cloud nine these days, all because of cloud hosting. With more and more applications and services adopting cloud-native architectures, it's a good time to take a closer look at Quarkus, which is one of the leading frameworks for building high-performance cloud applications.
We regularly check open-source projects with our PVS-Studio analyzer and publish articles about the errors we find. Articles like these give readers a better understanding of static analysis, highlight the most common errors, and showcase the nuances of developing in a particular language.
Large-scale projects are especially interesting to look at, since they're constantly evolving. They have massive codebases, and their development pulls in a wide range of approaches and technologies. That variety carries over into the bugs themselves.
This time we picked the popular project—Quarkus, a modern framework built for developing high-performance cloud applications in Java, using technologies that have become standard in enterprise development: microservices, DI containers, and more.
Quarkus focuses on production performance by shifting much of the work to the build phase: including reading part of the application configuration, scanning the classpath for annotated classes, and constructing a model of the application. Quarkus apps also compile easily into a native image.
This makes it clear we're dealing with a large, complex project, so we decided to see what errors we could find. We ran our analysis on this commit. Here are the results.
No, this isn't a lecture on parenting. I'll just remind you of some of the pitfalls hiding in Java's inheritance hierarchy. Here's the code we'll be looking at.
Snippet 1
public class InterceptorInfo extends BeanInfo
implements Comparable<InterceptorInfo> {
private final Set<AnnotationInstance> bindings;
....
@Override
public String toString() {
return "INTERCEPTOR bean [bindings=" + bindings +
", target=" + getTarget() + "]";
}
}Notice that the toString method uses the bindings field of the InterceptorInfo class. Now let's look at the constructor of the parent class BeanInfo:
BeanInfo(....) {
....
this.identifier = Hashes.sha1_base64(
(identifier != null ? identifier : "") +
toString() + beanDeployment.toString()
);
....
}To finally put all the pieces of the puzzle together, here's the InterceptorInfo class constructor:
InterceptorInfo(. . . ., Set<AnnotationInstance> bindings) {
super(. . . .);
this.bindings = bindings;
....
}Let's trace through what's actually happening here. When InterceptorInfo is initialized, the constructor of its parent class, BeanInfo, gets called first. That constructor, in turn, calls the toString method, which is overridden in the InterceptorInfo class. At this point, the toString method uses the bindings field before it's been initialized. The resulting string ends up reflecting the object's state before initialization.
We covered this issue in more detail, along with how to deal with it, in a separate article.
The PVS-Studio warning:
V6052 Calling overridden 'toString' method in 'BeanInfo' parent-class constructor may lead to use of uninitialized data. Inspect field: bindings. InterceptorInfo.java 265
Forgotten, lost, or mixed-up code snippets tend to show up in every project sooner or later, and Quarkus isn't an exception.
Snippet 2
public void build(Path projectDir) {
....
try {
ModelUtils.persistModel(projectDir.resolve("pom.xml"), model);
} catch (IOException e) {
throw new IllegalStateException(); // <=
}
}The class method is responsible for building the Maven project model and its dependencies. As the snippet shows, saving the model file can throw an exception, and that exception gets replaced by an IllegalStateException. This isn't a great practice, because if something goes wrong, the exception thrown from the build method won't carry any information about the actual error. No one will ever know what skeletons are hiding in that closet.
The PVS-Studio warning:
V6118 The original exception object 'IOException' was swallowed. Cause of original exception could be lost. MvnProjectBuilder.java 122
Snippet 3
Foo.Bar bar1 = new Bar(new ArrayList<>());
event1.fire(bar1);
assertEquals(1, bar1.getNames().size()); // <=
assertEquals("bazinga", bar1.getNames().get(0));
Foo.Bar bar2 = new Bar(new ArrayList<>());
event2.fire(bar2);
assertEquals(1, bar1.getNames().size()); // <=
assertEquals("bazinga", bar2.getNames().get(0));This is an excerpt from tests that verify the event mechanism. Two instances of Foo.Bar get created and tested here. It's easy to see that the second block of test code is copy-pasted from the first. The duplicated line assertEquals(1, bar1.getNames().size()), still references bar1 instead of bar2, so the check ends up testing the wrong object's state.
The PVS-Studio warning:
V6072 Two similar code fragments were found. Perhaps, this is a typo and 'bar2' variable should be used instead of 'bar1'. MockEventTest.java 40
By the way, we've added the exact same rule to our new JavaScript and TypeScript analyzer.
Snippet 4
public class PathTreeBuilder {
private List<String> includes;
private List<String> excludes;
....
List<String> getIncludes() {
return includes;
}
List<String> getExcludes() {
return includes; // <=
}
}As the class name suggests, PathTreeBuilder creates a PathTree instance, which is used to easily manage a project's dependencies as a tree structure. The builder has methods for populating the includes and excludes fields, but the getExcludes method mistakenly returns the includes field instead.
The PVS-Studio warning:
V6091 Suspicious getter implementation. The 'excludes' field should probably be returned instead. PathTreeBuilder.java 58
Snippet 5
private static boolean isIgnored(DotName classDotName) {
String className = classDotName.toString();
if (className.startsWith("java.util.")
|| className.startsWith("java.lang.")
|| className.startsWith("org.hibernate.engine.spi.")
|| className.startsWith("jakarta.persistence.")
|| className.startsWith("jakarta.persistence.") // <=
) {
return true;
}
return false;
}The method is used in the JpaJandexScavenger class to filter the classes needed for the integrated Hibernate ORM to work. The class itself determines in advance which types it needs. As mentioned in the Intro, Quarkus aims to move part of its logic to build time, and JpaJandexScavenger is a direct implementation of that idea.
In the method body, you can see the jakarta.persistence. package repeated in the logical condition:
className.startsWith("jakarta.persistence.") ||
className.startsWith("jakarta.persistence.")Given what this method is for, it's possible that another restriction was meant to go here and got left out. This probably won't affect functionality, but the final size of the application could grow slightly, since more classes end up covered by metadata than necessary. Or maybe it's all simpler than that, and it's just an extra check that was left in by mistake.
The PVS-Studio warning:
V6001 There are identical sub-expressions 'className.startsWith("jakarta.persistence.")' to the left and to the right of the '||' operator. JpaJandexScavenger.java 621
Numbers always have their own ways of going wrong.
Snippet 6
@ConsumeEvent("address-4")
CompletionStage<Long> listenAddress4(int i) {
return CompletableFuture.completedFuture((long) (i + 1));
}If the variable i equals to Integer.MAX_VALUE, adding one to it causes an overflow, and the result becomes Integer.MIN_VALUE. We can prevent this by casting one of the operands to long. Although there's a type cast in the example, it's applied to the result of the operation, so it doesn't actually prevent the overflow.
The PVS-Studio warning:
V6117 Possible overflow. The expression will be evaluated before casting. Consider casting one of the operands instead. CodecRegistrationTest.java 195
Snippet 7
@Override
public void apply(SocketSettings.Builder builder) {
if (config.connectTimeout().isPresent()) {
builder.connectTimeout((int) config.connectTimeout().get() // <=
.toMillis(), TimeUnit.MILLISECONDS);
}
if (config.readTimeout().isPresent()) {
builder.readTimeout((int) config.readTimeout().get() // <=
.toMillis(), TimeUnit.MILLISECONDS);
}
}Here's another cast that can lead to an overflow. Its purpose is unclear, especially since both connectTimeout and readTimeout on the builder variable take a long.
The PVS-Studio analyzer issues the following warnings for these spots:
V6106 Casting expression to int type before implicitly casting it to other type may be excessive or incorrect. MongoClients.java 289
V6106 Casting expression to int type before implicitly casting it to other type may be excessive or incorrect. MongoClients.java 286
Snippet 8
static <T> List<T> takeLast(List<T> list, int n) {
if (n < 1 || n > list.size()) {
throw new IndexOutOfBoundsException(n);
}
if (list.isEmpty()) { // <=
return list;
}
return list.subList(list.size() - n, list.size());
}The method takes the last n elements from list. The check above implies we can't request fewer than one item, or more than the list actually contains. But the check also implies the list itself can't be empty. If we're taking at least one element from the list, the list has to contain at least one element already. So, the check right below, list.isEmpty(), always evaluates to false. If the intent was to handle an empty list as a special case, this check won't catch it, and it needs to be reworked.
The PVS-Studio warning:
V6007 Expression 'list.isEmpty()' is always false. CollectionTemplateExtensions.java 57
Snippet 9
if (errorRate != -1.0) {
list.add("ERROR");
list.add(new BigDecimal(errorRate).toPlainString()); // Prevent E notation
}This one's worth a closer look. If we pass 0.1 into the BigDecimal constructor, what actually gets stored is 0.1000000000000000055511151231257827021181583404541015625. This "tail" is a natural feature of how real numbers are represented in a computer. If you're curious why this happens, here's a link with the theory behind it.
Now let's see how to create the object without that tail showing up as a surprise. The standard library provides the BigDecimal#valueOf method for exactly this, and it implements some clever logic:
public static BigDecimal valueOf(double val) {
....
var fmt = FormattedFPDecimal.valueForDoubleToString(Math.abs(val));
long s = fmt.getSignificand();
....
}Roughly speaking, here's what happens: the method takes the binary representation of a real number and converts it into a decimal representation, one that's correctly rounded and as short as possible. For the number we're discussing, that string comes out to exactly 0.1. So the BigDecimal#valueOf method will preserve our exact value.
This method first appeared in Java 5 and looked completely different:
public static BigDecimal valueOf(double val) {
return new BigDecimal(Double.toString(val));
}A string was used instead of a decimal representation here, but the problem was solved the same way overall.
By the way, the BigDecimal#valueOf method still refers to Double#toString() in its Javadoc, even though it now uses different methods under the hood these days. The core of the algorithm hasn't changed though, and it's well documented in the Javadoc for the Double#toString() method.
Before Java 5, the BigDecimal(String) constructor had already existed and was the recommended choice over BigDecimal(double).
Such nuances are particularly important in high-precision computing systems, where even small errors can lead to unexpected consequences.
The PVS-Studio warning:
V6068 Constructor call can result in imprecise representation of the initialized value. BfInsertArgs.java 95
If you suddenly find yourself in the past or the future after celebrating New Year's, don't panic, it's just a bug.
Snippet 10
public class WebSocketNextJsonRPCService implements ConnectionListener {
private static final DateTimeFormatter FORMATTER =
DateTimeFormatter.ofPattern("YYYY-MM-dd HH:mm:ss");
....
}At first glance, there's nothing wrong here at all, but take a closer look at the date format string: YYYY-MM-dd. Still looks fine on the surface, but there's a catch.
If the current date is, say, January 1, 2027, this format will display it as January 1, 2026.
That's because YYYY displays the year based on the week number, not the calendar date. There's a specific rule for it too: if most of the week falls in the previous year, that's the year that gets shown. In our example, January 1 falls on a Friday, so most of that week belongs to 2026.
The reverse happens when only a small part of the week falls in the outgoing year. December 31, 2024, for instance, would show up as December 31, 2025, in this format, since most of that week belongs to 2025. A whole year gone, and nobody even noticed.
This bug is unusual enough that we devoted a separate article to it.
We can fix this by replacing YYYY with yyyy, which is exactly what PVS-Studio says:
V6122 Usage of 'Y' (week year) pattern was detected: it was probably intended to use 'y' (year). WebSocketNextJsonRPCService.java 39
We explained what this theory is and how it applies to development in the article. Now let's see once again how it plays out in practice.
Snippet 11
@Override
public <T> T getClaim(String claimName) {
if (claimName.equals(Claims.groups)) {
return (T) getGroups();
} else if (claimName.equals(Claims.groups)) { // <=
return (T) getAudience();
} else if (claimName.equals(Claims.exp)) {
return (T) Long.valueOf(getExpirationTime());
} else if (claimName.equals(Claims.iat)) {
return (T) Long.valueOf(getIssuedAtTime());
} else if (claimName.equals(Claims.aud)) {
return (T) getAudience();
}
return (T) claims.getClaim(claimName);
}The PVS-Studio warning:
V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. CognitoPrincipal.java 38
Here's what's happening: the second condition in the if-else chain has a typo, checking Claims.groups again, while still returning the result of the getAudience method call. One might argue that it's no big deal, since the correct condition appears later in the chain, and the second check, claimName.equals(Claims.groups), will never be true at that point anyway, since it already failed the first condition. But that's not really the issue here. The warning is issued for the amazon-lambda-rest package. Now let's take a look at another warning nearby, in the amazon-lambda-http package:
@Override
public <T> T getClaim(String claimName) {
if (claimName.equals(Claims.groups)) {
return (T) getGroups();
} else if (claimName.equals(Claims.groups)) { // <=
return (T) getAudience();
} else if (claimName.equals(Claims.exp)) {
return (T) Long.valueOf(getExpirationTime());
} else if (claimName.equals(Claims.iat)) {
return (T) Long.valueOf(getIssuedAtTime());
} else if (claimName.equals(Claims.aud)) {
return (T) getAudience();
}
return (T) getClaims().getClaims().get(claimName);
}Same exact code, same exact bug. The only difference is in the last line. That's how buggy code spreads through a project via simple copy-paste.
PVS-Studio triggers in the second package: V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. CognitoPrincipal.java 41
Snippet 12
@BuildStep
@Record(ExecutionTime.RUNTIME_INIT)
VertxWebRouterBuildItem initializeRouter(....) {
....
List<RouteBuildItem> redirectRoutes = new ArrayList<>();
....
if (frameworkRouterCreated) {
if (redirectRoutes.size() > 0) { // <=
recorder.setNonApplicationRedirectHandler(
nonApplicationRootPath.getNonApplicationRootPath(),
nonApplicationRootPath.getNormalizedHttpRootPath()
);
redirectRoutes.forEach(route -> recorder.addRoute(
httpRouteRouter,
route.getRouteFunction(),
recorder.getNonApplicationRedirectHandler(),
route.getType()
)
);
}
}
return new VertxWebRouterBuildItem(httpRouteRouter, mainRouter,
frameworkRouter, managementRouter,
mutinyRouter);
}The PVS-Studio warning:
V6007 Expression 'redirectRoutes.size() > 0' is always false. VertxHttpProcessor.java 392
Here, part of the logic never runs, since the redirectRoutes collection never gets populated within the method and is never passed anywhere either. We decided to look at the project's history for this file, and we found a commit from 2021 with some curious lines that had been removed:
if (httpBuildTimeConfig.redirectToNonApplicationRootPath &&
route.isRequiresLegacyRedirect()
) {
redirectRoutes.add(route);
}So, what do broken windows have to do with any of this? Extra code left in one place can lead to the same kind of code appearing elsewhere in the project. Take that abandoned code, for example: it uses a method that never gets removed, because it's still needed right there. As a result, the abandoned code now touches two classes, and this chain could keep extending further. The end result is a bloated codebase that gets harder and harder to reason about.
Here's the code the analyzer highlighted.
Snippet 13
static final boolean IS_CYGWIN = OS.WINDOWS.isCurrent()
&& System.getenv("PWD") != null
&& System.getenv("PWD").startsWith("/");The code reads the PWD environment variable, usually used to indicate the current working directory. It comes with its own quirks though: it might not be set at all, or it might be overridden from outside. . For consistent results, it's safer to use the JVM system property user.dir, since it's guaranteed to be set when the application starts and can't be changed externally.
The PVS-Studio warnings for this code:
V6110 Using the 'PWD' environment variable could be unsafe or unreliable. Consider using trusted system property 'user.dir' instead. TerminalUtils.java 27
V6110 Using the 'PWD' environment variable could be unsafe or unreliable. Consider using trusted system property 'user.dir' instead. TerminalUtils.java 28
The project also has several similar warnings for other values, which should likewise be retrieved through System.getProperty:
V6110 Using the 'HOME' environment variable could be unsafe or unreliable. Consider using trusted system property 'user.home' instead. Constants.java 11
V6110 Using the 'USER' environment variable could be unsafe or unreliable. Consider using trusted system property 'user.name' instead. AnalyticsService.java 231
Multithreaded code punishes even small mistakes.
Snippet 14
public class KubernetesDevUIProcessor {
static volatile List<Manifest> manifests;
public List<Manifest> getManifests() throws BootstrapException {
if (manifests == null) {
synchronized (Holder.class) {
if (manifests == null) {
manifests = new ArrayList<>();
....
try (CuratedApplication bootstrap = quarkusBootstrap.bootstrap()) {
....
for (var entry : context.entrySet()) {
manifests.add(
new Manifest(entry.getKey(),
new String(entry.getValue()))
);
}
}
}
}
}
return manifests;
}
}This is an example of an incorrect implementation of the double-checked locking pattern applied to the manifests field. If you want to know how this pattern works and why it's used, we cover that in the article.
When the thread enters the synchronization block, it initializes the manifests field with an empty list and then proceeds to populate it. In other words, it was originally assumed that the field would become available once it had been created and populated with values. However, the publication occurs right after manifests is initialized, so another thread might end up with a list that's been created but not yet populated.
Fixing this is straightforward: just initialize the field with a fully built list from a local variable.
The PVS-Studio warning:
V6082 Unsafe double-checked locking. Object was assigned to the field before it was initialized. KubernetesDevUIProcessor.java 66
Snippet 15
public class VertxUdpMetrics implements DatagramSocketMetrics {
private volatile Tags tags;
@Override
public void listening(String localName, SocketAddress localAddress) {
tags = tags.and("address", NetworkMetrics.toString(localAddress)); // <=
}
}The tags field has been marked with the keyword volatile. Just to recap, here's what that means in Java: if one thread modifies a variable, another thread immediately sees that change. However, this works flawlessly for atomic operations. If the logic involves several actions, a completely different approach is required to avoid a race condition.
To see what's happening here, suppose two threads call the listening method at the same time. Both entered the method, both created a new instance using the and method, and both attempted to store the new value in the tags variable.
These three operations don't add up to a single atomic operation, so the volatile keyword won't help here. A different approach is needed instead, like making the method synchronized.
The PVS-Studio warning:
V6074 Non-atomic modification of volatile variable. Inspect 'tags'. VertxUdpMetrics.java 37
This is a case where carelessness led to buggy, unnecessary code.
Snippet 16
Let's break this code down step by step.
public class TemplateHtmlBuilder {
private static final String HEADER_TEMPLATE_NO_STACK = "<h1>%1$s</h1>\n" +
"%2$s \n" +
"<div class=\"container content\">\n";
private static final String HTML_TEMPLATE_START_NO_STACK = "" +
"<!doctype html>\n" +
"<html lang=\"en\">\n" +
"<head>\n" +
" <title>%1$s%2$s</title>\n" +
" <meta charset=\"utf-8\">\n" +
"</head>";
....
}Here we see two HTML templates stored in these fields. Looking closely, we'll notice that both the first and second templates contain placeholders for two string variables.
Now let's take a look at how they're used:
public TemplateHtmlBuilder(....,
String title,
String details
) {
....
result = new StringBuilder(String.format(HTML_TEMPLATE_START_NO_STACK,
escapeHtml(title),
subTitle == null || subTitle.isEmpty() ? "" : " - " +
escapeHtml(subTitle), CSS));
result.append(String.format(HEADER_TEMPLATE_NO_STACK,
escapeHtml(title),
escapeHtml(details),
actionLinks.toString()
));
}When using the HEADER_TEMPLATE_NO_STACK and HTML_TEMPLATE_START_NO_STACK fields, the template gets populated with three arguments in both cases, even though the lines themselves only have two slots. This same method contains identical lines using the exact same arguments, but with templates built for three arguments. This looks like a typical copy-paste mistake, where the extra arguments never got removed.
The PVS-Studio warning:
V6046 Incorrect format. A different number of format items is expected. Arguments not used: 3. TemplateHtmlBuilder.java 325
V6046 Incorrect format. A different number of format items is expected. Arguments not used: 3. TemplateHtmlBuilder.java 328
Snippet 17
public void boot(...., Optional<FunctionInitializedBuildItem> hasFunctions) {
if (!hasFunctions.isPresent() || hasFunctions.get() == null) // <=
return;
}The method takes a hasFunctions parameter of type Optional, which is used to perform a very strange check:
!hasFunctions.isPresent() || hasFunctions.get() == nullHere's a quick refresher on what Optional::isPresent does:
public boolean isPresent() {
return value != null;
}In this specific case, only the call to Optional::isPresent was needed, since Optional::get throws an exception if the value is actually null.
The PVS-Studio warning:
V6007 Expression 'hasFunctions.get() == null' is always false. FunqyHttpBuildStep.java 77
Snippet 18
if (typeInfo == null || (typeInfo != null &&
typeInfo.endsWith(SectionHelperFactory.HINT_METADATA))
) {
continue;
}This one's straightforward: the check for typeInfo != null is unnecessary, since the opposite condition was already checked first.
The PVS-Studio warning:
V6007 Expression 'typeInfo != null' is always true. QuteProcessor.java 920
Snippet 19
if (persistenceProviderResolver == null ||
(persistenceProviderResolver != null // <=
&& !(persistenceProviderResolver instanceof
MultiplePersistenceProviderResolver
)
)
) {
....
}This warning is similar to the previous one, but here even the check for equality with null is unnecessary. In fact, instanceof returns false if the object passed to it is null. Since this checks that persistenceProviderResolver is not of type MultiplePersistenceProviderResolver, this bulky if statement can be simplified to:
!(persistenceProviderResolver instanceof MultiplePersistenceProviderResolver)The PVS-Studio warning:
V6007 Expression 'persistenceProviderResolver != null' is always true. PersistenceProviderSetup.java 28
Snippet 20
for (String name : res.headers().names()) {
if (name.equalsIgnoreCase("Transfer-Encoding")) { // <=
continue; // ignore transfer encoding,
// chunked screws up message and response
}
for (String v : res.headers().getAll(name)) {
if (name.equalsIgnoreCase("Transfer-Encoding") // <=
&& v.contains("chunked")) {
continue;
}
responseBuilder.getMultiValueHeaders().add(name, v);
}
}The name variable gets checked for equality with the Transfer-Encoding string before the inner loop and on every iteration of it. This unnecessary operation just bloats the code.
The PVS-Studio warning:
V6007 Expression 'name.equalsIgnoreCase("Transfer-Encoding")' is always false. LambdaHttpHandler.java 104
Snippet 21
while (
!ResteasyReactiveDotNames.OBJECT.equals(currentClazz.name()) &&
currentClazz != null // <=
) {
....
}And here's the cherry on top: currentClazz gets dereferenced first, and the null check comes only after.
The PVS-Studio warning:
V6060 The 'currentClazz' reference was utilized before it was verified against null. ResteasyReactiveProcessor.java 1125
We've now gone through some interesting bugs and suspicious spots we managed to find in the Quarkus source code. We've checked other major projects too, and found plenty of interesting things there as well:
By the way, in the Quarkus repository, like in many modern projects, you can find .md files meant for AI agents. But no matter how powerful a tool AI is, it still makes sneaky mistakes sometimes, like the ones we already covered in the articles:
And that wraps up this article. If you'd like to check your own project, we've got a free trial of the PVS-Studio analyzer for you, you can get it here.
0
0
0
0