# OpenJDK check by PVS\-Studio Nowadays a lot of projects are opening their source code and letting those who are interested in the development of it edit the code\. We'll check one such project \- OpenJDK and help the developers improve the code\. ![0404_OpenJDK/image1.png](https://import.viva64.com/docx/blog/0404_OpenJDK/image1.png) ## Introduction [OpenJDK ](http://openjdk.java.net/)\(Open Java Development Kit\) \- a project for the creation and implementation of Java \(Java SE\) platform, which is now free and open source\. The project was started in 2006, by the Sun company\. The project uses multiple languages\- C, C\+\+, and Java\. We are interested in the source code written in C and C\+\+\. Let's take the 9th version of OpenJDK\. The code of this implementation of Java platform is available at the [Mercurial repository](http://hg.openjdk.java.net/jdk9)\. The project was scanned with PVS\-Studio static code analyzer\. It has plenty of diagnostic rules, helping it to find a large number of errors in the code, and is also able to find those that are hard to detect during simple code reviews\. Some of these errors do not affect the logic of the program, and some can lead to sad consequences during the program execution\. There are various [examples of errors](https://pvs-studio.com/en/blog/examples/) on the analyzers website, which were found in other open\-source projects\. This tool is able to analyze projects that are written in C, C\+\+, and C\#\. The trial version of the analyzer can be downloaded through this [link](https://pvs-studio.com/en/pvs-studio/download/)\. ## Errors in logical expressions ![0404_OpenJDK/image2.png](https://import.viva64.com/docx/blog/0404_OpenJDK/image2.png) First let's take a look at errors in logical expressions: ```cpp int StubAssembler::call_RT(....) { #ifdef _LP64 // if there is any conflict use the stack if (arg1 == c_rarg2 || arg1 == c_rarg3 || arg2 == c_rarg1 || arg1 == c_rarg3 || arg3 == c_rarg1 || arg1 == c_rarg2) { .... } ``` **PVS\-Studio** **warning:** V501 There are identical sub\-expressions 'arg1 \=\= c\_rarg3' to the left and to the right of the '\|\|' operator\. c1\_Runtime1\_x86\.cpp 174 The analyzer tells us about the duplication of _arg1_ _\=\=_ _c\_rarg3 _check_\._ There is a redundant check here, or worse, a logical error\. Perhaps something else should be checked instead of the duplicated condition\. This code is worth revising for sure\. There is one more recurring expression _arg1 \=\= c\_rarg2_: in the same condition\. **PVS\-Studio** **warning:** V501 There are identical sub\-expressions 'arg1 \=\= c\_rarg2' to the left and to the right of the '\|\|' operator\. c1\_Runtime1\_x86\.cpp 174 These warnings are great proof of the usefulness of the analyzer\. It's very simple to make a mistake in a large number of similar expressions; and they're hard to notice during the visual code review\. In the next fragment we have a "nonideal" check in the condition of the _Ideal_ method: ```cpp Node *AddLNode::Ideal(PhaseGVN *phase, bool can_reshape) { .... if( op2 == Op_AddL && in2->in(1) == in1 && op1 != Op_ConL && 0 ) { .... } ``` **PVS\-Studio warning:** V560 A part of conditional expression is always false: 0\. addnode\.cpp 435 It's quite odd to use 0 in a logical expression\. Most likely, this code is still in the process of development, and to debug it, this condition was not made executable\. The necessary comments are missing in the code, and chances are, in the future will be forgotten\. This bug can result in everything inside of this condition l never being executed, and as a result of the logical expression, evaluation is always false\. ## Operation precedence Quite often, programmers put too much faith in their knowledge of precedence, and don't enclose the component parts of the parentheses of a complex expression: ```cpp int method_size() const { return sizeof(Method)/wordSize + is_native() ? 2 : 0; } ``` **PVS\-Studio warning:** V502 Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\+' operator\. method\.hpp 249 In this case I don't know the specifics of the code, but I have a suspicion, that it was intended to choose a value '2' or '0' depending on the result of the is\_native\(\) function call, but the expression has a different evaluation order\. First there will be addition \- _sizeof\(Method\)/wordSize_ _\+_ _is\_native\(\)_, and then we'll have the result 0 or 2 returned, i\.e\. the code was probably meant to be like this: ```cpp { return sizeof(Method)/wordSize + (is_native() ? 2 : 0); } ``` This is a very common error with operation precedence\. In the error base of the analyzer we have found the most popular ones, and put them in an article: [Logical Expressions in C/C\+\+\. Mistakes Made by Professionals](https://pvs-studio.com/en/blog/posts/cpp/0390/)\. ## Copy\-Paste ![0404_OpenJDK/image3.png](https://import.viva64.com/docx/blog/0404_OpenJDK/image3.png) The next group of errors is caused by copying the code\. There is no way to work around this favorite method of programmers, so let's investigate the fragments where we have it: ```cpp static int setImageHints(....) { .... if (dstCMP->isDefaultCompatCM) { hintP->allocDefaultDst = FALSE; hintP->cvtToDst = FALSE; } else if (dstCMP->isDefaultCompatCM) { hintP->allocDefaultDst = FALSE; hintP->cvtToDst = FALSE; } .... } ``` **PVS\-Studio warning:** V517 The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 1873, 1877\. awt\_ImagingLib\.c 1873 In this fragment the conditions are the same in _if _and _else if_, as well as the code that should be executed\. The second condition is completely pointless, as it will never be executed\. Another similar case: ```cpp static int expandPackedBCR(JNIEnv *env, RasterS_t *rasterP, int component, unsigned char *outDataP) { .... /* Convert the all bands */ if (rasterP->numBands < 4) { /* Need to put in alpha */ for (y=0; y < rasterP->height; y++) { inP = lineInP; for (x=0; x < rasterP->width; x++) { for (c=0; c < rasterP->numBands; c++) { *outP++ = (unsigned char) (((*inP&rasterP->sppsm.maskArray[c]) >> roff[c]) <scanlineStride; } } else { for (y=0; y < rasterP->height; y++) { inP = lineInP; for (x=0; x < rasterP->width; x++) { for (c=0; c < rasterP->numBands; c++) { *outP++ = (unsigned char) (((*inP&rasterP->sppsm.maskArray[c]) >> roff[c]) <scanlineStride; } } .... } ``` **PVS\-Studio warning:** V523 The 'then' statement is equivalent to the 'else' statement\. awt\_ImagingLib\.c 2927 The executable code is identical in both blocks, so there is no difference, in what is evaluated in the condition\. It makes sense to look at this fragment, and remove the unnecessary branch, or correct the code if a different logic was intended to be here, to avoid duplication\. Two more fragments with identical duplication\. I'll just show them here without citing the code: * V523 The 'then' statement is equivalent to the 'else' statement\. awt\_ImagingLib\.c 3111 * V523 The 'then' statement is equivalent to the 'else' statement\. awt\_ImagingLib\.c 3307 And the last interesting case, caused by the copy paste error: ```cpp Node* GraphKit::record_profiled_receiver_for_speculation(Node* n) { .... ciKlass* exact_kls = profile_has_unique_klass(); bool maybe_null = true; if (java_bc() == Bytecodes::_checkcast || java_bc() == Bytecodes::_instanceof || java_bc() == Bytecodes::_aastore) { ciProfileData* data = method()->method_data()->bci_to_data(bci()); bool maybe_null = data == NULL ? true : <== data->as_BitData()->null_seen(); } return record_profile_for_speculation(n, exact_kls, maybe_null); return n; } ``` **PVS\-Studio warning:** V561 It's probably better to assign value to 'maybe\_null' variable than to declare it anew\. Previous declaration: graphKit\.cpp, line 2170\. graphKit\.cpp 2175 What is going on in this code? A variable _bool maybe\_null \= true;_ is declared before the if block\. Then, when the code in the if clock is executed, a variable with the same name is declared\. After the block is exited, the value of this variable will be lost, and the function call, using this variable, will always be true\. It's good, if the variable was duplicated for the sake of debugging\. Otherwise, this code is executed incorrectly and requires modification: ```cpp maybe_null = data == NULL ? true : data->as_BitData()->null_seen(); ``` ## Pointer handling ![0404_OpenJDK/image4.png](https://import.viva64.com/docx/blog/0404_OpenJDK/image4.png) A programmer should be very careful, and especially attentive, when working with pointers; because during the pointer usage you may get errors that will be hard to detect later on\. As a rule, the main danger is in using not valid pointers, or using pointers without verifying them against null\. Firstly, let's have a look at a case of an explicit use of a null pointer: ```cpp static jint JNICALL cbObjectTagInstance(....) { ClassInstancesData *data; /* Check data structure */ data = (ClassInstancesData*)user_data; if (data == NULL) { data->error = AGENT_ERROR_ILLEGAL_ARGUMENT; return JVMTI_VISIT_ABORT; } .... } ``` **PVS\-Studio** **warning:** V522 Dereferencing of the null pointer 'data' might take place\. util\.c 2424 Completely unclear code with a null pointer, can lead to a program crash\. Perhaps this branch was never executed, that's why some problems were avoided\. There were three more similar fragments in the same file: * V522 Dereferencing of the null pointer 'data' might take place\. util\.c 2543 * V522 Dereferencing of the null pointer 'data' might take place\. util\.c 2601 * V522 Dereferencing of the null pointer 'data' might take place\. util\.c 2760 But in the following cases the possibility of using a null pointer isn't so evident\. This is a very common situation, such warnings are found in almost every project we check\. ```cpp static jboolean visibleClasses(PacketInputStream *in, PacketOutputStream *out) { .... else { (void)outStream_writeInt(out, count); for (i = 0; i < count; i++) { jbyte tag; jclass clazz; clazz = classes[i]; <== tag = referenceTypeTag(clazz); (void)outStream_writeByte(out, tag); (void)outStream_writeObjectRef(env, out, clazz); } } if ( classes != NULL ) <== jvmtiDeallocate(classes); .... return JNI_TRUE; } ``` **PVS\-Studio warning**: V595 The 'classes' pointer was utilized before it was verified against nullptr\. Check lines: 58, 66\. ClassLoaderReferenceImpl\.c 58 In the lower block the pointer is verified against null, so the programmer assumes that a pointer value may be null\. But in the block above we see that the pointer is used without a check\. Thus, if the pointer value is zero, this check won't help us, and the program will terminate\. To fix this error, we should check the pointer that is above two blocks\. I'll give a similar example: ```cpp int InstructForm::needs_base_oop_edge(FormDict &globals) const { if( is_simple_chain_rule(globals) ) { const char *src = _matrule->_rChild->_opType; OperandForm *src_op = globals[src]->is_operand(); assert( src_op, "Not operand class of chain rule" ); return src_op->_matrule ? src_op->_matrule->needs_base_oop_edge() : 0; } // Else check instruction return _matrule ? _matrule->needs_base_oop_edge() : 0; } ``` **PVS\-Studio** **warning:** V595 The '\_matrule' pointer was utilized before it was verified against nullptr\. Check lines: 3534, 3540\. formssel\.cpp 3534 Here the pointer check is carried out below in the ternary operator \- _\_matrule_ _?_ _\_matrule\-\>needs\_base\_oop\_edge\(\)_ _:_ _0;\._ Earlier in the code there is addressing the pointer \- _const_ _char_ _\*src_ _\=_ _\_matrule\-\>\_rChild\-\>\_opType;\._ The recipe to correct it is the same: the pointer should be checked before it is used\. There were quite a few of such spots, so I'll give them as a list here: * V595 The '\_pipeline' pointer was utilized before it was verified against nullptr\. Check lines: 3265, 3274\. output\_c\.cpp 3265 * V595 The 'index\_bound' pointer was utilized before it was verified against nullptr\. Check lines: 790, 806\. c1\_RangeCheckElimination\.cpp 790 * V595 The 'g\_type\_init' pointer was utilized before it was verified against nullptr\. Check lines: 94, 108\. GioFileTypeDetector\.c 94 * V595 The 'classArray' pointer was utilized before it was verified against nullptr\. Check lines: 1169, 1185\. JPLISAgent\.c 1169 * V595 The 'q' pointer was utilized before it was verified against nullptr\. Check lines: 594, 599\. mpi\.c 594 * V595 The 'info\.waiters' pointer was utilized before it was verified against nullptr\. Check lines: 224, 228\. ObjectReferenceImpl\.c 224 * V595 The 'methods' pointer was utilized before it was verified against nullptr\. Check lines: 225, 229\. ReferenceTypeImpl\.c 225 * V595 The 'fields' pointer was utilized before it was verified against nullptr\. Check lines: 433, 437\. ReferenceTypeImpl\.c 433 * V595 The 'nested' pointer was utilized before it was verified against nullptr\. Check lines: 538, 540\. ReferenceTypeImpl\.c 538 * V595 The 'interfaces' pointer was utilized before it was verified against nullptr\. Check lines: 593, 595\. ReferenceTypeImpl\.c 593 * V595 The 'buf' pointer was utilized before it was verified against nullptr\. Check lines: 265, 266\. ps\_proc\.c 265 * V595 The 'monitors' pointer was utilized before it was verified against nullptr\. Check lines: 382, 387\. ThreadReferenceImpl\.c 382 * V595 The 'monitors' pointer was utilized before it was verified against nullptr\. Check lines: 557, 560\. ThreadReferenceImpl\.c 557 * V595 The 'signature' pointer was utilized before it was verified against nullptr\. Check lines: 520, 526\. debugInit\.c 520 * V595 The 'BlackPoint' pointer was utilized before it was verified against nullptr\. Check lines: 192, 208\. cmssamp\.c 192 * V595 The 'nativename' pointer was utilized before it was verified against nullptr\. Check lines: 506, 511\. awt\_Font\.c 506 * V595 The 'pseq\-\>seq' pointer was utilized before it was verified against nullptr\. Check lines: 788, 791\. cmsnamed\.c 788 * V595 The 'GammaTables' pointer was utilized before it was verified against nullptr\. Check lines: 1430, 1434\. cmsopt\.c 1430 Sometimes programmers check the pointers, but do it in a wrong way\. ```cpp FileBuff::FileBuff( BufferedFile *fptr, ArchDesc& archDesc) : _fp(fptr), _AD(archDesc) { .... _bigbuf = new char[_bufferSize]; if( !_bigbuf ) { file_error(SEMERR, 0, "Buffer allocation failed\n"); exit(1); .... } ``` **PVS\-Studio** **warning:** V668 There is no sense in testing the '\_bigbuf' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. filebuff\.cpp 47 In this case the check of the verification of_ \_bigbuf _pointer against null after using the new operator is pointless\._ _In the case of system not being able to allocate the memory, an exception will be thrown, and the function execution will be interrupted\. We can use several approaches to fix this issue\. We could allocate the memory in the _try catch _block, or use _new\(std::nothrow\)_ construction, which won't throw exceptions in case of a failure\. There are several more incorrect checks\. * V668 There is no sense in testing the 'vspace' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. psParallelCompact\.cpp 455 * V668 There is no sense in testing the 'uPtr' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. jni\.cpp 113 The last error that is related to the pointer handling occurred during the explicit casting of one type of pointer to another\. ```cpp mlib_status mlib_convMxNext_f32(...) { mlib_d64 dspace[1024], *dsa = dspace; .... mlib_f32 *fsa; .... if (3 * wid_e + m > 1024) { dsa = mlib_malloc((3 * wid_e + m) * sizeof(mlib_d64)); if (dsa == NULL) return MLIB_FAILURE; } fsa = (mlib_f32 *) dsa; <== .... } ``` **PVS\-Studio warning**: V615 An odd explicit conversion from 'double \*' type to 'float \*' type\. mlib\_ImageConvMxN\_Fp\.c 294 A programmer tries to assign a pointer to float _mlib\_f32_ _\*fsa_ with a pointer _mlib\_d64_ _dspace\[1024\],_ _\*dsa_ _\=_ _dspace_\. But the types float and double have different sizes, so this type cast is most likely erroneous\. The mismatch of the casted types causes the _fsa_ _t_o point to a digit number that is incorrect for the _float_ type_\._ There are two more similar castings in another file, it would be a good thing to check this code, and use correct type castings\. * V615 An odd explicit conversion from 'double \*' type to 'float \*' type\. mlib\_ImageLookUp\_Bit\.c 525 * V615 An odd explicit conversion from 'double \*' type to 'float \*' type\. mlib\_ImageLookUp\_Bit\.c 526 At this point, let's stop looking at errors related to incorrect pointer handling, and let's move on to the other analyzer warnings\. ## Miscellaneous errors ![0404_OpenJDK/image5.png](https://import.viva64.com/docx/blog/0404_OpenJDK/image5.png) The following bug is probably the result of an improper copy\-paste: ```cpp static bool parse_bool (const char **pp, const char *end, unsigned int *pv) { .... /* CSS allows on/off as aliases 1/0. */ if (*pp - p == 2 || 0 == strncmp (p, "on", 2)) *pv = 1; else if (*pp - p == 3 || 0 == strncmp (p, "off", 2)) *pv = 0; else return false; return true; } ``` **PVS\-Studio** **warning:** V666 Consider inspecting third argument of the function 'strncmp'\. It is possible that the value does not correspond with the length of a string which was passed with the second argument\. hb\-shape\.cc 104 Here is a case where a bug doesn't affect the working of the program\. Instead of comparing three symbols, only the first two symbols get compared, but I do not conclude that the author of the code did not do this check deliberately\. As the value in the p buffer can be on or off, then it's enough to compare first two symbols\. But to make it clearer, we can correct the code: ```cpp else if (*pp - p == 3 || 0 == strncmp (p, "off", 3)) ``` There were several more places ```cpp class ProductionState { .... private: // Disable public use of constructor, copy-ctor, ... ProductionState( ) : _production(cmpstr, hashstr, Form::arena) { assert( false, "NotImplemented"); }; ProductionState( const ProductionState & ) : _production(cmpstr, hashstr, Form::arena) { assert( false, "NotImplemented"); }; // Deep-copy }; ``` **PVS\-Studio** **warning:** V690 Copy constructor is declared as private in the 'ProductionState' class, but the default '\=' operator will still be generated by compiler\. It is dangerous to use such a class\. dfa\.cpp 76 In this class the programmer made an attempt to prohibit copying the code, but forgot to add a copy assignment operator to a private area\. It will be generated by default, and will be available for use\. Even if this operator isn't used anywhere in the code, there is a guarantee that it won't be called accidentally in the future\. During the call of such an operator we'll have memberwise copying for a class that should not be copied\. This can cause various effects, even a program crash\. In this case we should add the declaration of the "\=" operator to the private area\. There are two more classes where we see the same problems; it would be great to fix them in such a way so that the "[The Law of The Big Two](https://www.artima.com/cppsource/bigtwo.html)" isn't violated\. * V690 The 'MemRegion' class implements a copy constructor, but lacks the '\=' operator\. It is dangerous to use such a class\. memRegion\.hpp 43 * V690 Copy constructor is declared as private in the 'Label' class, but the default '\=' operator will still be generated by compiler\. It is dangerous to use such a class\. assembler\.hpp 73 The latter looks like a simple typo\. ```cpp bool os::start_debugging(char *buf, int buflen) { int len = (int)strlen(buf); char *p = &buf[len]; .... if (yes) { // yes, user asked VM to launch debugger jio_snprintf(buf, sizeof(buf), "gdb /proc/%d/exe %d", os::current_process_id(), os::current_process_id()); os::fork_and_exec(buf); yes = false; } return yes; } ``` **PVS\-Studio** **warning:** V579 The jio\_snprintf function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the second argument\. os\_linux\.cpp 6094 A programmer wanted to pass a buffer size, but didn't take into account that it is not a locally declared array, but a pointer that comes in the function argument\. In the result of evaluation of _sizeof\(buf\)_ we'll get not the buffer size, but the pointer size, which will be either 4 or 8 bytes\. This bug can be easily fixed, as the buffer length was already received earlier in the code: _int_ _len_ _\=_ _\(int\)strlen\(buf\);\._ The correct variant will be as follows: ```cpp jio_snprintf(buf, len .... ``` ## Conclusion ![0404_OpenJDK/image6.png](https://import.viva64.com/docx/blog/0404_OpenJDK/image6.png) It's always amusing to check a project which is used and maintained by a large number of people\. We found a considerable number of errors; in this article we have described only a part of them, the rest require more thorough investigation\. Those bugs we found, are further proof of the usefulness of an analyzer, as it allows the detection of such errors as would otherwise be hard to detect during simple code review\. The most effective way is to use an analyzer on a regular basis, as it will save a lot of time that could be spent debugging the program\. And again, I remind you, that you can try the analyzer on your project by downloading the [trial version](https://pvs-studio.com/en/pvs-studio/download/)\.