Соавтор: Роман Фомичёв.

В настоящее время многие проекты открывают свой исходный код и разрешают делать изменения в нем сообществу заинтересованных разработчиков. Мы проверим один из таких проектов — OpenJDK, и поможем разработчикам улучшить их код.

Введение

OpenJDK (Open Java Development Kit) — проект по созданию реализации платформы Java (Java SE), состоящий исключительно из свободного и открытого исходного кода. Проект стартовал в 2006 году усилиями компании Sun. В проекте используются несколько языков — C, C++ и Java. Нас интересуют исходные коды написанные на С и С++. Для проверки возьмем 9-ю версию OpenJDK. Код этой реализации Java платформы доступен в репозитории Mercurial.

Проект был проверен с помощью статического анализатора кода PVS-Studio. В нем реализовано множество диагностических правил, позволяющих найти большое количество ошибок, допущенных при программировании, в том числе и таких, которые трудно обнаружимы при простом просмотре кода. Некоторые из этих ошибок не влияют на логику работы программы, а некоторые могут привести к печальным последствиям при исполнении программы. На сайте анализатора есть примеры ошибок, найденных в других проектах. Для анализа доступны проекты, использующие языки C, C++ и С#. Загрузить пробную версию анализатора можно по ссылке.

Ошибки в логических выражениях

Сначала рассмотрим ошибки в логических выражениях:

int StubAssembler::call_RT(....) {
#ifdef _LP64
  // if there is any conflict use the stack
  if (arg1 == c_rarg2 || arg1 == c_rarg3 ||
      arg2 == c_rarg1 || arg1 == c_rarg3 ||
      arg3 == c_rarg1 || arg1 == c_rarg2) {
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'arg1 == c_rarg3' to the left and to the right of the '||' operator. c1_Runtime1_x86.cpp 174

Анализатор сообщает о дублировании проверки arg1 == c_rarg3. В этом фрагменте или избыточная проверка или, что гораздо хуже, присутствует логическая ошибка. Возможно, вместо продублированного условия должно было проверяться что-то другое. Разработчикам есть смысл взглянуть повнимательнее на этот код.

В этом же условии есть еще одно повторяющееся выражение arg1 == c_rarg2:

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'arg1 == c_rarg2' to the left and to the right of the '||' operator. c1_Runtime1_x86.cpp 174

Эти предупреждения особенно хорошо показывают необходимость использования статического анализатора. В большом количестве однотипных выражений очень легко допустить ошибку, которую трудно обнаружить при поверхностном просмотре кода.

В следующем фрагменте встретилась «неидеальная» проверка в условии метода Ideal:

Node *AddLNode::Ideal(PhaseGVN *phase, bool can_reshape) {
  ....
  if( op2 == Op_AddL &&
      in2->in(1) == in1 &&
      op1 != Op_ConL &&
      0 ) {
  ....
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always false: 0. addnode.cpp 435

Довольно странно в логическом выражении использовать 0. Скорее всего, этот фрагмент кода ещё в разработке и с целью отладки условие сделали невыполнимым. Соответствующие комментарии в коде отсутствуют, и в этом случае высока вероятность забыть исправить код в дальнейшем. Результатом этой ошибки будет то, что все, что находится внутри этого условия, никогда не будет исполняться, так как результат вычисления логического выражения будет всегда false.

Приоритеты операций

Довольно часто программисты надеются на свое знание приоритетов и не выделяют составные части сложного выражения скобками:

int method_size() const
  { return sizeof(Method)/wordSize + is_native() ? 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. method.hpp 249

В данном случае я не знаю специфики кода, но есть подозрение, что в функции планировали выбирать значение смещения '2' или '0' в зависимости от результата вызова функции is_native(), но выражение имеет иной порядок вычисления. Сначала выполнится сложение sizeof(Method)/wordSize + is_native(), а потом уже будет возвращен результат 2 или 0. Скорее всего, код должен выглядеть так:

{ return sizeof(Method)/wordSize + (is_native() ? 2 : 0); }

Это очень распространённая ошибка с приоритетом операций. В базе найденных анализатором ошибок были выявлены самые популярные из них и приведены в статье: Логические выражения в C/C++. Как ошибаются профессионалы.

Copy-Paste

Следующая характерная группа ошибок связана с копированием кода. От этого излюбленного приема программистов никуда не денешься, поэтому исследуем места, где был применен copy paste:

static int
setImageHints(....)
{
  ....
  if (dstCMP->isDefaultCompatCM) {
      hintP->allocDefaultDst = FALSE;
      hintP->cvtToDst = FALSE;
  }
  else if (dstCMP->isDefaultCompatCM) {
      hintP->allocDefaultDst = FALSE;
      hintP->cvtToDst = FALSE;
  }
  ....
}

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1873, 1877. awt_ImagingLib.c 1873

В этом примере полностью совпадают как условия в if и else if, так и тот код, который должен исполняться. Второе условие полностью бессмысленно, оно никогда не будет исполняться.

Еще один похожий случай:

static int expandPackedBCR(JNIEnv *env, RasterS_t *rasterP, 
                           int component,
                           unsigned char *outDataP)
{
  ....
  /* Convert the all bands */
  if (rasterP->numBands < 4) {
      /* Need to put in alpha */
      for (y=0; y < rasterP->height; y++) {
          inP = lineInP;
          for (x=0; x < rasterP->width; x++) {
              for (c=0; c < rasterP->numBands; c++) {
                  *outP++ = (unsigned char)
                      (((*inP&rasterP->sppsm.maskArray[c]) >> roff[c])
                       <<loff[c]);
              }
              inP++;
          }
          lineInP += rasterP->scanlineStride;
      }
  }
  else {
      for (y=0; y < rasterP->height; y++) {
          inP = lineInP;
          for (x=0; x < rasterP->width; x++) {
              for (c=0; c < rasterP->numBands; c++) {
                  *outP++ = (unsigned char)
                      (((*inP&rasterP->sppsm.maskArray[c]) >> roff[c])
                       <<loff[c]);
              }
              inP++;
          }
          lineInP += rasterP->scanlineStride;
      }
  }
  ....
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. awt_ImagingLib.c 2927

Исполняемый код в обоих блоках идентичен, соответственно нет никакой разницы, что рассчитывается в условии. Есть смысл взглянуть на это место и убрать ненужную ветку или, если подразумевалась разная логика, скорректировать код, чтобы исключить дублирование.

Есть еще два места с идентичным дублированием. Просто укажу их, без приведения кода:

• V523 The 'then' statement is equivalent to the 'else' statement. awt_ImagingLib.c 3111
• V523 The 'then' statement is equivalent to the 'else' statement. awt_ImagingLib.c 3307

Ну и последний интересный пример, связанный с возможной ошибкой copy paste:

Node* GraphKit::record_profiled_receiver_for_speculation(Node* n)
{
  ....
  ciKlass* exact_kls = profile_has_unique_klass();
  bool maybe_null = true;
  if (java_bc() == Bytecodes::_checkcast ||
      java_bc() == Bytecodes::_instanceof ||
      java_bc() == Bytecodes::_aastore) {
    ciProfileData* data = 
      method()->method_data()->bci_to_data(bci());
    bool maybe_null = data == NULL ? true :    <==
                      data->as_BitData()->null_seen();
  }
  return record_profile_for_speculation(n, 
    exact_kls, maybe_null);
  return n;
}

Предупреждение PVS-Studio: V561 It's probably better to assign value to 'maybe_null' variable than to declare it anew. Previous declaration: graphKit.cpp, line 2170. graphKit.cpp 2175

Что же происходит в этом коде? Перед блоком if объявлена переменная bool maybe_null = true;. Затем, когда исполняется код в блоке if, объявляется переменная с аналогичным именем. После выхода из блока значение этой переменной будет утеряно, и вызов функции, использующей эту переменную, произойдет в любом случае со значением true. Хорошо, если эта переменная была продублирована с целью отладки. В противном случае данный код исполняется некорректно и требует модификации:

maybe_null = data == NULL ? true :    
             data->as_BitData()->null_seen();

Работа с указателями

Работа с указателями требует внимательности и аккуратности, так как при неправильном использовании указателей могут появиться ошибки, которые будет сложно идентифицировать. Как правило, основную опасность представляет использование битых указателей или использование указателей без проверки на нулевое значение.

Первым делом рассмотрим случай явного использования нулевого указателя:

static jint JNICALL
cbObjectTagInstance(....)
{
    ClassInstancesData  *data;

    /* Check data structure */
    data = (ClassInstancesData*)user_data;
    if (data == NULL) {
        data->error = AGENT_ERROR_ILLEGAL_ARGUMENT;
        return JVMTI_VISIT_ABORT;
    }
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'data' might take place. util.c 2424

Совершенно непонятный код с использованием нулевого указателя, может привести к аварийному завершению программы. Возможно, эта ветка никогда не срабатывала, благодаря чему удалось избежать проблем при работе программы. В этом же файле нашлось еще три подобных места:

• V522 Dereferencing of the null pointer 'data' might take place. util.c 2543
• V522 Dereferencing of the null pointer 'data' might take place. util.c 2601
• V522 Dereferencing of the null pointer 'data' might take place. util.c 2760

А вот в следующих случаях возможность использования нулевого указателя скрыта глубже. Это очень распространенная ситуация, подобные предупреждения встречаются практически во всех проверяемых проектах:

static jboolean
visibleClasses(PacketInputStream *in, PacketOutputStream *out)
{
  ....
  else {
    (void)outStream_writeInt(out, count);
    for (i = 0; i < count; i++) {
      jbyte tag;
      jclass clazz;

      clazz = classes[i];                     <==
      tag = referenceTypeTag(clazz);

      (void)outStream_writeByte(out, tag);
      (void)outStream_writeObjectRef(env, out, clazz);
    }
  }

  if ( classes != NULL )                      <==
    jvmtiDeallocate(classes);
  ....
    return JNI_TRUE;
}

Предупреждение PVS-Studio: V595 The 'classes' pointer was utilized before it was verified against nullptr. Check lines: 58, 66. ClassLoaderReferenceImpl.c 58

В нижнем блоке указатель проверяется на нулевое значение, значит, программист допускает возможность того, что значение указателя будет нулевым. Но в блоке выше указатель используется без проверки. Соответственно, если значение указателя будет нулевым, то такая проверка нам не поможет, и мы получим аварийное завершение программы. Чтобы исправить эту ошибку, надо проверить указатель выше обоих блоков.

Приведу еще один аналогичный пример:

int InstructForm::needs_base_oop_edge(FormDict &globals) const {
  if( is_simple_chain_rule(globals) ) {
    const char *src = _matrule->_rChild->_opType;
    OperandForm *src_op = globals[src]->is_operand();
    assert( src_op, "Not operand class of chain rule" );
    return src_op->_matrule ? 
           src_op->_matrule->needs_base_oop_edge() : 0;
  }                             // Else check instruction

  return _matrule ? _matrule->needs_base_oop_edge() : 0;
}

Предупреждение PVS-Studio: V595 The '_matrule' pointer was utilized before it was verified against nullptr. Check lines: 3534, 3540. formssel.cpp 3534

Здесь проверка указателя осуществляется снизу в тернарном операторе — _matrule? _matrule->needs_base_oop_edge(): 0;. А выше происходит простое обращение к этому указателю — const char *src = _matrule->_rChild->_opType;. Рецепт исправления аналогичен: нужно проверить указатель до его использования. Таких мест нашлось довольно много, я приведу их списком:

• V595 The '_pipeline' pointer was utilized before it was verified against nullptr. Check lines: 3265, 3274. output_c.cpp 3265
• V595 The 'index_bound' pointer was utilized before it was verified against nullptr. Check lines: 790, 806. c1_RangeCheckElimination.cpp 790
• V595 The 'g_type_init' pointer was utilized before it was verified against nullptr. Check lines: 94, 108. GioFileTypeDetector.c 94
• V595 The 'classArray' pointer was utilized before it was verified against nullptr. Check lines: 1169, 1185. JPLISAgent.c 1169
• V595 The 'q' pointer was utilized before it was verified against nullptr. Check lines: 594, 599. mpi.c 594
• V595 The 'info.waiters' pointer was utilized before it was verified against nullptr. Check lines: 224, 228. ObjectReferenceImpl.c 224
• V595 The 'methods' pointer was utilized before it was verified against nullptr. Check lines: 225, 229. ReferenceTypeImpl.c 225
• V595 The 'fields' pointer was utilized before it was verified against nullptr. Check lines: 433, 437. ReferenceTypeImpl.c 433
• V595 The 'nested' pointer was utilized before it was verified against nullptr. Check lines: 538, 540. ReferenceTypeImpl.c 538
• V595 The 'interfaces' pointer was utilized before it was verified against nullptr. Check lines: 593, 595. ReferenceTypeImpl.c 593
• V595 The 'buf' pointer was utilized before it was verified against nullptr. Check lines: 265, 266. ps_proc.c 265
• V595 The 'monitors' pointer was utilized before it was verified against nullptr. Check lines: 382, 387. ThreadReferenceImpl.c 382
• V595 The 'monitors' pointer was utilized before it was verified against nullptr. Check lines: 557, 560. ThreadReferenceImpl.c 557
• V595 The 'signature' pointer was utilized before it was verified against nullptr. Check lines: 520, 526. debugInit.c 520
• V595 The 'BlackPoint' pointer was utilized before it was verified against nullptr. Check lines: 192, 208. cmssamp.c 192
• V595 The 'nativename' pointer was utilized before it was verified against nullptr. Check lines: 506, 511. awt_Font.c 506
• V595 The 'pseq->seq' pointer was utilized before it was verified against nullptr. Check lines: 788, 791. cmsnamed.c 788
• V595 The 'GammaTables' pointer was utilized before it was verified against nullptr. Check lines: 1430, 1434. cmsopt.c 1430

Иногда программисты наоборот проверяют указатели, но делают это неправильно:

FileBuff::FileBuff( BufferedFile *fptr, ArchDesc& archDesc) : 
                   _fp(fptr), _AD(archDesc) {
  ....
  _bigbuf = new char[_bufferSize];
  if( !_bigbuf ) {
    file_error(SEMERR, 0, "Buffer allocation failed\n");
    exit(1);
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the '_bigbuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. filebuff.cpp 47

В данном случае проверка указателя _bigbuf на нулевое значение после использования new бессмысленна. В случае если системе не удастся выделить память, то будет сгенерировано исключение и выполнение функции прекратится. Для исправления ошибки можно использовать несколько подходов. Cделать выделение памяти в блоке try catch, или использовать для выделения памяти конструкцию new(std::nothrow), которая не будет генерировать исключение в случае неудачи. Есть еще несколько таких неправильных проверок:

• V668 There is no sense in testing the 'vspace' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. psParallelCompact.cpp 455
• V668 There is no sense in testing the 'uPtr' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. jni.cpp 113

Последняя ошибка в работе с указателями допущена при явном приведении указателя одного типа к указателю другого типа:

mlib_status mlib_convMxNext_f32(...)
{
  mlib_d64 dspace[1024], *dsa = dspace;
  ....
  mlib_f32 *fsa;
  ....

  if (3 * wid_e + m > 1024) {
    dsa = mlib_malloc((3 * wid_e + m) * sizeof(mlib_d64));

    if (dsa == NULL)
      return MLIB_FAILURE;
  }

  fsa = (mlib_f32 *) dsa; <==
  ....
}

Предупреждение PVS-Studio: V615 An odd explicit conversion from 'double *' type to 'float *' type. mlib_ImageConvMxN_Fp.c 294

Указателю на float mlib_f32 *fsa пытаются присвоить указатель на double mlib_d64 dspace[1024], *dsa = dspace. Типы float и double имеют различный размер и подобное приведение типов, скорее всего, ошибочно. Несоответствие размеров приводимых типов приведет к тому, что указатель fsa будет указывать на некорректный для типа float формат числа.

В другом файле есть еще два таких аналогичных преобразования, надо хорошенько проверить этот код и использовать правильные преобразования типов.

• V615 An odd explicit conversion from 'double *' type to 'float *' type. mlib_ImageLookUp_Bit.c 525
• V615 An odd explicit conversion from 'double *' type to 'float *' type. mlib_ImageLookUp_Bit.c 526

На этом закончим рассмотрение ошибок, связанных с использованием указателей и исследуем оставшиеся предупреждения анализатора.

Разные ошибки

Следующая ошибка, наверное, тоже результат неудачного копирования кода:

static bool
parse_bool (const char **pp, const char *end, unsigned int *pv)
{
  ....

  /* CSS allows on/off as aliases 1/0. */
  if (*pp - p == 2 || 0 == strncmp (p, "on", 2))
    *pv = 1;
  else if (*pp - p == 3 || 0 == strncmp (p, "off", 2))
    *pv = 0;
  else
    return false;

  return true;
}

Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. hb-shape.cc 104

Здесь как раз тот случай, когда ошибка не влияет на работоспособность программы. Вместо сравнения трех символов, сравниваются только первые два, я не исключаю, что автор кода может и специально сделал такую проверку. Так как значение в буфере p может быть on или off, то достаточно сравнивать и первые два символа. Но для порядка, все-таки можно скорректировать код:

else if (*pp - p == 3 || 0 == strncmp (p, "off", 3))

Нашлось несколько мест, где возможны ошибки при реализации классов:

class ProductionState {
  ....
private:
    // Disable public use of constructor, copy-ctor,  ...
  ProductionState( )                         :
  _production(cmpstr, hashstr, Form::arena) 
  {  assert( false, "NotImplemented");  };
  ProductionState( const ProductionState & ) :
  _production(cmpstr, hashstr, Form::arena) 
  {  assert( false, "NotImplemented");  }; // Deep-copy
};

Предупреждение PVS-Studio: V690 Copy constructor is declared as private in the 'ProductionState' class, but the default '=' operator will still be generated by compiler. It is dangerous to use such a class. dfa.cpp 76

В этом классе попытались запретить копирование, но забыли добавить в приватную область оператор копирования. Он будет сгенерирован по умолчанию и будет доступен для использования. Даже если сейчас в коде нигде не используется этот оператор, то нет гарантии, что в дальнейшем кто-то случайно его не вызовет. При вызове такого оператора произойдет почленное копирование для класса, который не должен копироваться. Это может привести к различным эффектам вплоть до аварийного завершения программы. В данном случае следует добавить в приватную область объявление оператора "=".

Есть еще два класса, где есть подобные проблемы, желательно скорректировать их таким образом, чтобы не нарушать "Закон Большой Двойки".

• V690 The 'MemRegion' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. memRegion.hpp 43
• V690 Copy constructor is declared as private in the 'Label' class, but the default '=' operator will still be generated by compiler. It is dangerous to use such a class. assembler.hpp 73

Последняя ошибка похожа на простую опечатку:

bool os::start_debugging(char *buf, int buflen) {
  int len = (int)strlen(buf);
  char *p = &buf[len];
  ....
  if (yes) {
    // yes, user asked VM to launch debugger
    jio_snprintf(buf, sizeof(buf), "gdb /proc/%d/exe %d",
      os::current_process_id(), os::current_process_id());

    os::fork_and_exec(buf);
    yes = false;
  }
  return yes;
}

Предупреждение PVS-Studio: V579 The jio_snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. os_linux.cpp 6094

Программист хотел передать длину буфера, но не учел, что он не локально объявленный массив, а указатель приходящий в аргументе функции. В результате вычисления выражения sizeof(buf) мы получим не длину буфера, а размер указателя, который будет равен 4 или 8 байтам. Исправить ошибку легко, так выше длина буфера уже была получена: int len = (int)strlen(buf);. Правильный вариант будет выглядеть следующим образом:

jio_snprintf(buf, len ....

Заключение

Всегда интересно проверять проект, который использует множество людей и следит за его качеством. Нашлось достаточно большое количество ошибок, в этой статье описано только их некоторое количество, остальные требуют более глубокого исследования. Найденные ошибки еще раз подтверждают эффективность использования статического анализатора, так как он позволяет обнаружить ошибки, которые трудно обнаружить при просмотре простым глазом. Эффективнее всего использовать анализатор на постоянной основе, так как он сэкономит много времени, которое было бы потрачено на отладку программы при поиске багов. Напоминаю, что вы можете попробовать работу статического анализатора PVS-Studio на своем проекте, загрузив его пробную версию.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. OpenJDK check by PVS-Studio.