# How PVS\-Studio checked ELKI in January If you feel like the New Year just came, and you missed the first half of January, then all this time you've been busy looking for tricky bugs in the code you maintain\. It also means that our article is what you need\. PVS\-Studio has checked the ELKI open source project to show you errors that may occur in the code, how cunningly they can hide there, and how you can deal with them\. ![0791_ELKI/image1.png](https://import.viva64.com/docx/blog/0791_ELKI/image1.png) ## What kind of library is ELKI? The abbreviation [ELKI](https://elki-project.github.io/) stands for **E**nvironment for Deve**l**oping **K**DD\-Applications Supported by **I**ndex\-Structures\. This project is written in Java and is designed for data mining\. Most users of this library are students, researchers, data scientists, and software engineers\. No wonder since this library was developed for research only\. The algorithms included in the library are mainly related to cluster analysis and outlier detection that may indicate experimental errors\. Every year, there are more and more references to research conducted using ELKI\. On the developers' official website, you can find a whole [list](https://elki-project.github.io/references) of scientific works that were conducted via this library\. From year to year, this list is constantly growing and being supplemented, and the topics of scientific works are getting more diverse\. The only obstacle to using this library may be the AGPL license, which requires the full source code of the application to be open to any user\. In other words, you won't be able to use this library in commercial projects with closed source code\. ## Let's start checking The ELKI [library](https://github.com/elki-project/elki/tree/474659eba15a27525db5f4c76162f28a7f1733aa) contains 2 630 java files with 186 444 lines of code, except for comments\. Therefore, the project seems to be quite small compared to some open source giants\. However, the small size of the library does not guarantee the absence of errors\. As you know, errors can occur in any projects, regardless of their scale\. That's why we are here now\! Let's see my top 10 most interesting warnings found when viewing the report\. ## Unchecked boundaries [V6001](https://pvs-studio.com/en/docs/warnings/v6001/) There are identical sub\-expressions 'bounds\[j \+ 1\]' to the left and to the right of the '\!\=' operator\. CLIQUEUnit\.java\(252\) ```cpp private boolean checkDimensions(CLIQUEUnit other, int e) { for(int i = 0, j = 0; i < e; i++, j += 2) { if (dims[i] != other.dims[i] || bounds[j] != other.bounds[j] || bounds[j + 1] != bounds[j + 1]) { return false; } } return true; } ``` In the _if_ block of the _checkDimensions_ method, the value of _bounds\[j \+ 1\]_ is checked for inequality with its own value\. Naturally, this condition will always be false, and one of the boundaries will never be checked\. That is, the _checkDimensions_ method may return _true_ when checking, even if the boundaries of the arrays do not match\. The correct check in the _if_ block must look this way: ```cpp bounds[j + 1] != other.bounds[j + 1] ``` ## Lazy constructor [V6022](https://pvs-studio.com/en/docs/warnings/v6022/) Parameter 'updates' is not used inside constructor body\. DataStoreEvent\.java\(60\) [V6022](https://pvs-studio.com/en/docs/warnings/v6022/) Parameter 'removals' is not used inside constructor body\. DataStoreEvent\.java\(60\) ```cpp public DataStoreEvent(DBIDs inserts, DBIDs removals, DBIDs updates) { super(); this.inserts = inserts; this.removals = inserts; this.updates = inserts; } ``` Two warnings were received for this code snippet\. Here, three parameters are passed to the _DataStoreEvent_ constructor, two of which are not used for initialization\. Looking closer at the code in the constructor, it becomes clear that the programmer used copy\-paste and forgot to correct the names of variables that should have been involved in initialization\. If you go even further and take a closer look at the _DataStoreEvent_ class, you can see three methods in it that are actively using the above constructor\. ```cpp public static DataStoreEvent insertionEvent(DBIDs inserts) { return new DataStoreEvent(inserts, DBIDUtil.EMPTYDBIDS, DBIDUtil.EMPTYDBIDS); } public static DataStoreEvent removalEvent(DBIDs removals) { return new DataStoreEvent(DBIDUtil.EMPTYDBIDS, removals, DBIDUtil.EMPTYDBIDS); } public static DataStoreEvent updateEvent(DBIDs updates) { return new DataStoreEvent(DBIDUtil.EMPTYDBIDS, DBIDUtil.EMPTYDBIDS, updates); } ``` Due to incorrect initialization in the constructor, the behavior of these methods will also differ from what is expected\. The correct code version should be as follows: ```cpp this.inserts = inserts; this.removals = removals; this.updates = updates; ``` ## Opaque variable [V6012](https://pvs-studio.com/en/docs/warnings/v6012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value '0\.5'\. ClusterHullVisualization\.java\(173\), ClusterHullVisualization\.java\(173\) ```cpp public void fullRedraw() { .... boolean flat = (clusters.size() == topc.size()); // Heuristic value for transparency: double baseopacity = flat ? 0.5 : 0.5; .... } ``` In this code snippet, the value of the _baseopacity_ variable will always be 0\.5, regardless of what the value of the _flat_ variable is\. It's clear that there should have been two different values, but due to inattention, the author of the code forgot to correct one of them\. The mistake is simple and very disappointing\. It immediately strikes your eye if you exclude lines of code that are not related to this variable from the method\. But it's not an easy task to find a mistake in a method consisting of almost 90 lines\. It looks like this is exactly what became the obstacle for the programmer\. ## In pursuit of something beyond the limits [V6025](https://pvs-studio.com/en/docs/warnings/v6025/) Index '1' is out of bounds\. GeneratorStatic\.java\(104\) ```cpp @Override public double[] computeMean() { // Not supported except for singletons. return points.size() == 1 ? points.get(1) : null; } ``` In the _computeMean_ method, the code author checks that the size of the _points_ collection is equal to one\. If so, the developer tries to return \.\.\. a collection element with index 1 from the method\. Since indexing starts from zero, an _IndexOutOfBoundsException_ is inevitable\. The correct version of the code must look like this: ```cpp return points.size() == 1 ? points.get(0) : null; ``` ## Is it possible to divide by zero? [V6020](https://pvs-studio.com/en/docs/warnings/v6020/) Divide by zero\. The range of the 'referenceSetSize' denominator values includes zero\. PreDeConNeighborPredicate\.java\(138\) ```cpp protected PreDeConModel computeLocalModel(DoubleDBIDList neighbors, ....) { final int referenceSetSize = neighbors.size(); .... // Shouldn't happen: if(referenceSetSize < 0) { LOG.warning("Empty reference set – should at least include the query point!"); return new PreDeConModel(Integer.MAX_VALUE, DBIDUtil.EMPTYDBIDS); } .... for(int d = 0; d < dim; d++) { s[d] /= referenceSetSize; mvVar.put(s[d]); } .... } ``` Every programmer knows that you can't divide by zero\. Every programmer tries to avoid the occurrence of such an error in their code\. And it works in most cases\. But not always\. In this code snippet, everything depends on the size of the _neighbors_ parameter passed to the _computeLocalModel_ method\. The code author checks the _neighbors_ size and cuts off the values that are less than zero by checking in the _if_ statement, but isn't taking any actions if the size is zero\. Since the check of _referenceSetSize < 0_ doesn't make any sense, and the message for logging also says about an empty _set_, all of that seems like a typo\. The check of _referenceSetSize \=\= 0_ was most likely to be here\. If an empty _neighbors_ container is passed in this method anyway, the division by zero will occur in the _for_ loop\. We can only hope that this will never really happen\. ## Endless initialization [V6062](https://pvs-studio.com/en/docs/warnings/v6062/) Possible infinite recursion inside the 'setInitialMeans' method\. Predefined\.java\(65\), Predefined\.java\(66\) ```cpp public void setInitialMeans(List initialMeans) { this.setInitialMeans(initialMeans); } ``` This one\-line method is a true recursion\. It's difficult to guess what exactly the code author wanted to write here\. The only line of this method probably should have looked as follows: ```cpp this.setInitialMeans(initialMeans.toArray(new double[0][0])); ``` Since there is another method with the same name in this class, the programmer most likely wanted to pass data for initialization to it, but in the end, something went wrong\. This is what the body of the second method looks like, by the way: ```cpp public void setInitialMeans(double[][] initialMeans) { double[][] vecs = initialMeans.clone(); // TODO: deep copy? this.initialMeans = vecs; } ``` ## Lost weight [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;\. ProbabilityWeightedMoments\.java\(130\) ```cpp public static double[] alphaBetaPWM(...., final int nmom) { final int n = adapter.size(data); final double[] xmom = new double[nmom << 1]; double aweight = 1. / n, bweight = aweight; for(int i = 0; i < n; i++) { .... for(int j = 1, k = 2; j < nmom; j++, k += 2) { xmom[k + 1] += val * (aweight *= (n - i - j + 1) / (n - j + 1)); xmom[k + 1] += val * (bweight *= (i - j + 1) / (n - j + 1)); } } return xmom; } ``` In the _for_ loop of this code snippet, the _\(n \- i \- j \+ 1\) / \(n \- j \+ 1\) _expression is converted implicitly from the _int_ type to the _double_ type\. In this case, the loss of accuracy may be quite different: from a few digits after the decimal point to a complete nullification if the number modulo is less than one\. Probably, it's not exactly the expected behavior given that the _xmom_ array is of the _double_ type\. The _\(n – i – j \+ 1\) / \(n – j \+ 1\)_ expression helps to prove that the programmer meant something completely different here\. Let's say _n – j \+ 1_ equals _x_\. Then we get the expression: _\(x – i\) / x_\. This expression will always give 0 for integer division unless we start going into negative ranges\. But since the values of _n_ in this code snippet are always greater than zero, we can conclude that the programmer did not intend to use integer division here\. Not to lose precision, an explicit conversion to the _double_ type is required: ```cpp xmom[k + 1] += val * (aweight *= (double) (n - i - j + 1) / (n - j + 1)); xmom[k + 1] += val * (bweight *= (double) (i - j + 1) / (n - j + 1)); ``` ## Out of bounds [V6079](https://pvs-studio.com/en/docs/warnings/v6079/) Value of the 'splitpoint' variable is checked after use\. Potential logical error is present\. KernelDensityFittingTest\.java\(97\), KernelDensityFittingTest\.java\(97\) ```cpp public final void testFitDoubleArray() throws IOException { .... int splitpoint = 0; while(fulldata[splitpoint] < splitval && splitpoint < fulldata.length) { splitpoint++; } .... } ``` In this code fragment, in the _while_ loop, first, the _fulldata_ array element with the _splitpoint_ index is compared with the value of the _splitval_ variable, and only then we check whether the _splitpoint_ value is smaller than the size of the array itself\. These two checks in the _while_ loop need to be changed, otherwise, you can very easily find yourself outside the array's bounds\. ## Unreachable code [V6019](https://pvs-studio.com/en/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. Tokenizer\.java\(172\) [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'c \!\= '\\n'' is always true\. Tokenizer\.java\(169\) ```cpp public String getStrippedSubstring() { int sstart = start, ssend = end; while(sstart < ssend) { char c = input.charAt(sstart); if(c != ' ' || c != '\n' || c != '\r' || c != '\t') { break; } ++sstart; } .... } ``` Two diagnostics decided to act together this time\. They both issued warnings\. The V6019 diagnostic pointed to an unreachable code fragment: _\+\+sstart_, and V6007 pointed to a condition in the _if_ statement that will always be true\. Why will there always be true in the _if_ block? The answer is very simple\. In this statement several conditions are checked at once: _c \!\=' '_ or _c \!\= _ _'\\n'_, or _c \!\= '\\r'_, or _c \!\= '\\t'_\. Whatever the input data, some of those will be true\. Even if one of the checks is _false_, the next check will return _true_, and because of the _\|\|_ \(or\) operator, the condition in _if_ will eventually be true\. The condition in the _if_ block will always be true\. Therefore, the _break_ statement, which prematurely ends the _while_ loop, will trigger\. As a result, the increment of the _sstart_ variable will never be executed\. This is exactly what the V6019 diagnostics noticed and started sounding the alarm\. Most likely, the programmer wanted to write something like this: ```cpp if(c != ' ' && c != '\n' && c != '\r' && c != '\t') ``` ## Override but check [V6009](https://pvs-studio.com/en/docs/warnings/v6009/) Function 'equals' receives an odd argument\. An object 'other\.similarityFunction' is used as an argument to its own method\. AbstractSimilarityAdapter\.java\(91\) ```cpp @Override public boolean equals(Object obj) { if(obj == null) { return false; } if(!this.getClass().equals(obj.getClass())) { return false; } AbstractSimilarityAdapter other = (AbstractSimilarityAdapter) obj; return other.similarityFunction.equals(other.similarityFunction); } ``` The code author decided to override the _equals_ method in the _AbstractSimilarityAdapter_ class\. If it is assumed that objects in the program will be stored in containers, or checked for equality, the _equals_ override is required\. However, the whole author's idea was spoiled by the method's last line, in which _equals_ is called for the same object\. As a result, even the most common comparison will be incorrect\. The code was probably meant to be like this: ```cpp return this.similarityFunction.equals(other.similarityFunction); ``` I'd like to note that this error pattern is often found in programs in different languages, and not only in Java\. We've got an article '[The Evil within the Comparison Functions](https://pvs-studio.com/en/blog/posts/cpp/0509/)' on this topic\. You must read it if you're interested in finding out why programmers often make mistakes in fairly simple functions designed to compare two objects\. ## Let's recap As you can see, mistakes may appear in any project\. Some of them are simple and disappointing, others are cunning and tricky\. And it doesn't matter whether your project is small or large\. It doesn't matter if you're a professional developer or just starting to write code\. Mistakes can appear anywhere in your program and hide safely from your eyes\. For every developer and every project, static code analysis is an indispensable tool\. As essential as a code review or unit tests\. That's why, we recommend using static code analysis in your work [right now](https://pvs-studio.com/en/pvs-studio/download/), so that in the New Year your code will become better and clearer than in the previous one\. [![getTrialImageLink](https://wcdn.pvs-studio.com/media/get_trial_insert.png)](https://pvs-studio.com/en/pvs-studio-download/) ## In conclusion It's not the first time we are checking packages related to calculations, statistics, and scientific researches\. And every time we worry about mistakes in the code of such projects\. Can we trust the data calculated using these libraries? Of course, it's incredibly difficult to write code without mistakes, and in practice, it's even impossible\. However, libraries used as building blocks in scientific, medical, and research programs should be checked as carefully as possible\. The code should be as clean as possible\. After all, every mistake can lead to very unpleasant consequences in the future\. In any case, static analysis will prove to be extremely useful for such projects\. We have already covered this topic in some of our other articles, so if this question is interesting for you, don't hesitate to check out some of them: 1. [Analysis of the Trans\-Proteomic Pipeline \(TPP\) project](https://pvs-studio.com/en/blog/posts/cpp/0156/)\. 1. [The big calculator gone crazy](https://pvs-studio.com/en/blog/posts/cpp/0212/)\. 1. [Can we trust the libraries we use?](https://pvs-studio.com/en/blog/posts/cpp/0271/) 1. [Analyzing the code of ROOT, scientific Data Analysis Framework](https://pvs-studio.com/en/blog/posts/cpp/0682/)\. 1. [NCBI Genome Workbench: scientific research under threat](https://pvs-studio.com/en/blog/posts/cpp/0591/)\.