At the request of readers, check the LDAP server code ReOpenLDAP

In this article I want to talk about the verification of the project ReOpenLDAP. This project was implemented to solve the problems encountered during the operation of OpenLDAP in the infrastructure of MegaFon PJSC - the largest mobile operator in Russia. Now ReOpenLDAP successfully works in MegaFon branches throughout Russia, so it would be interesting to check such a highly loaded project with the help of the PVS-Studio static analyzer.

Introduction

ReOpenLDAP , also known as “TelcoLDAP”, is a fork of the OpenLDAP project, created by Russian developers for use in the telecommunications industry, with a lot of bug fixes and working replication in the multi-master topology. ReOpenLDAP is an open source implementation of the LDAP protocol server in C.

ReOpenLDAP provides a high level of performance:
')

• More than 50 thousand LDAP changes per second
• More than 100 thousand LDAP requests per second

It is worth noting that, as a legacy of the OpenLDAP project, the ReOpenLDAP project received 3,185 goto operators, which greatly complicate the analysis, but even so, some errors were found.

Please sign up for testing the Beta-version of PVS-Studio under Linux

The writing of this article was made possible by the start of work on the creation of PVS-Studio for Linux. It was in Linux that we checked the ReOpenLDAP project. However, there is a threat that the Linux version will end its existence even before its publication. The reason is little practical interest. When there is some discussion on the forum, it seems that the biggest problem of the PVS-Studio product is the lack of a version for Linux. But when it came to collecting contacts of people willing to participate in testing the Beta-version, it turned out that very few of them. Note: we wrote about searching for enthusiasts in the article " PVS-Studio confesses its love for Linux ".

I want to note: we are not worried about testing PVS-Studio. Some for some reason decided that we had conceived all this so that programmers could work for us as testers for free. Of course, this is not the case: we are able to organize testing on our own. However, if only a few people respond, then it may be worth it to slow down or even suspend work in this direction. And, unfortunately, there are really very few respondents. Therefore, our unicorn has a request for all Linux-programmers.

Please join the ranks of the PVS-Studio for Linux Beta Testers. This will show that there is a practical interest in the instrument. Once again I will write how you can join the ranks of enthusiasts.

If you want to help us test the work of PVS-Studio for Linux, please write to us. To make the letters easier to process, please indicate the line “PVS-Studio for Linux, Beta” in the subject line of the letter. Send letters to support@viva64.com . Please write letters from corporate mailboxes and briefly introduce yourself. We will be grateful to everyone who responds, but first of all we will take into account the wishes and recommendations of those people who could potentially become our clients over time.

Also I ask in the letter to give answers to the following questions:

• What operating system do you plan to run the analyzer under?
• What development environment are you using?
• What compiler is used to build the project?
• What build system are you using?

When there is a version that you can try, we will write letters to all responding. Thank you in advance.

Test results

Error in the priority of operations

PVS-Studio warning : V593 Consider reviewing the expression A = B == C 'kind. The expression is calculated as the following: 'A = (B == C)'. mdb_dump.c 150

static int dumpit(....) { .... while ((rc = mdb_cursor_get(...) == MDB_SUCCESS)) { .... } .... } 

The author made a mistake with the location of the closing parenthesis in the condition of the while loop , which led to an error in the priority of operations. As a result, a comparison is first performed, and then the result of this comparison is written to the variable rc .

This code should be corrected as follows:

 while ((rc = mdb_cursor_get(...)) == MDB_SUCCESS) { .... } 

Work with a null pointer

PVS-Studio warning : V595 The 'key' pointer was used before it was verified against nullptr. Check lines: 1324, 1327. mdb.c 1324

 char * mdb_dkey(MDB_val *key, char *buf) { .... unsigned char *c = key->mv_data; // <= .... if (!key) // <= return ""; .... } 

In the if block, the key pointer is checked for NULL , therefore, the author admits the possibility that this pointer is equal to zero, however, the above pointer is already used without verification. To avoid an error, you should check the value of the key pointer before using it.

Similar warning:

• V595 Check lines: 7282, 7291. mdb.c 7282

Suspicious Ternary Operator

PVS-Studio warning : V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: "vlvResult". common.c 2119

 static int print_vlv(....) { .... tool_write_ldif( ldif ? LDIF_PUT_COMMENT : LDIF_PUT_VALUE, ldif ? "vlvResult" : "vlvResult", buf, rc ); // <= } .... } 

When using the specified ternary operator, the same value will be returned, regardless of the condition. Judging by the similar places in the source, there is a typo, and this code should be rewritten like this:

 .... tool_write_ldif( ldif ? LDIF_PUT_COMMENT : LDIF_PUT_VALUE, ldif ? "vlvResult: " : "vlvResult", buf, rc ); .... 

Probable typo in the field name

PVS-Studio warning : V571 Recurring check. The 'if (s-> state.r == 0)' condition was already verified in line 147. rurwl.c 148

 void rurw_r_unlock(....) { .... if (s->state.r == 0) { // <= if (s->state.r == 0) // <= s->thr = 0; p->rurw_readers -= 1; } .... } 

Here, the same condition is checked twice. If you pay attention to similar places in the source, for example:

 void rurw_w_unlock(....) { .... if (s->state.w == 0) { if (s->state.r == 0) s->thr = 0; p->rurw_writer = 0; } .... } 

then we can assume that one of the conditions meant checking s-> state.w == 0. However, this is only an assumption, but in any case, developers should check this piece of code, and either correct one of the conditions or delete duplicate check.

Similar place:

• V571 Recurring check. The 'def-> mrd_usage & 0x0100U' condition was already verified in line 319. mr.c 322

Parameter rewriting

PVS-Studio warning : V763 Parameter 'rc' is always rewritten in function body before being used. tls_o.c 426

 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 the parameter is used. It is probably worth removing rc from the parameter list.

Invalid format specifier

PVS-Studio warning : V576 Incorrect format. Consider checking the fourth argument of the 'snprintf' function. The SIGNED argument of memsize type is expected. conn.c 309

 struct Connection { .... unsigned long c_connid; .... } .... static int conn_create(....) { .... bv.bv_len = snprintf( buf, sizeof( buf ), "cn=Connection %ld", // <= c->c_connid ); .... } 

The format specifier "% ld" does not match the c-> c_connid argument passed to snprintf . The correct qualifier for unsigned long is "% lu". Using "% ld" instead of "% lu" will result in invalid values, with sufficiently large arguments.

Similar warnings:

• V576 Incorrect format. Consider checking the fprintf function. The SIGNED integer type argument is expected. ure.c 1865
• V576 Incorrect format. Consider checking the fprintf function. The SIGNED argument of memsize type is expected. tools.c 211
• V576 Incorrect format. Consider checking the fourth argument of the fprintf function. The UNSIGNED integer type argument is expected. mdb.c 1253

Undivided pointer

PVS-Studio warning : V528 It is odd that the pointer to the 'char' type is compared with the '\ 0' value. Probably meant: * ludp-> lud_filter! = '\ 0'. backend.c 1525

 int fe_acl_group(....) { .... if ( ludp->lud_filter != NULL && ludp->lud_filter != '\0') // <= { .... } } 

The author of the code wanted to reveal a situation when the pointer is zero or the string is empty. But I forgot to dereference the ludp-> lud_filter pointer , so the pointer just double-checks for NULL equality.

You should dereference the pointer:

  .... if ( ludp->lud_filter != NULL && *ludp->lud_filter != '\0') .... 

Similar unclaimed pointers:

• V528 It is odd that the pointer to the 'char' type is compared with the '\ 0' value. Probably meant: * (* lsei) -> lsei_values ​​[0] == '\ 0'. syntax.c 240
• V528 It is odd that the pointer to the 'char' type is compared with the '\ 0' value. Probably meant: * (* lsei) -> lsei_values ​​[1]! = '\ 0'. syntax.c 241

Extra check

PVS-Studio warning : V560 A part of conditional expression is always true:! Saveit. syncprov.c 1510

 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 ) { .... } .... } 

There is no sense in the else branch to check saveit for equality to zero, since This has already been done in the first condition. This worsens readability. And, perhaps, this is even a mistake, as it was supposed to check something else.

But most likely the code is quite simple:

 if ( saveit || op->o_tag == LDAP_REQ_ADD ) { .... } else if ( op->o_tag == LDAP_REQ_MODRDN ) { .... } 

Dangerous use of realloc

PVS-Studio warning : 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

 int main( int argc, char *argv[]) { .... lud.lud_exts = (char **)realloc( lud.lud_exts, sizeof( char * ) * ( nexts + 2 ) ); .... } 

An expression like foo = realloc (foo, ....) is potentially dangerous. If memory allocation is impossible, realloc will return a null pointer, overwriting the previous pointer value. To prevent this, it is recommended to store the pointer value in an additional variable before using realloc .

Rewriting value

PVS-Studio warning : V519 The 'ca.argv' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 7774, 7776. bconfig.c 7776

 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, then the first superfluous assignment should be deleted.

Conclusion

ReOpenLDAP is a project designed to ensure stable operation under high load. Therefore, developers are responsible for testing and use specialized tools such as ThreadSanitizer and Valgrind . However, as we see, this is not always enough, as with the help of PVS-Studio a number of errors were identified, although there are few of them.

Static analysis helps to find errors before the testing phase - at the very early stages, thereby saving a lot of time. Therefore, analyzers should be used regularly, and not like us, occasionally, demonstrating the capabilities of our PVS-Studio tool.

I suggest to download and try the PVS-Studio static analyzer on your own project.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Egor Bredikhin. Checking the Code for LDAP Server ReOpenLDAP on Our Readers' Request .