Our website uses cookies to enhance your browsing experience.
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.
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:
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.
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:
Once a runnable version is ready, we will e-mail everyone who has applied. Thank you all in advance!
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) {
....
}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:
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 );
....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:
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.
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:
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:
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 ) {
....
}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.
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.
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.
0
0
0
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
0