# Checking the Code of LDAP\-Server ReOpenLDAP on Our Readers' Request In this article, I'd like to talk about the analysis of ReOpenLDAP project\. It was developed to help solve issues that PAO \(PJSC\) MegaFon, Russia's largest mobile network operator, was faced with when employing OpenLDAP in their infrastructure\. ReOpenLDAP is now successfully used in MegaFon affiliates all over Russia, so we thought it would be interesting to check such a high\-load project as this one with our static analyzer PVS\-Studio\. ![0422_ReOpenLDAP/image1.png](https://import.viva64.com/docx/blog/0422_ReOpenLDAP/image1.png) ## Introduction [ReOpenLDAP](https://github.com/erthink/reopenldap), also known as "TelcoLDAP", is a fork of [OpenLDAP](https://www.openldap.org/) project, created by Russian developers for use in the telecommunication industry, with a lot of bug fixing and addition of multi\-master clustering with a hot replication\. ReOpenLDAP is an open\-source C implementation of an [LDAP](https://en.wikipedia.org/wiki/Lightweight_Directory_Access_Protocol)\-protocol server\. ReOpenLDAP shows a high level of performance: * Up to 50 thousand LDAP\-changes per second * Up to 100 thousand LDAP\-queries per second It should be noted that ReOpenLDAP inherited 3185 _goto_ statements from OpenLDAP, which complicate the analysis process quite a lot\. Despite that, PVS\-Studio still managed to find a certain amount of errors\. ## Please sign up for beta testing of PVS\-Studio for Linux What made this article possible is the development of PVS\-Studio's Linux version that we have started recently: it is on Linux that the check of ReOpenLDAP project was done\. There is a threat, however, that the Linux version may cease to exist before it is out as we don't see much interest from potential users\. If you look at some forum discussions, you might think PVS\-Studio's biggest problem is the lack of support for Linux, but when we started looking for beta testers, very few responded\. Note: the story about our search of enthusiasts was told in the article "[PVS\-Studio confesses its love for Linux](https://pvs-studio.com/en/blog/posts/0415/)"\. I should note that we are not that much concerned about the beta test\. For some reason, some people treat the whole thing as if we have started this campaign purposely to attract programmers to do the job of free testers for us\. That's far from true, of course: we could test our tool on our own\. It's just that the small number of responses suggests that we should probably slow down or even pause our work on that version\. Unfortunately, there are really very few people willing to participate\. In light of all that, Unicorn is calling out to all Linux\-programmers\. ![0422_ReOpenLDAP/image2.png](https://import.viva64.com/docx/blog/0422_ReOpenLDAP/image2.png) Please sign up for beta testing of PVS\-Studio's Linux version: that's how we can see that people are really interested in our tool\. Here is a reminder on how to apply\. If you want to help us in testing PVS\-Studio on Linux, e\-mail us at _support@viva64\.com_\. Specify "PVS\-Studio for Linux, Beta" as the message subject so that we could deal with e\-mails quicker\. Please send your message from your corporate e\-mail address and make sure to write a few words about yourself\. We will appreciate help from everyone, but our potential customers' wishes and suggestions will be considered in the first place\. Also, please answer the following questions in your e\-mail: * With what operating system are you going to use the analyzer? * What IDE do you use? * What compiler do you use to build your projects? * What build system do you use? Once a runnable version is ready, we will e\-mail everyone who has applied\. Thank you all in advance\! ## Analysis results ### Operation\-precedence bug **PVS\-Studio diagnostic message**: [V593](https://pvs-studio.com/en/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B \=\= C' kind\. The expression is calculated as following: 'A \= \(B \=\= C\)'\. mdb\_dump\.c 150 ```cpp static int dumpit(....) { .... while ((rc = mdb_cursor_get(...) == MDB_SUCCESS)) { .... } .... } ``` The author misplaced the closing parenthesis in the _while_ loop's condition, which caused an operation\-precedence error: the comparison is executed first, and then its result is written to the _rc_ variable\. This is how the code should be fixed: ```cpp while ((rc = mdb_cursor_get(...)) == MDB_SUCCESS) { .... } ``` ### Using a null pointer **PVS\-Studio diagnostic message**: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'key' pointer was utilized before it was verified against nullptr\. Check lines: 1324, 1327\. mdb\.c 1324 ```cpp char * mdb_dkey(MDB_val *key, char *buf) { .... unsigned char *c = key->mv_data; // <= .... if (!key) // <= return ""; .... } ``` The _key _pointer is tested for _NULL _in the _if_ block, which means that the programmer assumes that this pointer can be null\. However, it was already used without any check a few lines earlier\. To avoid this error, you need to check the _key_ pointer before using it\. A similar error: * V595 The 'key' pointer was utilized before it was verified against nullptr\. Check lines: 7282, 7291\. mdb\.c 7282 ### Suspicious ternary operator **PVS\-Studio diagnostic message**: [V583](https://pvs-studio.com/en/docs/warnings/v583/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: "vlvResult"\. common\.c 2119 ```cpp static int print_vlv(....) { .... tool_write_ldif( ldif ? LDIF_PUT_COMMENT : LDIF_PUT_VALUE, ldif ? "vlvResult" : "vlvResult", buf, rc ); // <= } .... } ``` The ternary operator in question will return the same value regardless of the condition\. Judging by other similar fragments in the source files, we are dealing with a typo here and the code should actually look like this: ```cpp .... tool_write_ldif( ldif ? LDIF_PUT_COMMENT : LDIF_PUT_VALUE, ldif ? "vlvResult: " : "vlvResult", buf, rc ); .... ``` ### Possible typo in a field name **PVS\-Studio diagnostic message**: [V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(s\-\>state\.r \=\= 0\)' condition was already verified in line 147\. rurwl\.c 148 ```cpp void rurw_r_unlock(....) { .... if (s->state.r == 0) { // <= if (s->state.r == 0) // <= s->thr = 0; p->rurw_readers -= 1; } .... } ``` One condition is checked twice\. Looking at similar fragments in the source files, for example: ```cpp void rurw_w_unlock(....) { .... if (s->state.w == 0) { if (s->state.r == 0) s->thr = 0; p->rurw_writer = 0; } .... } ``` I'd say that one of the conditions was meant to actually check if _s\-\>state\.w \=\= 0_\. It's just an assumption, but the authors should examine this code anyway and either fix one of the conditions or remove the duplicate check\. Another similar error: * V571 Recurring check\. The 'def\-\>mrd\_usage & 0x0100U' condition was already verified in line 319\. mr\.c 322 ### Overwriting a parameter **PVS\-Studio diagnostic message**: [V763](https://pvs-studio.com/en/docs/warnings/v763/) Parameter 'rc' is always rewritten in function body before being used\. tls\_o\.c 426 ```cpp static char * tlso_session_errmsg(...., int rc, ....) { char err[256] = ""; const char *certerr=NULL; tlso_session *s = (tlso_session *)sess; rc = ERR_peek_error(); // <= .... } ``` In this function, the value of the _rc_ parameter is always overwritten before it is used\. Perhaps _rc_ should be removed from the parameter list\. ### Incorrect format specifier **PVS\-Studio diagnostic message**: [V576](https://pvs-studio.com/en/docs/warnings/v576/) Incorrect format\. Consider checking the fourth actual argument of the 'snprintf' function\. The SIGNED argument of memsize type is expected\. conn\.c 309 ```cpp struct Connection { .... unsigned long c_connid; .... } .... static int conn_create(....) { .... bv.bv_len = snprintf( buf, sizeof( buf ), "cn=Connection %ld", // <= c->c_connid ); .... } ``` The _%ld_ format specifier does not correspond to the _c\-\>c\_connid_ argument passed to _snprintf_\. Instead, _%lu_ should be used, which is the proper specifier for _unsigned long_\. Using _%ld_ instead of _%lu_ will result in printing wrong values if the arguments are large enough\. Other similar errors: * V576 Incorrect format\. Consider checking the third actual argument of the 'fprintf' function\. The SIGNED integer type argument is expected\. ure\.c 1865 * V576 Incorrect format\. Consider checking the third actual argument of the 'fprintf' function\. The SIGNED argument of memsize type is expected\. tools\.c 211 * V576 Incorrect format\. Consider checking the fourth actual argument of the 'fprintf' function\. The UNSIGNED integer type argument is expected\. mdb\.c 1253 ### Undereferenced pointer **PVS\-Studio diagnostic message**: [V528](https://pvs-studio.com/en/docs/warnings/v528/) It is odd that pointer to 'char' type is compared with the '\\0' value\. Probably meant: \*ludp\-\>lud\_filter \!\= '\\0'\. backend\.c 1525 ```cpp int fe_acl_group(....) { .... if ( ludp->lud_filter != NULL && ludp->lud_filter != '\0') // <= { .... } } ``` The programmer wanted to check for a null pointer or an empty string but forgot to dereference the _ludp\-\>lud\_filter_ pointer, so it is simply tested for _NULL_ twice\. The pointer should be dereferenced: ```cpp .... if ( ludp->lud_filter != NULL && *ludp->lud_filter != '\0') .... ``` Other unused pointers: * V528 It is odd that pointer to 'char' type is compared with the '\\0' value\. Probably meant: \*\(\* lsei\)\-\>lsei\_values\[0\] \=\= '\\0'\. syntax\.c 240 * V528 It is odd that pointer to 'char' type is compared with the '\\0' value\. Probably meant: \*\(\* lsei\)\-\>lsei\_values\[1\] \!\= '\\0'\. syntax\.c 241 ### Redundant check **PVS\-Studio diagnostic message**: [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: \!saveit\. syncprov\.c 1510 ```cpp static void syncprov_matchops( Operation *op, opcookie *opc, int saveit ) { .... if ( saveit || op->o_tag == LDAP_REQ_ADD ) { .... } else if ( op->o_tag == LDAP_REQ_MODRDN && !saveit ) { .... } .... } ``` _saveit _is tested for _null_ in the _else_ branch, which doesn't make sense as it was already checked in the first condition\. Such a redundant check only complicates the code\. Perhaps it's not even an error and the programmer actually wanted to check something else instead\. However, the first option is more likely, so the code should be simplified: ```cpp if ( saveit || op->o_tag == LDAP_REQ_ADD ) { .... } else if ( op->o_tag == LDAP_REQ_MODRDN ) { .... } ``` ### Dangerous use of realloc **PVS\-Studio diagnostic message**: [V701](https://pvs-studio.com/en/docs/warnings/v701/) realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'lud\.lud\_exts' is lost\. Consider assigning realloc\(\) to a temporary pointer\. ldapurl\.c 306 ```cpp int main( int argc, char *argv[]) { .... lud.lud_exts = (char **)realloc( lud.lud_exts, sizeof( char * ) * ( nexts + 2 ) ); .... } ``` An expression of the _foo \= realloc\(foo, \.\.\.\.\)_ kind is potentially dangerous\. When memory cannot be allocated, _realloc_ returns a null pointer, overwriting the previous pointer value\. To avoid this, it is recommended that you save the pointer's value in an auxiliary variable before using _realloc_\. ### Rewriting a value **PVS\-Studio diagnostic message**: [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'ca\.argv' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 7774, 7776\. bconfig\.c 7776 ```cpp int config_back_initialize( BackendInfo *bi ) { .... ca.argv = argv; // <= argv[ 0 ] = "slapd"; ca.argv = argv; // <= ca.argc = 3; ca.fname = argv[0]; .... } ``` If this code is correct, the first assignment is redundant and should be removed\. ## Conclusion ReOpenLDAP is a project designed to maintain stability under high load, so the developers take the testing stage very seriously and use special tools such as ThreadSanitizer and [Valgrind](https://www.valgrind.org/)\. We have seen, however, that sometimes it's not enough, as PVS\-Studio found a number of errors, though few\. Static analysis can detect errors at the earliest development stages before testing, helping save a lot of developers' time\. This is the reason why you should use analyzers regularly, not occasionally like we do to showcase PVS\-Studio\. Welcome to [download](https://pvs-studio.com/en/pvs-studio/download/) and try PVS\-Studio static analyzer with your own projects\.