PVS-Studio в гостях у Apache Hive

• О PVS-Studio
• Об Apache Hive
• Об анализе
• Предопределенность условий
• NPE
• Не уследили за сдвигом
• Увлеклись логированием
• Украли исключение
• ... прочее
• Заключение

Последние десять лет движение open source является одним из ключевых факторов развития IT-отрасли и важной ее составной частью. Роль и место open source не только усиливается в виде роста количественных показателей, но происходит и изменение его качественного позиционирования на IT-рынке в целом. Не сидя сложа руки, бравая команда PVS-Studio активно способствует закреплению позиций open source проектов, находя затаившиеся баги в огромных толщах кодовых баз и предлагая для таких проектов бесплатные лицензии. Эта статья не исключение! Сегодня речь пойдет об Apache Hive! Отчет получен - есть на что посмотреть!

О PVS-Studio

Статический анализатор кода PVS-Studio существует более 10 лет на IT рынке и представляет собой многофункциональное и легко внедряемое программное решение. На данный момент анализатор поддерживает языки C, C++, C#, Java и работает на платформах Windows, Linux и macOS.

PVS-Studio является платным B2B-решением и используется большим количеством команд в различных компаниях. Если хотите посмотреть, на что способен анализатор, то скачать дистрибутив и запросить триальный ключ можно здесь.

Если Вы гик open source или, например, являетесь студентом, то вы можете воспользоваться одним из бесплатных вариантов лицензирования PVS-Studio.

Об Apache Hive

Объем данных в последние годы растет с большой скоростью. Стандартные базы данных больше не могут поддерживать работоспособность при таком темпе роста объёма информации, что послужило появлению термина Big Data и всего, что с ним связано (обработка, хранение и все вытекающие действия с такими объемами данных).

На данный момент Apache Hadoop считается одной из основополагающих технологий Big Data. Главные задачи этой технологии - хранение, обработка и управление большими объемами данных. Основными составляющими фреймворка являются Hadoop Common, HDFS, Hadoop MapReduce, Hadoop YARN. Со временем вокруг Hadoop образовалась целая экосистема из связанных проектов и технологий, многие из которых развивались изначально в рамках проекта, а впоследствии стали самостоятельными. Одним из таких проектов и является Apache Hive.

Apache Hive представляет собой распределенное хранилище данных. Оно управляет данными, которые хранятся в HDFS, и предоставляет язык запросов на базе SQL(HiveQL) для работы с этими данными. Для подробного ознакомления с этим проектом можно изучить информацию здесь.

Об анализе

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

• Получил Apache Hive с GitHub;
• Воспользовался инструкцией по запуску Java-анализатора и запустил анализ;
• Получил отчет анализатора, проанализировал его и выделил интересные случаи.

Результаты анализа: 1456 предупреждений уровня достоверности High и Medium (602 и 854, соответственно) были выданы для 6500+ файлов.

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

Среди предупреждений не рассматривались 407 предупреждений (177 High и 230 Medium), приходящихся на файлы с тестами. Не рассматривалось диагностическое правило V6022 (сложно отделить ошибочные ситуации от корректных в незнакомом коде), у которого было аж 482 предупреждения. V6021 c 179 предупреждениями тоже не рассматривалось.

В конечном итоге все равно осталось достаточное количество предупреждений. А поскольку я не настраивал анализатор, то среди них опять-таки есть ложные срабатывания. Нет смысла описывать большое количество предупреждений в статье :). Рассмотрим то, что мне попалось на глаза и показалось интересным.

Предопределенность условий

Диагностическое правило V6007 - рекордсмен среди всех оставшихся предупреждений анализатора. Чуть более 200 предупреждений!!! Какие-то, вроде, безобидные, некоторые - подозрительные, а другие и вовсе реальные ошибки! Давайте разберем некоторые из них.

V6007 Expression 'key.startsWith("hplsql.")' is always true. Exec.java(675)

void initOptions() 
{
  ....
  if (key == null || value == null || !key.startsWith("hplsql.")) { // <=
    continue;
  }
  else if (key.compareToIgnoreCase(Conf.CONN_DEFAULT) == 0) {
    ....
  }
  else if (key.startsWith("hplsql.conn.init.")) {
    ....
  }
  else if (key.startsWith(Conf.CONN_CONVERT)) {
    ....
  }
  else if (key.startsWith("hplsql.conn.")) {
   ....
  }
  else if (key.startsWith("hplsql.")) {                            // <=
   ....
  }    
}

Довольно продолжительная конструкция if-else-if! Анализатор ругается на последний if (key.startsWith("hplsql.")), указывая на его истинность в случае достижения программой этого фрагмента кода. Ведь правда, если глянуть в самое начало конструкции if-else-if, то проверка уже была осуществлена. И в случае, если наша строка не начиналась с подстроки "hplsql.", то выполнение кода сразу же перескакивало на следующую итерацию.

V6007 Expression 'columnNameProperty.length() == 0' is always false. OrcRecordUpdater.java(238)

private static 
TypeDescription getTypeDescriptionFromTableProperties(....) 
{
  ....
  if (tableProperties != null) {
    final String columnNameProperty = ....;
    final String columnTypeProperty = ....;
    
    if (   !Strings.isNullOrEmpty(columnNameProperty)
        && !Strings.isNullOrEmpty(columnTypeProperty)) 
    {
      List<String> columnNames = columnNameProperty.length() == 0 
                                 ? new ArrayList<String>() 
                                 : ....;
                                 
      List<TypeInfo> columnTypes = columnTypeProperty.length() == 0 
                                   ? new ArrayList<TypeInfo>() 
                                   : ....;
      ....
      }
    }
  }
  ....
}

Сравнение длины строки columnNameProperty с нулем всегда нам будет возвращать false. Это происходит из-за того, что наше сравнение находится под проверкой !Strings.isNullOrEmpty(columnNameProperty). Если состояние программы дойдет до нашего рассматриваемого условия, то строка columnNameProperty гарантированно будет ненулевой и не пустой.

Это актуально и для строки columnTypeProperty. Предупреждение строкой ниже:

• V6007 Expression 'columnTypeProperty.length() == 0' is always false. OrcRecordUpdater.java(239)

V6007 Expression 'colOrScalar1.equals("Column")' is always false. GenVectorCode.java(3469)

private void 
generateDateTimeArithmeticIntervalYearMonth(String[] tdesc) throws Exception
{
  ....
  String colOrScalar1 = tdesc[4];
  ....
  String colOrScalar2 = tdesc[6];
  ....
  if (colOrScalar1.equals("Col") && colOrScalar1.equals("Column"))    // <=
  {
    ....
  } else if (colOrScalar1.equals("Col") && colOrScalar1.equals("Scalar")) 
  {
    ....
  } else if (colOrScalar1.equals("Scalar") && colOrScalar1.equals("Column"))    
  {
    ....
  }
}

Здесь банальный copy-paste. Получилось так, что строка colOrScalar1 должна одновременно равняться разным значениям, а это невозможно. По всей видимости слева должна проверяться переменная colOrScalar1, а справа colOrScalar2.

Еще схожие предупреждения строками ниже:

• V6007 Expression 'colOrScalar1.equals("Scalar")' is always false. GenVectorCode.java(3475)
• V6007 Expression 'colOrScalar1.equals("Column")' is always false. GenVectorCode.java(3486)

Как итог, никакие действия в конструкции if-else-if никогда выполняться не будут.

Некоторые другие предупреждения V6007:

• V6007 Expression 'characters == null' is always false. RandomTypeUtil.java(43)
• V6007 Expression 'writeIdHwm > 0' is always false. TxnHandler.java(1603)
• V6007 Expression 'fields.equals("*")' is always true. Server.java(983)
• V6007 Expression 'currentGroups != null' is always true. GenericUDFCurrentGroups.java(90)
• V6007 Expression 'this.wh == null' is always false. New returns not-null reference. StorageBasedAuthorizationProvider.java(93), StorageBasedAuthorizationProvider.java(92)
• и так далее...

NPE

V6008 Potential null dereference of 'dagLock'. QueryTracker.java(557), QueryTracker.java(553)

private void handleFragmentCompleteExternalQuery(QueryInfo queryInfo)
{
  if (queryInfo.isExternalQuery()) 
  {
    ReadWriteLock dagLock = getDagLock(queryInfo.getQueryIdentifier());
    if (dagLock == null) {
      LOG.warn("Ignoring fragment completion for unknown query: {}",
          queryInfo.getQueryIdentifier());
    }
    boolean locked = dagLock.writeLock().tryLock();
    .....
  }
}

Отловили нулевой объект, залогировали и ... продолжили работать. Это приводит к тому, что следом за проверкой объекта происходит разыменование нулевого объекта. Печалька!

Скорее всего в случае нулевой ссылки следовало сразу выйти из функции или сгенерировать какое-то специальное исключение.

V6008 Null dereference of 'buffer' in function 'unlockSingleBuffer'. MetadataCache.java(410), MetadataCache.java(465)

private boolean lockBuffer(LlapBufferOrBuffers buffers, ....) 
{
  LlapAllocatorBuffer buffer = buffers.getSingleLlapBuffer();
  if (buffer != null) {                              // <=
    return lockOneBuffer(buffer, doNotifyPolicy);
  }
  LlapAllocatorBuffer[] bufferArray = buffers.getMultipleLlapBuffers();
  for (int i = 0; i < bufferArray.length; ++i) {
    if (lockOneBuffer(bufferArray[i], doNotifyPolicy)) continue;
    for (int j = 0; j < i; ++j) {
      unlockSingleBuffer(buffer, true);               // <=
    }
    ....
  }
  ....
}
....
private void unlockSingleBuffer(LlapAllocatorBuffer buffer, ....) {
  boolean isLastDecref = (buffer.decRef() == 0);      // <=
  if (isLastDecref) {
    ....
  }
}

И снова потенциальный NPE. Если выполнение программы достигнет метода unlockSingleBuffer, то объект buffer будет нулевым. Допустим, это произошло! Заглянем в метод unlockSingleBuffer и сразу на первой строчке видим, что происходит разыменование нашего объекта. Вот мы и попались!

Не уследили за сдвигом

V6034 Shift by the value of 'bitShiftsInWord - 1' could be inconsistent with the size of type: 'bitShiftsInWord - 1' = [-1 .. 30]. UnsignedInt128.java(1791)

private void shiftRightDestructive(int wordShifts,
                                   int bitShiftsInWord,
                                   boolean roundUp) 
{
  if (wordShifts == 0 && bitShiftsInWord == 0) {
    return;
  }

  assert (wordShifts >= 0);
  assert (bitShiftsInWord >= 0);
  assert (bitShiftsInWord < 32);
  if (wordShifts >= 4) {
    zeroClear();
    return;
  }

  final int shiftRestore = 32 - bitShiftsInWord;

  // check this because "123 << 32" will be 123.
  final boolean noRestore = bitShiftsInWord == 0;
  final int roundCarryNoRestoreMask = 1 << 31;
  final int roundCarryMask = (1 << (bitShiftsInWord - 1));  // <=
  ....
}

Возможное смещение на -1. В случае, если на вход рассматриваемого метода придут, например, wordShifts == 3 и bitShiftsInWord == 0, то в указанной строке произойдет 1 << -1. Запланировано ли это?

V6034 Shift by the value of 'j' could be inconsistent with the size of type: 'j' = [0 .. 63]. IoTrace.java(272)

public void logSargResult(int stripeIx, boolean[] rgsToRead)
{
  ....
  for (int i = 0, valOffset = 0; i < elements; ++i, valOffset += 64) {
    long val = 0;
    for (int j = 0; j < 64; ++j) {
      int ix = valOffset + j;
      if (rgsToRead.length == ix) break;
      if (!rgsToRead[ix]) continue;
      val = val | (1 << j);                // <=
    }
    ....
  }
  ....
}

В указанной строке переменная j может принимать значение в диапазоне [0 .. 63]. Из-за этого вычисление значения val в цикле может происходить не так, как задумывал разработчик. В выражении (1 << j) единица имеет тип int, и, смещая ее от 32 и более, мы выходим за рамки допустимого. Для исправления ситуации необходимо написать ((long)1 << j).

Увлеклись логированием

V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1, 2. StatsSources.java(89)

private static 
ImmutableList<PersistedRuntimeStats> extractStatsFromPlanMapper (....) {
  ....
  if (stat.size() > 1 || sig.size() > 1)
  {
    StringBuffer sb = new StringBuffer();
    sb.append(String.format(
      "expected(stat-sig) 1-1, got {}-{} ;",    // <=
      stat.size(),
      sig.size()
    ));
    ....
  }
  ....
  if (e.getAll(OperatorStats.IncorrectRuntimeStatsMarker.class).size() > 0)
  {
    LOG.debug(
      "Ignoring {}, marked with OperatorStats.IncorrectRuntimeStatsMarker",
      sig.get(0)
    );
    continue;
  }
  ....
}

При форматировании строки через String.format() разработчик спутал синтаксис. Итог: в результирующую строку переданные параметры так и не попали. Могу предположить, что в предыдущей задаче разработчик работал над логированием, откуда и позаимствовал синтаксис.

Украли исключение

V6051 The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions. ObjectStore.java(9080)

private List<MPartitionColumnStatistics> 
getMPartitionColumnStatistics(....)
throws NoSuchObjectException, MetaException 
{
  boolean committed = false;

  try {
    .... /*some actions*/
    
    committed = commitTransaction();
    
    return result;
  } 
  catch (Exception ex) 
  {
    LOG.error("Error retrieving statistics via jdo", ex);
    if (ex instanceof MetaException) {
      throw (MetaException) ex;
    }
    throw new MetaException(ex.getMessage());
  } 
  finally 
  {
    if (!committed) {
      rollbackTransaction();
      return Lists.newArrayList();
    }
  }
}

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

В блоке try происходит формирование запроса и обращение к хранилищу. Переменная committed по умолчанию имеет значение false и меняет свое состояние только после всех успешно выполненных действий в try блоке. Это означает, что в случае возникновения исключения наша переменная будет всегда false. Catch блок словил исключение, немного скорректировал и пробросил далее. И когда приходит время блока finally, выполнение заходит в условие, из которого возвращаем наружу пустой список. Чего стоит нам это возвращение? А стоит это того, что все пойманные исключения никогда не будут выброшены наружу и обработаны соответствующим способом. Все те исключения, что указаны в сигнатуре метода, никогда не будут выброшены и просто сбивают с толку.

Подобное предупреждение:

• V6051 The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions. ObjectStore.java(808)

... прочее

V6009 Function 'compareTo' receives an odd argument. An object 'o2.getWorkerIdentity()' is used as an argument to its own method. LlapFixedRegistryImpl.java(244)

@Override
public List<LlapServiceInstance> getAllInstancesOrdered(....) {
  ....
  Collections.sort(list, new Comparator<LlapServiceInstance>() {
    @Override
    public int compare(LlapServiceInstance o1, LlapServiceInstance o2) {
      return o2.getWorkerIdentity().compareTo(o2.getWorkerIdentity()); // <=
    }
  });
  ....
}

Copy-paste, невнимательность, спешка и много других причин сделать эту глупую ошибку. При проверках open source проектов ошибки такого рода встречаются довольно часто. Есть даже целая статья про это.

V6020 Divide by zero. The range of the 'divisor' denominator values includes zero. SqlMathUtil.java(265)

public static long divideUnsignedLong(long dividend, long divisor) {
  if (divisor < 0L) {
    /*some comments*/
    return (compareUnsignedLong(dividend, divisor)) < 0 ? 0L : 1L;
  }

  if (dividend >= 0) { // Both inputs non-negative
    return dividend / divisor;                     // <=
  } else {
    ....
  }
}

Здесь достаточно все просто. Ряд проверок не предостерегли от деления на 0.

Еще предупреждения:

• V6020 Mod by zero. The range of the 'divisor' denominator values includes zero. SqlMathUtil.java(309)
• V6020 Divide by zero. The range of the 'divisor' denominator values includes zero. SqlMathUtil.java(276)
• V6020 Divide by zero. The range of the 'divisor' denominator values includes zero. SqlMathUtil.java(312)

V6030 The method located to the right of the '|' operator will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. OperatorUtils.java(573)

public static Operator<? extends OperatorDesc> findSourceRS(....) 
{
  ....
  List<Operator<? extends OperatorDesc>> parents = ....;
  if (parents == null | parents.isEmpty()) {
    // reached end e.g. TS operator
    return null;
  }
  ....
}

Вместо логического оператора || написали битовый оператор |. Это означает, что правая часть будет выполнена независимо от результата левой части. Такая опечатка, в случае parents == null, сразу же приведет к NPE в следующем логическом подвыражении.

V6042 The expression is checked for compatibility with type 'A' but is cast to type 'B'. VectorColumnAssignFactory.java(347)

public static 
VectorColumnAssign buildObjectAssign(VectorizedRowBatch outputBatch,
                                     int outColIndex,
                                     PrimitiveCategory category)
                                     throws HiveException 
{
  VectorColumnAssign outVCA = null;
  ColumnVector destCol = outputBatch.cols[outColIndex];
  if (destCol == null) {
    ....
  }
  else if (destCol instanceof LongColumnVector)
  {
    switch(category) {
    ....
    case LONG:
      outVCA = new VectorLongColumnAssign() {
                   ....
                   } .init(.... , (LongColumnVector) destCol);
      break;
    case TIMESTAMP:
      outVCA = new VectorTimestampColumnAssign() {
                   ....
                   }.init(...., (TimestampColumnVector) destCol);       // <=
      break;
    case DATE:
      outVCA = new VectorLongColumnAssign() {
                   ....
                   } .init(...., (LongColumnVector) destCol);
      break;
    case INTERVAL_YEAR_MONTH:
      outVCA = new VectorLongColumnAssign() {
                    ....
                   }.init(...., (LongColumnVector) destCol);
      break;
    case INTERVAL_DAY_TIME:
      outVCA = new VectorIntervalDayTimeColumnAssign() {
                    ....
                    }.init(...., (IntervalDayTimeColumnVector) destCol);// <=
    break;
    default:
      throw new HiveException(....);
    }
  }
  else if (destCol instanceof DoubleColumnVector) {
    ....
  }
  ....
  else {
    throw new HiveException(....);
  }
  return outVCA;
}

Рассматриваемые классы LongColumnVector extends ColumnVector и TimestampColumnVector extends ColumnVector. Проверка нашего объекта destCol на принадлежность LongColumnVector явно нам указывает, что внутри условного оператора будет именно объект этого класса. Несмотря на это, у нас происходит каст к TimestampColumnVector! Как видно, эти классы разные, не считая их общего родителя. Как итог - ClassCastException.

Все то же самое можно сказать и про приведение типа к IntervalDayTimeColumnVector:

• V6042 The expression is checked for compatibility with type 'A' but is cast to type 'B'. VectorColumnAssignFactory.java(390)

V6060 The 'var' reference was utilized before it was verified against null. Var.java(402), Var.java(395)

@Override
public boolean equals(Object obj) 
{
  if (getClass() != obj.getClass()) {  // <=
    return false;
  }    
  Var var = (Var)obj;  
  if (this == var)
  {
    return true;
  }
  else if (var == null ||               // <=
           var.value == null ||
           this.value == null)
  {
    return false;
  }
  ....
}

Странное сравнение объекта var с null после того, как произошло разыменование. В данном контексте var и obj - один и тот же объект (var = (Var)obj). Проверка на null подразумевает, что может прийти нулевой объект. И в случае вызова equals(null) мы получим сразу же на первой строке NPE вместо ожидаемого false. Увы, проверка есть, но не там.

Подобные подозрительные моменты с использованием объекта перед тем, как происходит проверка:

• V6060 The 'value' reference was utilized before it was verified against null. ParquetRecordReaderWrapper.java(168), ParquetRecordReaderWrapper.java(166)
• V6060 The 'defaultConstraintCols' reference was utilized before it was verified against null. HiveMetaStore.java(2539), HiveMetaStore.java(2530)
• V6060 The 'projIndxLst' reference was utilized before it was verified against null. RelOptHiveTable.java(683), RelOptHiveTable.java(682)
• V6060 The 'oldp' reference was utilized before it was verified against null. ObjectStore.java(4343), ObjectStore.java(4339)
• и так далее...

Заключение

Кто хоть чуть-чуть интересовался Big Data, тот вряд ли пропустил мимо своих ушей значимость Apache Hive. Проект популярный и достаточно большой, и в своем составе имеет более 6500 файлов исходного кода (*.java). Код писался многими разработчиками долгие годы и, как следствие, статическому анализатору есть что находить. Это еще раз подтверждает то, что статический анализ крайне важен и полезен при разработке средних и больших проектов!

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

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

Невозможно представить Apache Hive без Apache Hadoop, поэтому вполне вероятно, что единорог от PVS-Studio заглянет и туда. Но на сегодня это все, а пока скачивайте анализатор и проверяйте собственные проекты.