Andrey Karpov

Oct 13 2011

Tags:

#Cpp

PVS-Studio vs Chromium - Continuation

Oct 13 2011

Author: Andrey Karpov

About half a year ago we checked the Chromium project and wrote an article about it. The PVS-Studio analyzer naturally keeps developing, and in the new Chromium version we have found some errors undetected before. Many errors, of course, refer not to the Chromium project itself but to libraries it employs. But in this article I want to show you how the analyzer's capabilities have improved and not tell you about what we have found in this or that part of Chromium. That's why I will give messages together.

So, in this post we will enumerate the new defects found by the analyzer. This is far from all of the things found, we've only glanced through the report and included only what we found at first glance. If you want a more thorough analysis of Chromium or its libraries, we can provide developers with the full version of PVS-Studio for a time so that they perform it themselves. By the way, go by this link and perhaps you will get interested in the opportunity to try the full PVS-Studio version too: http://www.viva64.com/en/b/0092/

Fragment N1

std::string TestFileIO::TestParallelReads() {
  ...
  const char* expected_result_1 =
    "__border__abc__border__";
  const char* expected_result_2 =
    "__border__defghijkl__border__";
  if (strncmp(extended_buf_1, expected_result_1,
              sizeof(expected_result_1)) != 0 ||
      strncmp(extended_buf_2, expected_result_2,
              sizeof(expected_result_2)) != 0) {
  ...
}

PVS-Studio's diagnostic messages:

V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. ppapi_tests test_file_io.cc 759

V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. ppapi_tests test_file_io.cc 761

Calls of the strncmp() function in this code compare only the first characters and not the whole strings. To calcluate strings' lengths the programmer tried to use the sizeof() operator absolutely inappropriate for this purpose. The sizeof() operator will calculate the pointer's size instead of the number of bytes in a string.

Fragment N2

int  AffixMgr::parse_convtable(..., const char * keyword)
{
  ...
  if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
  ...
}

PVS-Studio's diagnostic message:

V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. hunspell affixmgr.cxx 3545

Again, the mistake here is that only the first 4 or 8 bytes will be compared depending on the pointer's size.

Fragment N3

#define SEC_ASN1_CHOICE        0x100000

typedef struct sec_ASN1Template_struct {
  unsigned long kind; 
  ...
} SEC_ASN1Template;

PRBool SEC_ASN1IsTemplateSimple(
  const SEC_ASN1Template *theTemplate)
{
  ...
  if (!theTemplate->kind & SEC_ASN1_CHOICE) {
  ...
}

PVS-Studio's diagnostic message:

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. nss secasn1u.c 121

The error is caused by the issue of operation priorities. The correct code is this one:

if (!(theTemplate->kind & SEC_ASN1_CHOICE)) {

Fragment N4

bool GetPlatformFileInfo(...) {
  ...
  info->is_directory =
    file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
  ...
}

PVS-Studio's diagnostic message:

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 219

The error is caused by the issue of operation priorities. The correct code is this one:

info->is_directory =
  (file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0;

Fragment N5

WebRtc_Word32
interpolator::SupportedVideoType(VideoType srcVideoType,
                                 VideoType dstVideoType)
{
  ...
  if ((srcVideoType != kI420) ||
      (srcVideoType != kIYUV) ||
      (srcVideoType != kYV12))
  {
      return -1;
  }
  ...
}

PVS-Studio's diagnostic message:

V547 Expression is always true. Probably the '&&' operator should be used here. webrtc_vplib interpolator.cc 119

The (A != 123 || A!= 321) - like condition is always true. There is obviously a misprint here and the condition must look in a different way.

Fragment N6

static GLenum
get_temp_image_type(GLcontext *ctx, GLenum baseFormat)
{
  ...
  if (ctx->DrawBuffer->Visual.redBits <= 8)
     return GL_UNSIGNED_BYTE;
  else if (ctx->DrawBuffer->Visual.redBits <= 8)
     return GL_UNSIGNED_SHORT;
  else
     return GL_FLOAT;
  ...
}

PVS-Studio's diagnostic message:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2520, 2522. osmesa meta.c 2520

One and the same check is performed twice. Perhaps it should look this way:

if (ctx->DrawBuffer->Visual.redBits <= 8)
   return GL_UNSIGNED_BYTE;
else if (ctx->DrawBuffer->Visual.redBits <= 16)
   return GL_UNSIGNED_SHORT;

Fragment N7

WebRtc_Word32 ModuleFileUtility::UpdateWavHeader(OutStream& wav)
{
  ...
  if(STR_CASE_CMP(codec_info_.plname, "L16") == 0)
  {
     res = WriteWavHeader(wav, codec_info_.plfreq, 2,
             channels, kWaveFormatPcm, _bytesWritten);
  } else if(STR_CASE_CMP(codec_info_.plname, "PCMU") == 0) {
     res = WriteWavHeader(wav, 8000, 1, channels,
             kWaveFormatMuLaw, _bytesWritten);
  } else if(STR_CASE_CMP(codec_info_.plname, "PCMU") == 0) {
     res = WriteWavHeader(wav, 8000, 1, channels,
             kWaveFormatALaw, _bytesWritten);
  } else {
  ...
}

PVS-Studio's diagnostic message:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1324, 1327. media_file media_file_utility.cc 1324

The 'plname' variable is compared to the "PCMU" string twice in a row. Most likely, another string must be used for the second time.

Fragment N8

enum ContentSettingsType;
struct EntryMapKey {
  ContentSettingsType content_type;
  ...
};

bool OriginIdentifierValueMap::EntryMapKey::operator<(
    const OriginIdentifierValueMap::EntryMapKey& other) const {
  if (content_type < other.content_type)
    return true;
  else if (other.content_type > content_type)
    return false;
  return (resource_identifier < other.resource_identifier);
}

PVS-Studio's diagnostic message:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 61, 63. browser content_settings_origin_identifier_value_map.cc 61

The condition looks this way for the first time: "A < B"; and this way for the second time: "B > A". Thus, the check is meaningles. There is obviously a misrpint in the code.

Fragment N9

WebRtc_Word32

RTPReceiverVideo::ReceiveH263Codec(...)
{
  ...
  if (IP_PACKET_SIZE < parsedPacket.info.H263.dataLength +
       parsedPacket.info.H263.insert2byteStartCode? 2:0)
  ...
}

PVS-Studio's diagnostic message:

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. rtp_rtcp rtp_receiver_video.cc 480

The '?:' operator has a lower priority than the '+' operator. As a result, the condition works in a way other than the programmer expected. The correct condition must look this way:

if (IP_PACKET_SIZE < parsedPacket.info.H263.dataLength +
    (parsedPacket.info.H263.insert2byteStartCode ? 2:0))

The same error is here:

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. rtp_rtcp rtp_receiver_video.cc 504

Fragment N10

static int 
xmlXPathCompOpEvalFirst(...)
{
  ...
  total += xmlXPathCompOpEvalFirst(...);
  ...
  total =+ xmlXPathCompOpEvalFilterFirst(ctxt, op, first);
  ...
}

PVS-Studio's diagnostic message:

V588 The expression of the 'A =+ B' kind is utilized. Consider reviewing it, as it is possible that 'A += B' was meant. libxml xpath.c 12676

As you may see from the first line, a certain sum is being calculated in the total variable. But then there is a misprint and we have "=+" instead of "+=".

Fragment N11

static VisiblePosition updateAXLineStartForVisiblePosition(...)
{
  ...
  tempPosition = startPosition.previous();
  if (tempPosition.isNull() || tempPosition.isNull())
      break;
  ...
}

PVS-Studio's diagnostic message:

V501 There are identical sub-expressions 'tempPosition.isNull ()' to the left and to the right of the '||' operator. webcore_remaining accessibilityobject.cpp 489

Strange code.

Fragment N12

TEST(SharedMemoryTest, MultipleThreads) {
  ...
  int threadcounts[] = { 1, kNumThreads };
  for (size_t i = 0;
       i < sizeof(threadcounts) / sizeof(threadcounts); i++) {
  ...
}

PVS-Studio's diagnostic message:

V501 There are identical sub-expressions 'sizeof (threadcounts)' to the left and to the right of the '/' operator. base_unittests shared_memory_unittest.cc 231

Because of the error, the loop in the test function performs only one iteration. This is the correct loop:

for (size_t i = 0;
     i < sizeof(threadcounts) / sizeof(*threadcounts); i++) {

Fragment N13

bool
ir_algebraic_visitor::reassociate_constant(...)
{
  if (ir1->operands[0]->type->is_matrix() ||
      ir1->operands[0]->type->is_matrix() ||
      ir2->operands[1]->type->is_matrix() ||
      ir2->operands[1]->type->is_matrix())
    return false;
}

PVS-Studio's diagnostic message:

V501 There are identical sub-expressions 'ir1->operands [0]->type->is_matrix ()' to the left and to the right of the '||' operator. mesa ir_algebraic.cpp 189

The code seems to have been written through the Copy-Paste method, while the programmer fixed the indexes incorrectly. This is how the comparison must look:

if (ir1->operands[0]->type->is_matrix() ||
    ir1->operands[1]->type->is_matrix() ||
    ir2->operands[0]->type->is_matrix() ||
    ir2->operands[1]->type->is_matrix())

Fragment N15

#define FRAMESAMPLES_HALF      240
#define FRAMESAMPLES           480

typedef struct {
  ...
  WebRtc_Word16 realFFT[FRAMESAMPLES_HALF];
  WebRtc_Word16 imagFFT[FRAMESAMPLES_HALF];
} ISACUBSaveEncDataStruct;

int WebRtcIsac_EncodeStoredDataUb12(...)
{
  ...
  for(n = 0; n < FRAMESAMPLES; n++)
  {
    realFFT[n] = (WebRtc_Word16)
      (scale * (float)ISACSavedEnc_obj->realFFT[n] + 0.5f);
    imagFFT[n] = (WebRtc_Word16)
      (scale * (float)ISACSavedEnc_obj->imagFFT[n] + 0.5f);
  }
  ...
}

PVS-Studio's diagnostic messages:

V557 Array overrun is possible. The value of 'n' index could reach 479. iSAC encode.c 1307

V557 Array overrun is possible. The value of 'n' index could reach 479. iSAC encode.c 1308

Array overruns occur in the loop. The loop must search through FRAMESAMPLES_HALF items only.

Fragment N16

static int
coff_helper_gasflags(...)
{
  ...
  case 'd':
      datasect = 1;
      load = 1;
      readonly = 0;
  case 'x':
      code = 1;
      load = 1;
      break;
  ...
}

PVS-Studio's diagnostic message:

V519 The 'load' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1346, 1350. yasm coff-objfmt.c 1350

The 'break;' operator seems to be missing here.

#Cpp