Bug 51542 - Apache HTTP Server vs PVS-Studio
Summary: Apache HTTP Server vs PVS-Studio
Status: NEW
Alias: None
Product: APR
Classification: Unclassified
Component: APR-iconv (show other bugs)
Version: HEAD
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache Portable Runtime bugs mailinglist
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-22 12:21 UTC by Andrey Karpov
Modified: 2018-05-28 20:10 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Karpov 2011-07-22 12:21:28 UTC
I've tested the our PVS-Studio analyzer with the projects Apache. While analyzing this project I've found some mistakes and decided to mention it. Hope it will come in handy.

See also this article: http://www.viva64.com/en/b/0105/

Bugs:

V501 There are identical sub-expressions 'state == rsl_encoding' to the left and to the right of the '||' operator. mod_mime_magic mod_mime_magic.c 787

static int magic_rsl_to_request(request_rec *r)
{
  ...
  if (state == rsl_subtype || state == rsl_encoding ||
      state == rsl_encoding) {
  ...
}

Need:
if (state == rsl_subtype || state == rsl_separator ||
    state == rsl_encoding) {

---------------------------------------------------------------
V512 A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 560

#define MEMSET_BZERO(p,l)       memset((p), 0, (l))

void apr__SHA256_Final(sha2_byte digest[], SHA256_CTX* context) {
  ...
  MEMSET_BZERO(context, sizeof(context));
  ...
}

And here:
False 1 902 V512 A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 581 False
False 1 916 V512 A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 892 False
False 1 917 V512 A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 912 False
False 1 918 V512 A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 967 False
False 1 919 V512 A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 987 False
---------------------------------------------------------------
V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *tag->arg = '\0'. mod_headers mod_headers.c 330

typedef struct {
    const char* (*func)(request_rec *r,char *arg);
    char *arg;
} format_tag;

static char *parse_format_tag(apr_pool_t *p, format_tag *tag, const char **sa)
{
  ...
  tag->arg = '\0';
  ...
}

Need:
tag->arg = NULL;
Or:
tag->arg[0] = '\0';

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

apr_status_t
iconv_uc_conv(..., apr_size_t *res)
{
  ...
  if (size)
    *res ++;
  ...
}

Need: (*res)++;
---------------------------------------------------------------
V547 Expression 'len < 0' is always false. Unsigned type value is never < 0. aprutil apr_memcache.c 814

typedef  size_t      apr_size_t;

APU_DECLARE(apr_status_t) apr_memcache_getp(...)
{
  ...
  apr_size_t len = 0;
  ...
  len = atoi(length);
  ...
  if (len < 0) {
    *new_length = 0;
    *baton = NULL;
  }
  else {
    ...  
  }
}
---------------------------------------------------------------
V547 Expression 'csd < 0' is always false. Unsigned type value is never < 0. libhttpd child.c 404

typedef UINT_PTR SOCKET;

static unsigned int __stdcall win9x_accept(void * dummy)
{
  SOCKET csd;
  ...
  do {
      clen = sizeof(sa_client);
      csd = accept(nsd, (struct sockaddr *) &sa_client, &clen);
  } while (csd < 0 && APR_STATUS_IS_EINTR(apr_get_netos_error()));
  ...
}
---------------------------------------------------------------
V560 A part of conditional expression is always true: 0x04. pcre pcre.c 3366

static BOOL
compile_branch(...)
{
  ...
  else if ((digitab[ptr[1]] && ctype_digit) != 0)
  ...
}

Need:
else if ((digitab[ptr[1]] & ctype_digit) != 0)
---------------------------------------------------------------
V568 It's odd that the argument of sizeof() operator is the 'sizeof (SECURITY_ATTRIBUTES)' expression. libhttpd util_win32.c 115

PSECURITY_ATTRIBUTES GetNullACL(void)
{
  PSECURITY_ATTRIBUTES sa;

  sa  = (PSECURITY_ATTRIBUTES) LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));
  sa->nLength = sizeof(sizeof(SECURITY_ATTRIBUTES));
  ...
}

Need: 
sa->nLength = sizeof(SECURITY_ATTRIBUTES);
---------------------------------------------------------------
V579 The apr_snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. libhttpd util_pcre.c 85

AP_DECLARE(apr_size_t) ap_regerror(int errcode, const ap_regex_t *preg,
                                   char *errbuf, apr_size_t errbuf_size)
{
  ...
  apr_snprintf(errbuf, sizeof errbuf,
               "%s%s%-6d", message, addmessage, (int)preg->re_erroffset);
Comment 1 Jeff Trawick 2011-07-22 14:13:23 UTC
Very cool/thanks for the report!
(child.c 404 is already fixed, but the others aren't familiar)
Comment 2 Stefan Fritsch 2011-09-19 18:05:38 UTC
- util_pcre.c 85 already fixed in r1095448
- other HTTPD issues fixed in trunk in r1172732
- the APR parts still need to be checked
Comment 3 Stefan Fritsch 2011-09-19 21:25:51 UTC
- fixed apr sha2.c issue in 1172825, r1172828, r1172829
- apr memcache issue already fixed in trunk in r982408, r982409
- Checked that the pcre issue is fixed in 8.12

Can somebody look at iconv_uc.c?
Comment 4 Christophe JAILLET 2018-05-28 20:10:33 UTC
Looks that all is solved now, except the iconv potential issue.

Re-affecting to APR for it.

> V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant:
> '(*pointer)++'. apriconv iconv_uc.c 114
> 
> apr_status_t
> iconv_uc_conv(..., apr_size_t *res)
> {
>   ...
>   if (size)
>     *res ++;
>   ...
> }
> 
> Need: (*res)++;