" + message + "
"); // <= .... } } ``` The PVS\-Studio warning: [V5330](https://pvs-studio.com/en/docs/warnings/v5330/)\. Possible XSS injection\. Potentially tainted data in the 'message' variable might be used to execute a malicious script\. [HelloSessionServlet\.java 70](https://github.com/jetty/jetty.project/blob/787aa033145ca93eae870f4728cec3aabe2651d4/jetty-demos/jetty-servlet6-demos/jetty-servlet6-demo-embedded/src/main/java/org/eclipse/jetty/servlet6/embedded/HelloSessionServlet.java#L70) The analyzer points out that there could be an [XSS injection](https://owasp.org/www-community/attacks/xss/)\. In the code, we obtain a string from the request, use it to generate messages, and then use those messages to generate an HTML page\. Since the request data isn't [sanitized](https://pvs-studio.com/en/blog/terms/6496/) in any way, the XSS injection is indeed possible\. If the externally passed `message` is ``, the JavaScript `alert("XSS Injection")` will be executed when the page is opened\. Just like any other js code\. If this is a real vulnerability, why doesn't it rank higher on the list? Because it's a part of a demonstration example\. In other words, we show the code just to see what can happen in Jetty\. Nevertheless, it's worth remembering that the demonstration example couldn't be copied into production\. After all, it's not best practice to show off unsafe code\. As the first [taint](https://pvs-studio.com/en/blog/terms/6496/)\-analysis warning in a real project, although it's in a demonstration example, we award it 5th place in the top\. ## 4th place\. Double only Look at the error from [Elasticsearch](https://github.com/elastic/elasticsearch)\. ```cpp public static Number truncate(Number n, Number precision) { .... Double result = (((n.doubleValue() < 0) ? Math.ceil(g) : Math.floor(g)) / tenAtScale); return n instanceof Float ? result.floatValue() : result; // <= } ``` The PVS\-Studio warning: [V6088](https://pvs-studio.com/en/docs/warnings/v6088/) Result of this expression will be implicitly cast to 'double'\. Check if program logic handles it correctly\. [Maths\.java 122](https://github.com/elastic/elasticsearch/blob/f2628392d1b89b1c96f5c835e70432ce568d5e9f/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/math/Maths.java#L122) This method cuts the passed number according to the specified precision\. For integer values, it reduces the number of digits, while for fractional values, it removes excess digits after the decimal point\. The snippet above shows the logic has applied to fractional numbers\. Judging by the ternary operator, in the case of fractions, developers want the method to return an identical number with which it came to the method \(either `Float` or `Double`\)\. But a subtle but important detail has been overlooked\. The ternary operator is an expression, which means its result must have a single and predetermined type\. At this point, the rules of [numeric context](https://docs.oracle.com/javase/specs/jls/se24/html/jls-5.html#jls-5.6) come into play\. In short, the following applies: 1. If objects of different types appear in an expression, they're converted to a single common type\. 1. In this case, the common type would be `Double`, since it represents both `Double` and `Float`\. That is, despite expectations, the method always returns a `Double` when processing fractional numbers\. The compact recording format really let us down\. A fix may look like this: ```cpp public static Number truncate(Number n, Number precision) { .... if (n instanceof Float) { return result.floatValue(); } else { return result; } } ``` Due to this non\-obvious language nuance, the error earns fourth place in the top ten\. ## 3rd place\. Please wait your turn\! Now we move on to the award podium\. The bronze medalist has been found in [IntelliJ IDEA](https://github.com/JetBrains/intellij-community)\. Look at the code fragment: ```cpp public final class UsageType { .... public static final UsageType DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED = new UsageType( UsageViewBundle.messagePointer( "usage.type.delegate.to.another.instance.method.parameters.changed" ) ); private static final Logger LOG = Logger.getInstance(UsageType.class); public UsageType(@NotNull Supplier<....> nameComputable) { myNameComputable = nameComputable; if (ApplicationManager.getApplication().isUnitTestMode()) { String usageTypeString = myNameComputable.get(); if (usageTypeString.indexOf('{') != -1) { LOG.error(....); } } } .... } ``` The PVS\-Studio warning: [V6050](https://pvs-studio.com/en/docs/warnings/v6050/) Class initialization cycle is present\. Initialization of 'DELEGATE\_TO\_ANOTHER\_INSTANCE\_PARAMETERS\_CHANGED' appears before the initialization of 'LOG'\. [UsageType\.java 46](https://github.com/JetBrains/intellij-community/blob/69f5cd09d18e2f29646f7828a6e3f9ba1a752e09/platform/usageView/src/com/intellij/usages/impl/rules/UsageType.java#L46) The analyzer warns about a cyclic dependency during static field initialization; one of those Java nuances that's easy to miss\. The constructor is used to initialize the `DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED` field\. And we access `LOG` logger in this constructor\. Fields [are initialized in the order in which they're declared](https://docs.oracle.com/javase/specs/jls/se25/html/jls-12.html#jls-12.4.2)\. As we can see, `DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED` is located above `LOG`, i\.e\. it's initialized earlier\. As a result, when the constructor runs, `LOG` [will be](https://docs.oracle.com/javase/specs/jls/se25/html/jls-12.html#jls-12.3.2) `null`, and when we try to access it, we'll encounter an NPE\. As in most of the cases above, the fix will be extremely simple\. We just need to place the `LOG` statement above `DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED`\. ## 2nd place\. Where did the block go? We moved on to the silver medalist\. Meet again the Minecraft mod, [CustomNPC\+](https://www.curseforge.com/minecraft/mc-mods/customnpc-plus)\. Look at the code fragment: ```cpp public int range = 5; public int speed = 5; .... public boolean aiShouldExecute() { healTicks++; if (healTicks < speed * 10) return false; for (Object plObj : npc.worldObj.getEntitiesWithinAABB( EntityLivingBase.class, npc.boundingBox.expand( range, range / 2, // <= range)) ) { .... } healTicks = 0; return !toHeal.isEmpty(); } ``` The PVS\-Studio warning: [V6094](https://pvs-studio.com/en/docs/warnings/v6094/) The expression was implicitly cast from 'int' type to 'double' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. [JobHealer\.java 41](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/roles/JobHealer.java#L41) In this code snippet, the game periodically \(based on the `speed` field\) scans for entities inside a rectangular area with a radius defined by `range`, centered around an NPC doctor\. Since the `expand` method has the `expand(double x, double y, double z)` signature, the analyzer is absolutely right to warn about this fragment: `int` is passed to the method where `double` is expected\. "So what?" you may ask\. We think this is really strange, so we've decided to investigate this case\. This minor flaw leads to the following consequence: if the radius is an odd number, when evaluating the Y axis, we lose half a block at the top and at the bottom \(i\.e\., a whole block in total\)\. We've proved it by visualizing the collision box calculated from the radius\. Look at the visualization:  * The NPC doctor collision is highlighted in white\. * The collision currently implemented \(5 / 2\) is highlighted in red\. * The collision without integer division \(5 / 2\.0\) is highlighted in green\. As we can see, if we use the `2.0` literal of the `double` type, the division is no longer an integer, and as a result, the collision will cover exactly the radius we want\. And to confirm that it worked correctly, the author of the [article](https://pvs-studio.com/en/blog/posts/java/1219/) had to "ask the universe": > Notably, developers have fixed this error in the original mod for Minecraft 1\.12\.2\. Unfortunately, I can't provide any direct evidence, so let's say I asked the universe—and this code snippet is what it sent back: ```cpp .... this.npc.world.getEntitiesWithinAABB( EntityLivingBase.class, npc.getEntityBoundingBox().expand(range, range / 2.0, range) ); .... ``` > Since devs have fixed the bug in the original modification, the same should apply here\. ## 1st place\. The mystery case of missed bonus And now, the gold medal goes to\.\.\. [CustomNPC\+](https://www.curseforge.com/minecraft/mc-mods/customnpc-plus)\! Could the abundance of Minecraft bugs be because half our team spent countless hours working on mods and plugins for it? Who knows\. Let's move on to the code: ```cpp .... int startIndex = -1; boolean number = false; try { startIndex = Integer.parseInt(bonusID); number = true; } catch (Exception var34) { number = false; } for (startIndex = 0; startIndex < bonuses.length; ++startIndex) { .... if (number && startIndex == startIndex || // <= !number && bonusValues[startIndex][0].equals(bonusID) ) { noNBTText = bonusValues[startIndex][0] + ";" + bonusValueString; bonuses[startIndex] = ""; bonuses[startIndex] = noNBTText; .... break; } } .... ``` The PVS\-Studio warning: V6001 There are identical sub\-expressions 'startIndex' to the left and to the right of the '\=\=' operator\. [ScriptDBCPlayer\.java 289](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L289) V6007 Expression '\!number' is always true\. [ScriptDBCPlayer\.java 289](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L289) V6033 An item with the same key 'startIndex' has already been changed\. [ScriptDBCPlayer\.java 293](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L293) Do warnings really indicate errors? And if so, what does it lead to? Here, a whole detective investigation with an interesting ending awaits us\. First, let's look at the first warning\. Comparing `startIndex` with itself does seem odd\. The intrepid author of the article, donning his detective hat and long cloak, goes to look for clues in the class\. The evidence is lying right nearby: File: [ScriptDBCPlayer\.java\(224\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L224): ```cpp .... int num = -1; boolean number; try { num = Integer.parseInt(bonusID); number = true; } catch (Exception var33) { number = false; } for (int i = 0; i < bonuses.length; ++i) { .... if (number && i == num || !number && bonusValues[i][0].equals(bonusID) ) { bonuses[i] = ""; break; } } .... ``` The code looks very similar, but it's no coincidence that in our team, we call the author of the article "The Keen Eye"—he found the difference\! In the second case, the variables are **different** in the `try` block and in the `for` loop: they're `num` and `i`\. In the first fragment, we first initialize `startIndex` in the `try` block, and then iterate through it in a `for` loop\. Therefore, in the second fragment, two different variables are compared in an identical check: `num` and `i`\. In the first one, we have what the analyzer pointed out—`startIndex == startIndex`\. So now we know what the first fragment was supposed to look like\. **But what does the error impact**? At this point, the detective noticed an important clue in the name of the class itself: the `Script` prefix means that the class is used in the game scripting mechanism\. The method with error is responsible for assigning bonuses to game attributes such as "Strength", "Agility", etc\. Logically, the method is supposed to read a specific bonus index from the in\-game script and then apply the corresponding modifier\. So how should this have worked? First, we need to parse the bonus index in the `try` block: ```cpp try { num = Integer.parseInt(bonusID); number = true; } .... ``` And then, in the loop, set the passed value to the bonus: ```cpp for (int i = 0; i < bonuses.length; ++i) { .... if (number && i == num || ....) { noNBTText = bonusValues[i][0] + ";" + bonusValueString; bonuses[i] = ""; bonuses[i] = noNBTText; .... break; } } ``` But in the fragment with the error, the attribute index obtained from the script is overwritten with zero and then compared with itself: ```cpp try { startIndex = Integer.parseInt(bonusID); number = true; } catch (Exception var34) { number = false; } for (startIndex = 0; startIndex < bonuses.length; ++startIndex) { .... if (number && startIndex == startIndex || // <= !number && bonusValues[startIndex][0].equals(bonusID) ) { noNBTText = bonusValues[startIndex][0] + ";" + bonusValueString; .... } } ``` That is, the bonus value passed in the script is always assigned to the **first** bonus because `startIndex` is overwritten in the `for` condition and becomes zero, and then is compared with itself\. Now the problem is crystal clear\. Let's look at the consequences of the error\. First, we need to write an in\-game script that will create bonuses for "Strength"\. First, let's create two bonuses with values of 1, and then set their values to 5 and 15, respectively:  This is what in\-game scripts look like\. We created bonuses for "Strength" with indices 0 and 1, and then assigned them the values\. Now, in theory, if we display these two bonuses from "Strength", we will obtain 5 and 15? If there had been no error in the code, that's what would have happened\. However, as we understood from the block above, the error leads to the value of the first bonus being overwritten\. To confirm this behavior, let's write another in\-game script that simply prints the bonus values:  As a result, we get:  Indeed, the second bonus in "Power" also has an initial value of 1, while the first bonus now equals 15\. It turns out that we first set the first bonus to 5, and then overwrite it with a value of 15\. Our hypothesis above turns out to be true\. The case has been solved\. Given the amount of detective work involved and the fact that this bug can be reliably reproduced in practice, we confidently award it the **honorable first place** in our top ten\! ## Wrap\-up Yay\! We've made it through all ten of the most bizarre, heart\-warming, and educational bugs, and we hope you enjoyed the journey as much as we did\! If you have your own thoughts about this top, or maybe a favorite bug of your own, we'd be happy to see you share them in the comments\. If you want to read more about checking other projects, we invite you on our [blog](https://pvs-studio.com/en/blog/posts/) or on [this page](https://pvs-studio.com/en/blog/inspections/)\. If you feel inspired to check your individual projects and want to try PVS\-Studio analyzer yourself, follow the [link](https://pvs-studio.com/en/pvs-studio/try-free/)\. And if you're a student, teacher, or maintainer of open\-source projects in C, C\+\+, C\#, or Java, you can use PVS\-Studio for free\. All the details are [here](https://pvs-studio.com/en/blog/posts/1286/)\. All that's left is to say goodbye for now\. Happy Holidays\! See ya soon\!