Would you like to see some errors and suspicious fragments in Java code? We found lost increments, empty regular expressions, strange string sanitization, and more in an open-source project called OpenAPI Generator.
Introduction
First, I'd like to say hello and introduce new readers to this type of articles we have on our blog.
In such articles, we explain how we use PVS-Studio static analyzer on popular open-source projects, and then we share some of our most curious findings. In special cases, we also highlight interesting aspects of the project's language by examining some of the warnings.
We're checking OpenAPI Generator today. This is an open-source project designed to automatically generate client libraries, server stubs, documentation, and configuration files based on OpenAPI specifications in JSON or YAML format.
One of the main project's objectives is to help implement the API-First (aka Contract-First) approach. Within this approach, teams that are developing client-server systems start by defining an interaction contract between the services and clients using a formal specification. Based on this contract, client libraries and server stubs are automatically generated, and then developers implement the business logic on top of them. The project provides plugins for the Gradle and Maven build systems for Java. You can find the full list of languages and technologies that OpenAPI Generator supports for code generation here.
The project is fairly popular: at the time of writing, it has just over 25,000 stars on GitHub.
For our check, we used the latest version, v7.19.0. Now, let's move on to the warnings.
An unclosed stream
We'll start the article with a discussion of an open file stream. Here's a code snippet that triggered the analyzer:
@Override
public File write(
Map<String, Object> data,
String template,
File target
) throws IOException {
....
} else {
InputStream is;
try {
String fullTemplatePath = getFullTemplateFile(template);
is = getInputStream(fullTemplatePath);
} catch (TemplateNotFoundException ex) {
is = new FileInputStream(Paths.get(template).toFile()); // <=
}
return writeToFile(
target.getAbsolutePath(),
IOUtils.toByteArray(is)
);
}
}
PVS-Studio warning: V6127 The 'is' Closeable object is not closed. This may lead to a resource leak. TemplateManager.java 177
The catch block creates a file input stream named is. The return statement then passes it to IOUtils.toByteArray, and that's it. Upon exiting the method, we lose the reference to this file stream, meaning there's no way to close it. As a result, the resource remains unavailable until the garbage collector eventually reaches the object holding it. Since the process is non-deterministic, this can lead to a file descriptor leak.
You might think that IOUtils.toByteArray() closes the stream, but it doesn't.
The try-with-resources construct in Java is designed for proper resource management, so it's preferable to use it in similar situations.
P.S. Starting with Java 11, Paths.get is considered obsolete. It's better to use Path.of instead.
Lost increment
One of the basic theoretical questions everyone faces when starting to learn Java (or another similar programming language) is the difference between post- and pre-increment operations. The following code snippet illustrates this:
private String patchPropertyName(
CodegenModel model,
CodegenProperty property,
String value, Set<String> composedPropertyNames
) {
....
if (composedPropertyNames != null) {
String tmpName = name;
long count = model.allVars.stream()
.map(v -> v.name)
.filter(n -> n.equals(tmpName))
.count() + composedPropertyNames.stream()
.filter(n -> n.equals(tmpName))
.count();
if (count > 0) {
name = name + count++; // <=
}
composedPropertyNames.add(name);
}
return name;
}
PVS-Studio warning: V6123 Modified value of the operand is not used after the increment operation. AbstractCSharpCodegen.java 780
I think you can see what's going on here. Developers attempted to add some count value to the name string. However, they used the post-increment operator on count when adding it, yet the incremented value was never used. In other words, this post-increment operation is useless.
This is likely the result of a typo, which can lead to various issues. Even with such simple code, attention to detail is crucial.
Stonks
@Override
public String toModelName(final String name) {
// obtain the name from modelNameMapping directly if provided
if (modelNameMapping.containsKey(name)) {
return modelNameMapping.get(name);
}
String result = sanitizeName(name);
// remove dollar sign
result = result.replaceAll("$", ""); // <=
....
}
PVS-Studio warning: V6059 An odd use of a special character in a regular expression. Possibly, it was intended to be escaped. AbstractJuliaCodegen.java 313
Both the comment above the highlighted line and the code itself make it clear that developers wanted to remove $ characters from the string. Unfortunately, that won't actually happen.
The replaceAll method treats its first argument as a regular expression, and $ is a special character that marks the end of the regular expression. As a result, devs got the regular expression that searches for absolutely nothing.
To fix this, we need to properly escape the special character with \. In that case, the fixed version looks like this:
result = result.replaceAll("\\$", ""); // <=
By the way, this code snippet is used to generate code for the Julia programming language.
Logging didn't help
....
LOGGER.warn(
"Unknown default value for var {0} in class {1}",
var.name,
cm.classname
);
....
PVS-Studio warning: V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1, 2. ScalaPlayFrameworkServerCodegen.java 407
The project uses slf4j for logging, where value substitution in log messages relies on empty curly braces—{}. However, the code here uses C#-style formatting syntax, so, as you might have guessed, the values will never make it into the log message.
The project also contains similar cases. The difference is that they use wildcard characters similar to String.format or C's printf.
....
LOGGER.error(
"Generated field number is "
"in reserved range (19000, 19999) for %s, %d",
name,
fieldNumber
);
....
PVS-Studio warning: V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1, 2. ProtobufSchemaCodegen.java 1076
The code uses the same slf4j, though.
In addition to incorrect wildcard characters, we also found some simple typos in the project. The developers forgot to add the required parameter to the logger here:
....
LOGGER.error(
"Type {} not yet supported in openapi-normalizer " +
"to process OpenAPI 3.1 spec with multiple types."
);
....
PVS-Studio warning: V6046 Incorrect format. A different number of format items is expected. Missing arguments: 1. OpenAPINormalizer.java 1857
And here, both an extra placeholder in the message and a missing parameter are possible:
....
List<String> newArgs = new ArrayList<>();
....
String newArg = String.join(" ", newArgs);
LOGGER.trace("new arg {} {}", newArg);
formattedArgs.add(newArg);
....
PVS-Studio warning: V6046 Incorrect format. A different number of format items is expected. Missing arguments: 2. SpringCodegen.java 1140
Misusing Optional can lead to disaster
private static CodegenParameter pathParamForName(
CodegenOperation op,
String pathParam
) {
final CodegenParameter
param = op.pathParams.stream()
.filter(p -> p.paramName.equals(pathParam))
.findFirst()
.get();
if (param == null) {
throw new RuntimeException("Bug: path param " + pathParam + " not found");
}
return param;
}
PVS-Studio warning: V6036 The value from the uninitialized 'op.pathParams.stream().filter(p -> p.paramName.equals(pathParam)).findFirst()' optional is used. ScalaCaskServerCodegen.java 1021
The Stream.findFirst method returns an Optional object. This is where the error lies.
Instead of obtaining the Optional object and checking whether it contains the required value using isPresent, it's unpacked using get. If Optional is empty, the code won't get to the point of checking and throwing a Runtime exception with its message. This is why:
public T get() {
if (value == null) {
throw new NoSuchElementException("No value present");
}
return value;
}
Overall, Optional was created to carefully work with values that may or may not exist. However, this care is neglected here. Optional itself provides everything its authors originally intended, but in a more conventional and concise manner:
private static CodegenParameter pathParamForName(
CodegenOperation op,
String pathParam
) {
return op.pathParams.stream()
.filter(p -> p.paramName.equals(pathParam))
.findFirst()
.orElseThrow(() -> new RuntimeException(
"Bug: path param " + pathParam + " not found"
));
}
Repeats
The project contains many similar code fragments:
public Swift6ClientCodegen() {
....
defaultIncludes = new HashSet<>(
Arrays.asList(
"Data",
"Date",
"URL", // for file
"UUID",
"Array",
"Dictionary",
"Set",
"Any", // <=
"Empty",
"AnyObject",
"Any", // <=
"Decimal")
);
....
}
PVS-Studio warning: V6033 An item with the same key '"Any"' has already been added. Swift6ClientCodegen.java 190
In this particular project, this issue is unlikely to cause catastrophic or hard-to-trace crashes. Still, it's worth a brief mention.
Based on our experience, errors like this can sometimes signal that the devs added the wrong value to a collection. Developers often insert something resembling the intended value or forget to update it after copying and pasting. A good example of such an error appeared in the GeoGebra project.
String comparison strikes again
We encounter this error rather frequently. The code fragment:
@Getter private String contentType;
@Getter private String style;
....
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
CodegenEncoding that = (CodegenEncoding) o;
return contentType == that.getContentType() && // <=
Objects.equals(headers, that.getHeaders()) &&
style == that.getStyle() && // <=
explode == that.getExplode() &&
allowReserved == that.getAllowReserved() &&
Objects.equals(vendorExtensions, that.vendorExtensions);
}
PVS-Studio warnings:
V6013 Strings 'contentType' and 'that.getContentType()' are compared by reference. Possibly an equality comparison was intended. CodegenEncoding.java 50
V6013
Strings 'style' and 'that.getStyle()' are compared by reference. Possibly an equality comparison was intended. CodegenEncoding.java 52
In each article on this topic, we explain why it's better to avoid doing so. We've done this so many times that it has become a part of our terminology section. In short, the results of such comparisons may be non-deterministic. It all depends on how the string used in the equals method is defined. For such a check to produce a deterministic result, the string must always be in the String Pool. It's quite difficult to guarantee such behavior throughout the entire application.
A pointless exchange
Let's take another look at a fragment where characters in a string are replaced:
@Override
public String toDefaultValue(Schema p) {
....
} else if (ModelUtils.isStringSchema(p)) {
if (p.getDefault() != null) {
String defaultValue = String.valueOf(p.getDefault());
if (defaultValue != null) {
defaultValue = defaultValue.replace("\\", "\\\\")
.replace("'", "\'"); // <=
....
}
}
}
....
}
PVS-Studio warning: V6009 Function 'replace' receives an odd argument. The '"'"' argument was passed several times. AbstractPythonPydanticV1Codegen.java 191
It's difficult to understand the author's intention. Perhaps they expected the created string to escape the ' character, but—spoiler alert—it won't. To replace ' with \' in the final line, we need to fix the call to the replace method:
.replace("'", "\\\\'");
Then, we'll get the \'aaa\' string from 'aaa'. Now, the 'aaa' string remains unchanged, but if that wasn't the goal, then we're dealing with some weird, outdated code.
Be careful with null
Now, let's look at the possible null dereferencing. The first fragment:
@Override
public String toOneOfName(
List<String> names,
Schema composedSchema
) {
Map<String, Object> exts = null;
if (composedSchema != null) { // <=
exts = composedSchema.getExtensions();
}
if (exts != null && exts.containsKey(X_ONE_OF_NAME)) {
return (String) exts.get(X_ONE_OF_NAME);
}
List<Schema> schemas = ModelUtils.getInterfaces(composedSchema); // <=
....
}
Judging by the context, the composedSchema parameter passed to the method may be null. We pass it to the getInterfaces method, where it's immediately dereferenced:
public static List<Schema> getInterfaces(Schema composed) {
if (composed.getAllOf() != null && !composed.getAllOf().isEmpty()) { // <=
....
}
PVS-Studio warning: V6008 Potential null dereference of 'composedSchema'. RustServerCodegen.java 1422
The project has a similar warning. Let's take a look at it:
@Override
public int compareTo(ModelDepend second) {
....
if ( depend != null
&& depend.size() != (second.depend == null
? 0
: second.depend.size()) // <=
) {
//LOGGER.debug("Compare " + name + " with " + second.name + "=D"
// + (depend.size() - second.depend.size()));
return depend.size() - second.depend.size(); // <=
}
//LOGGER.debug("Compare " + name + " with " + second.name + "=<name>");
return name.compareTo(second.name);
}
PVS-Studio warning: V6008 Potential null dereference of 'second.depend'. AbstractAdaCodegen.java 895
The condition contains the following fragment:
second.depend == null ? 0 : second.depend.size()
If the entire condition is true, the second.depend parameter is dereferenced in the then block without any checks. This can also lead to an NPE.
The best approach would be to either account for the possibility that second.depend may be null before dereferencing it, or remove the confusing check above.
Conclusion
Well, that's it for today. We'll notify the OpenAPI Generator creators about all the issues we've covered via PR and Issue in their repository.
If you'd like to try PVS-Studio on your C, C++, C#, or Java project—and soon on JavaScript, TypeScript, and Go as well—feel free to use this link.
If you own an open-source project or you're a student or teacher, we offer a free license. For more information, follow the links.
That's all folks. See ya soon!
Top comments (0)