Egor Bredikhin

Aug 22 2016

Tags:

#Cpp

Checking the Code of LDAP-Server ReOpenLDAP on Our Readers' Request

• Introduction
• Please sign up for beta testing of PVS-Studio for Linux
• Analysis results
  • Operation-precedence bug
  • Using a null pointer
  • Suspicious ternary operator
  • Possible typo in a field name
  • Overwriting a parameter
  • Incorrect format specifier
  • Undereferenced pointer
  • Redundant check
  • Dangerous use of realloc
  • Rewriting a value
• Conclusion

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.

Introduction

ReOpenLDAP, also known as "TelcoLDAP", is a fork of OpenLDAP 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-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".

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.

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 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. mdb_dump.c 150

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:

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

Using a null pointer

PVS-Studio diagnostic message: V595 The 'key' pointer was utilized 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 "";
  ....
}

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 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 ); // <=
  }
  ....
}

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:

....
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 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;
  }
  ....
}

One condition is checked twice. Looking at similar fragments in the source files, for example:

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 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 it is used. Perhaps rc should be removed from the parameter list.

Incorrect format specifier

PVS-Studio diagnostic message: V576 Incorrect format. Consider checking the fourth actual 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 %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 It is odd that pointer to '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 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:

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

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:

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 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 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 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, 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. 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 and try PVS-Studio static analyzer with your own projects.

#Cpp

0

0

0

0

SHARE

Tags:

#Cpp

We can email you a selection of our best articles once a month