Андрей Карпов

13 Окт 2011

Теги:

#Cpp

PVS-Studio vs Chromium - продолжение

13 Окт 2011

Автор: Андрей Карпов

Приблизительно полгода назад мы проверили проект Chromium и написали про это статью. Естественно, анализатор PVS-Studio постоянно развивается, и в новой версии Chromium мы нашли некоторые необнаруженные ранее ошибки. Конечно, многие ошибки относятся не к самому проекту Chromium, а к используемым в нём библиотекам. Однако в статье я хочу показать, что улучшились возможности анализатора, а не то, в какой части Chromium что-то нашлось, а в какой нет. Поэтому сообщения будут приведены вместе.

Итак, в этой заметке мы перечислим то новое, что нашел анализатор. Это далеко не всё, так как отчет мы просмотрели быстро и выписали только те фрагменты, на которых остановился взгляд. Для более подробного анализа Chromium или библиотек мы можем предоставить на время разработчикам полноценную версию PVS-Studio. Кстати, перейдите по этой ссылке и, возможно, Вы тоже заинтересуетесь возможностью полноценно попробовать PVS-Studio: http://www.viva64.com/ru/b/0092/

Фрагмент 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:

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

Имеющиеся в коде вызовы функции strncmp() сравнивают только первые символы, а не строки целиком. Для вычисления длины строк попытались использовать совершенно неуместный здесь оператор sizeof(). Оператор sizeof() вычислит размер указателя, а вовсе не количество байт в строке.

Фрагмент N2

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

Диагностическое сообщение PVS-Studio:

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

Вновь ошибка в том, что будут сравниваться только первые 4 или 8 байт, в зависимости от размера указателя.

Фрагмент 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:

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

Ошибка связана с приоритетом операций. Корректный вариант кода:

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

Фрагмент N4

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

Диагностическое сообщение PVS-Studio:

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

Ошибка связана с приоритетом операций. Корректный вариант кода:

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

Фрагмент N5

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

Диагностическое сообщение PVS-Studio:

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

Условие вида (A != 123 || A!= 321) всегда истинно. Очевидно, здесь есть опечатка, и условие должно было выглядеть по-другому.

Фрагмент 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:

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

Два раза выполняется одинаковая проверка. Видимо, должно быть:

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

Фрагмент 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:

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

Два раза подряд член 'plname' сравнивается со строкой "PCMU". Скорее всего, второй раз должна быть использована другая строка.

Фрагмент 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:

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

Первый раз условие написано так: "A < B". А второй раз так: "B > A". Таким образом, вторая проверка не имеет смысла. Очевидно, в коде имеется опечатка.

Фрагмент N9

WebRtc_Word32

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

Диагностическое сообщение PVS-Studio:

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

Оператор '?:' имеет более низкий приоритет, чем оператор '+'. В результате условие работает не так, как ожидал программист. Корректное условие должно выглядеть так:

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

Аналогичная ошибка есть здесь:

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

Фрагмент N10

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

Диагностическое сообщение PVS-Studio:

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

Как видно из первой строки, в переменной total считается некоторая сумма. Однако, потом допущена опечатка и вместо "+=", написано "=+".

Фрагмент N11

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

Диагностическое сообщение PVS-Studio:

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

Странный код.

Фрагмент N12

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

Диагностическое сообщение PVS-Studio:

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

Из-за ошибки, цикл в функции-тесте делает только одну итерацию. Корректный вариант цикла:

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

Фрагмент 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:

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

Видимо код писался методом Copy-Paste, и затем индексы были поправлены неправильно. Сравнение должно выглядеть так:

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())

Фрагмент 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:

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

В цикле происходит выход за границы массивов. Цикл должен перебирать только FRAMESAMPLES_HALF элементов.

Фрагмент N16

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

Диагностическое сообщение PVS-Studio:

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

Возможно, здесь забыт оператор 'break;'.

#Cpp