- PVSM.RU - https://www.pvsm.ru -

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

Рисунок 1

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

О PVS-Studio

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

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

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

Об Apache Hive

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

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

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

Об анализе

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

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

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

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

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

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

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

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

V6007 [14] 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 [14] 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 [14] 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 [14]:

  • 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 [15] 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 [15] 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 [16] 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 [16] 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 [17] 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 [18] 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 [19] 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 проектов ошибки такого рода встречаются довольно часто. Есть даже целая статья [20] про это.

V6020 [21] 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 [22] 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 [23] 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 [24] 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). Код писался многими разработчиками долгие годы и, как следствие, статическому анализатору есть что находить. Это еще раз подтверждает то, что статический анализ крайне важен и полезен при разработке средних и больших проектов!

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

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

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

Автор: stefanbuzz

Источник [27]


Сайт-источник PVSM.RU: https://www.pvsm.ru

Путь до страницы источника: https://www.pvsm.ru/java/327097

Ссылки в тексте:

[1] PVS-Studio: https://www.viva64.com/ru/pvs-studio-download/

[2] вариантов: https://www.viva64.com/ru/b/0614/

[3] Apache Hadoop: https://hadoop.apache.org/

[4] HDFS: https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-hdfs/HdfsDesign.html

[5] Hadoop MapReduce: https://hadoop.apache.org/docs/stable/hadoop-mapreduce-client/hadoop-mapreduce-client-core/MapReduceTutorial.html

[6] Hadoop YARN: https://hadoop.apache.org/docs/stable/hadoop-yarn/hadoop-yarn-site/YARN.html

[7] Apache Hive: https://hive.apache.org/

[8] здесь: https://cwiki.apache.org/confluence/display/Hive

[9] GitHub: https://github.com/apache/hive

[10] инструкцией: https://www.viva64.com/ru/m/0044/

[11] пример: https://www.viva64.com/ru/b/0523/

[12] V6022: https://www.viva64.com/ru/w/v6022/

[13] V6021: https://www.viva64.com/ru/w/v6021/

[14] V6007: https://www.viva64.com/ru/w/v6007/

[15] V6008: https://www.viva64.com/ru/w/v6008/

[16] V6034: https://www.viva64.com/ru/w/v6034/

[17] V6046: https://www.viva64.com/ru/w/v6046/

[18] V6051: https://www.viva64.com/ru/w/v6051/

[19] V6009: https://www.viva64.com/ru/w/v6009/

[20] статья: https://www.viva64.com/ru/b/0509/

[21] V6020: https://www.viva64.com/ru/w/v6020/

[22] V6030: https://www.viva64.com/ru/w/v6030/

[23] V6042: https://www.viva64.com/ru/w/v6042/

[24] V6060: https://www.viva64.com/ru/w/v6060/

[25] здесь: https://www.viva64.com/ru/b/0594/

[26] здесь: https://www.viva64.com/ru/b/0639/

[27] Источник: https://habr.com/ru/post/463759/?utm_source=habrahabr&utm_medium=rss&utm_campaign=463759