Alexander Chibisov

Sep 06 2016

Tags:

#Cpp

Rechecking Apache HTTP Server

Sep 06 2016

Author: Alexander Chibisov

Introduction
Incorrect check for an empty string
Incrementing a pointer instead of the value
Incorrect password clearing
Uninitialized variable
Suspicious expression
Incorrect check of HRESULT
Superfluous operation?
Redundant condition
Incorrect pragma directive
Conclusion

Apache HTTP Server project continues to develop, and so does PVS-Studio analyzer, growing even more powerful with every new version. Let's see what we've got this time.

Introduction

Apache HTTP Server is an open-source cross-platform project consisting of multiple modules. The HTTP Server kernel is written in C and developed completely by the Apache Software Foundation company. The other components were created by a number of third-party developers from the open-source community.

The project authors used Coverity to check the earlier versions of Apache HTTP Server. The recent check, however, hasn't revealed any signs of the code being analyzed by other tools. The project's code is of high quality, though PVS-Studio still managed to find a few interesting errors.

We already checked the project in 2011. For information about the bugs found during that check, see the article "Leo Tolstoy and static code analysis".

The recent analysis was done with PVS-Studio, version 6.08.

Incorrect check for an empty string

typedef struct {
  ....
  ap_regmatch_t *re_pmatch;
  apr_size_t re_nmatch;
  const char **re_source;
  ....
} ap_expr_eval_ctx_t;

static const char *ap_expr_eval_re_backref(
                     ap_expr_eval_ctx_t *ctx, ....)
{
  int len;

  if (!ctx->re_pmatch || 
      !ctx->re_source || 
      *ctx->re_source == '\0' ||    // <=
       ctx->re_nmatch < n + 1)
         return "";
....
}

Diagnostic message:

V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: ** ctx->re_source == '\0'. util_expr_eval.c 199

When handling pointers, programmers sometimes mix up pointers and values they point to. In the example above, the programmer forgot to dereference the pointer when checking the third subexpression in the condition. They wanted to check if the string was empty by comparing the first character of the string with the null terminator, but instead compared the pointer itself with the null character. After fixing this expression, we can see that another subexpression should be added to check if there is a pointer to the string.

The analyzer has already caught this error once, as indicated by an error description on our page with examples of errors found by the V528 diagnostic rule. Since the bug is still there, we should report it again. It can be fixed by changing the code in the following way:

if (!ctx->re_pmatch  || 
    !ctx->re_source  || 
    !*ctx->re_source || 
    **ctx->re_source == '\0' ||
    ctx->re_nmatch < n + 1)
        return "";

Incrementing a pointer instead of the value

apr_status_t iconv_uc_conv(...., apr_size_t *res)
{
  ....
  *res = (apr_size_t)(0);
  if (data == NULL) {
    *res = (apr_size_t) -1;
    return APR_EBADF;
  }
  ....
  if (size < 0) { 
     ....
     if (size)
       *res ++;                // <=
  }
  ....
}

Diagnostic message:

V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. iconv_uc.c 114

The pointer is dereferenced, but the returned value is not used. The code of the function, however, indicates that it is the value that the authors intended to work with, so the precedence of the dereferencing operation should be increased by adding a pair of parentheses: (*res) ++;.

Incorrect password clearing

int get_password(struct passwd_ctx *ctx)
{
  ....
  if (strcmp(ctx->passwd, buf) != 0) {
      ctx->errstr = "password verification error";
      memset(ctx->passwd, '\0', strlen(ctx->passwd));
      memset(buf, '\0', sizeof(buf));
      return ERR_PWMISMATCH;
  }
  ....
  memset(buf, '\0', sizeof(buf));              // <=
  return 0;
  ....
}

Diagnostic message:

V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. passwd_common.c 165

Any program, handling private, data must clear passwords and other critical data when they are no longer needed. In the fragment above, the programmer is trying to clear a buffer storing a password. The way they have chosen to do that seemed reliable, but the memset function can do its job properly only when the buffer is used in subsequent code after the cleanup. Otherwise, the compiler is allowed to delete the call to the memset function during the building process. As a result, the critical information that should have been deleted will remain in the memory. What will happen to this memory block and where that information will get is unknown. To clear the storage, use special functions such as RtlSecureZeroMemory() and memset_s().

Those were probably the most critical defects found in Apache HTTP Server project.

A few more errors found by this diagnostic:

V597 The compiler could delete the 'memset' function call, which is used to flush 'x' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. apr_md4.c 362
V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. apr_md5.c 436
V597 The compiler could delete the 'memset' function call, which is used to flush 'final' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. apr_md5.c 662

Uninitialized variable

static int warrsztoastr(...., const wchar_t * arrsz, int args)
{
  const apr_wchar_t *wch;
  apr_size_t totlen;
  apr_size_t newlen;
  apr_size_t wsize;
  char **env;
  char *pstrs;
  char *strs;
  int arg; 

  if (args < 0) {
    for (args = 1, wch = arrsz; wch[0] || wch[1]; ++wch)
      if (!*wch)
        ++args;
  }
  wsize = 1 + wch - arrsz; 

  newlen = totlen = wsize * 3 + 1;
  ....
  (void)apr_conv_ucs2_to_utf8(arrsz, &wsize, strs, &newlen);
  ....
  return args;
}

Diagnostic message:

V614 Potentially uninitialized pointer 'wch' used. start.c 58

The function prepares the information necessary for converting a string from Wide Unicode into UTF-8. If the value of the args variable is negative, the number of characters in the string is unknown and needs to be counted.

Then, the value of wsize is computed based on the address of the string's last character, stored in the wch variable, and the address of the string's first character, stored in arrsz. The wsize variable is used to create a buffer for the new string. The wch variable is initialized inside a loop that executes only if the value of args is negative. Otherwise, the variable won't be initialized, which will lead to undefined behavior as the buffer size will be computed incorrectly.

As for now, the function is used only once, with the value of args being -1. This would have let the error stay unnoticed for a long time until someone passed a positive value for args. I don't know what the authors wanted the function to do in such a situation. It is strange, to say the least, that this function receives as an argument the same value that it returns, while the presence of the conditional statement before it makes its execution absolutely pointless when args is a positive value.

Suspicious expression

static int is_quoted_pair(const char *s)
{
  int res = -1;
  int c;

  if (((s + 1) != NULL) && (*s == '\\')) {     // <=
    c = (int) *(s + 1);
    if (apr_isascii(c)) {
      res = 1;
    }
  }
  return (res);
}

Diagnostic message:

V694 The condition ((s + 1) != ((void *) 0)) is only false if there is pointer overflow which is undefined behaviour anyway. mod_mime.c 531

Quite a strange condition. The first expression can be false only when adding up a pointer with one results in an overflow. And a pointer overflow is undefined behavior, so this code is incorrect anyway.

Incorrect check of HRESULT

#define SHSTDAPI EXTERN_C DECLSPEC_IMPORT HRESULT STDAPICALLTYPE
SHSTDAPI SHGetMalloc(_Outptr_ IMalloc **ppMalloc);

LRESULT CALLBACK ConnectDlgProc(....)
{
  ....
  if (SHGetMalloc(&pMalloc)) {             // <=
   pMalloc->lpVtbl->Free(pMalloc, il);
   pMalloc->lpVtbl->Release(pMalloc);
  }
  ....
}

Diagnostic message:

V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'SHGetMalloc(& pMalloc)'. The SUCCEEDED or FAILED macro should be used instead. apachemonitor.c 915

SHGetMalloc is a system function that returns a result of type HRESULT. HRESULT is a 32-bit value logically divided into three fields. You can't use it as a value of bool type; instead, use the SUCCEEDED macro.

Superfluous operation?

static const char *process_resource_config_fnmatch(....)
{
  apr_status_t rv;
  ....
  rv = apr_dir_open(&dirp, path, ptemp);
  if (rv != APR_SUCCESS) {
    return apr_psprintf(p, 
               "Could not open config directory %s: %pm",
                path, &rv);
  }

  candidates = apr_array_make(ptemp, 1, sizeof(fnames));
  while (apr_dir_read(....) == APR_SUCCESS) {
     ....
     if (rest && (rv == APR_SUCCESS) &&              // <=
        (dirent.filetype != APR_DIR)) {     
          continue;
     }
     fnew = (fnames *) apr_array_push(candidates);
     fnew->fname = full_path;
  }
  ....
}

Diagnostic message:

V560 A part of conditional expression is always true: (rv == 0). config.c 2029

The analyzer found a redundant check inside the condition. It may seem just unnecessary code at first, but if you look closer, you'll see that the loop wouldn't start if the check of the rv variable were true. It's also not clear why the programmer uses the value resulting from the previous operations if it's not used elsewhere in the loop body.

The code logic suggests that the rv = apr_dir_open(...) function should be used before the condition: then the check of the rv variable would make sense. Perhaps I'm wrong and it's just a superfluous check, but I do advise the authors to examine this code and fix the error if there's one.

Two more errors of this kind:

V560 A part of conditional expression is always true: status == 0. mod_ident.c 217 (project mod_ident)
V560 A part of conditional expression is always true: j == 0. mod_ident.c 217 (project mod_ident)

Redundant condition

static int uldap_connection_init(....)
{
  ....
  if (ldc->ChaseReferrals==AP_LDAP_CHASEREFERRALS_ON){
    if ((ldc->ReferralHopLimit != AP_LDAP_HOPLIMIT_UNSET) && 
         ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) {
          ....
    }
  }
  ....
}

Diagnostic message:

V571 Recurring check. The 'ldc->ChaseReferrals == 1' condition was already verified in line 399. util_ldap.c 400

This example contains a redundant condition: there is no need to check the same expression both in the inner and outer conditional statements, as the inner statement can be executed only when the conditions of the outer one are true. The entire code within these statements requires that all the conditions in both if statements should be checked, so a better way would be to leave the outer statement out and amend the expression of the inner one to keep the checks in the same order.

if (ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON && 
   (ldc->ReferralHopLimit != AP_LDAP_HOPLIMIT_UNSET)) {
      ....
}

Incorrect pragma directive

#ifdef _MSC_VER
#pragma warning(disable: 4032)
#include <conio.h>
#pragma warning(default: 4032)
#else
#include <conio.h>
#endif

Diagnostic message:

V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 38, 40. apr_getpass.c 40

In the code above, the authors set a directive to its default value instead of the value it had before. This is a bad approach. Instead, save the previously used value using the #pragma warning(push) directive and then return it with the help of #pragma warning(pop):

#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4032)
#include <conio.h>
#pragma warning(pop)
#else
#include <conio.h>
#endif

Conclusion

The defects we have found in this application prove that even the most high-quality and well-tested projects are likely to contain bugs. Static analysis should be applied regularly; one-time checks are not enough. No matter how good you are at programming, typos and other defects are inevitable. PVS-Studio analyzer will allow you to catch errors and defects before they have a chance to slip into the release and cause trouble. We encourage you to download and try the analyzer on your own projects.

#Cpp