# PVS\-Studio в гостях у Apache Hive Последние десять лет движение open source является одним из ключевых факторов развития IT\-отрасли и важной ее составной частью\. Роль и место open source не только усиливается в виде роста количественных показателей, но происходит и изменение его качественного позиционирования на IT\-рынке в целом\. Не сидя сложа руки, бравая команда PVS\-Studio активно способствует закреплению позиций open source проектов, находя затаившиеся баги в огромных толщах кодовых баз и предлагая для таких проектов бесплатные лицензии\. Эта статья не исключение\! Сегодня речь пойдет об Apache Hive\! Отчет получен \- есть на что посмотреть\! ![0657_ApacheHive_ru/image1.png](https://import.viva64.com/docx/blog/0657_ApacheHive_ru/image1.png) ## О PVS\-Studio Статический анализатор кода [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/) существует более 10 лет на IT рынке и представляет собой многофункциональное и легко внедряемое программное решение\. На данный момент анализатор поддерживает языки C, C\+\+, C\#, Java и работает на платформах Windows, Linux и macOS\. PVS\-Studio является платным B2B\-решением и используется большим количеством команд в различных компаниях\. Если хотите посмотреть, на что способен анализатор, то скачать дистрибутив и запросить триальный ключ можно [здесь](https://pvs-studio.ru/ru/pvs-studio/download/)\. Если Вы гик open source или, например, являетесь студентом, то вы можете воспользоваться одним из бесплатных [вариантов](https://pvs-studio.ru/ru/blog/posts/0614/) лицензирования PVS\-Studio\. ## Об Apache Hive Объем данных в последние годы растет с большой скоростью\. Стандартные базы данных больше не могут поддерживать работоспособность при таком темпе роста объёма информации, что послужило появлению термина Big Data и всего, что с ним связано \(обработка, хранение и все вытекающие действия с такими объемами данных\)\. На данный момент [Apache Hadoop](https://hadoop.apache.org/) считается одной из основополагающих технологий Big Data\. Главные задачи этой технологии \- хранение, обработка и управление большими объемами данных\. Основными составляющими фреймворка являются Hadoop Common, [HDFS](https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-hdfs/HdfsDesign.html), [Hadoop MapReduce](https://hadoop.apache.org/docs/stable/hadoop-mapreduce-client/hadoop-mapreduce-client-core/MapReduceTutorial.html), [Hadoop YARN](https://hadoop.apache.org/docs/stable/hadoop-yarn/hadoop-yarn-site/YARN.html)\. Со временем вокруг Hadoop образовалась целая экосистема из связанных проектов и технологий, многие из которых развивались изначально в рамках проекта, а впоследствии стали самостоятельными\. Одним из таких проектов и является [Apache Hive](https://hive.apache.org/)\. Apache Hive представляет собой распределенное хранилище данных\. Оно управляет данными, которые хранятся в HDFS, и предоставляет язык запросов на базе SQL\(HiveQL\) для работы с этими данными\. Для подробного ознакомления с этим проектом можно изучить информацию [здесь](https://cwiki.apache.org/confluence/display/Hive)\. ## Об анализе Последовательность действий для анализа достаточно проста и не потребовала много времени: * Получил Apache Hive с [GitHub](https://github.com/apache/hive); * Воспользовался [инструкцией](https://pvs-studio.ru/ru/docs/manual/6703/) по запуску Java\-анализатора и запустил анализ; * Получил отчет анализатора, проанализировал его и выделил интересные случаи\. Результаты анализа: 1456 предупреждений уровня достоверности High и Medium \(602 и 854, соответственно\) были выданы для 6500\+ файлов\. Не все предупреждения являются ошибками\. Это нормальная ситуация и перед регулярным использованием анализатора требуется его настройка\. После чего можно ожидать достаточно низкий процент ложных срабатываний \([пример](https://pvs-studio.ru/ru/blog/posts/cpp/0523/)\)\. Среди предупреждений не рассматривались 407 предупреждений \(177 High и 230 Medium\), приходящихся на файлы с тестами\. Не рассматривалось диагностическое правило [V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/) \(сложно отделить ошибочные ситуации от корректных в незнакомом коде\), у которого было аж 482 предупреждения\. [V6021](https://pvs-studio.ru/ru/docs/warnings/v6021/) c 179 предупреждениями тоже не рассматривалось\. В конечном итоге все равно осталось достаточное количество предупреждений\. А поскольку я не настраивал анализатор, то среди них опять\-таки есть ложные срабатывания\. Нет смысла описывать большое количество предупреждений в статье :\)\. Рассмотрим то, что мне попалось на глаза и показалось интересным\. ## Предопределенность условий Диагностическое правило [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) \- рекордсмен среди всех оставшихся предупреждений анализатора\. Чуть более 200 предупреждений\!\!\! Какие\-то, вроде, безобидные, некоторые \- подозрительные, а другие и вовсе реальные ошибки\! Давайте разберем некоторые из них\. [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'key\.startsWith\("hplsql\."\)' is always true\. Exec\.java\(675\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'columnNameProperty\.length\(\) \=\= 0' is always false\. OrcRecordUpdater\.java\(238\) ```cpp private static TypeDescription getTypeDescriptionFromTableProperties(....) { .... if (tableProperties != null) { final String columnNameProperty = ....; final String columnTypeProperty = ....; if ( !Strings.isNullOrEmpty(columnNameProperty) && !Strings.isNullOrEmpty(columnTypeProperty)) { List columnNames = columnNameProperty.length() == 0 ? new ArrayList() : ....; List columnTypes = columnTypeProperty.length() == 0 ? new ArrayList() : ....; .... } } } .... } ``` Сравнение длины строки _columnNameProperty_ с нулем всегда нам будет возвращать _false_\. Это происходит из\-за того, что наше сравнение находится под проверкой _\!Strings\.isNullOrEmpty\(columnNameProperty\)_\. Если состояние программы дойдет до нашего рассматриваемого условия, то строка _columnNameProperty_ гарантированно будет ненулевой и не пустой\. Это актуально и для строки _columnTypeProperty_\. Предупреждение строкой ниже: * V6007 Expression 'columnTypeProperty\.length\(\) \=\= 0' is always false\. OrcRecordUpdater\.java\(239\) [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'colOrScalar1\.equals\("Column"\)' is always false\. GenVectorCode\.java\(3469\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'dagLock'\. QueryTracker\.java\(557\), QueryTracker\.java\(553\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6008/) Null dereference of 'buffer' in function 'unlockSingleBuffer'\. MetadataCache\.java\(410\), MetadataCache\.java\(465\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6034/) Shift by the value of 'bitShiftsInWord \- 1' could be inconsistent with the size of type: 'bitShiftsInWord \- 1' \= \[\-1 \.\. 30\]\. UnsignedInt128\.java\(1791\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6034/) Shift by the value of 'j' could be inconsistent with the size of type: 'j' \= \[0 \.\. 63\]\. IoTrace\.java\(272\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Arguments not used: 1, 2\. StatsSources\.java\(89\) ```cpp private static ImmutableList 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](https://pvs-studio.ru/ru/docs/warnings/v6051/) The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions\. ObjectStore\.java\(9080\) ```cpp private List 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](https://pvs-studio.ru/ru/docs/warnings/v6009/) Function 'compareTo' receives an odd argument\. An object 'o2\.getWorkerIdentity\(\)' is used as an argument to its own method\. LlapFixedRegistryImpl\.java\(244\) ```cpp @Override public List getAllInstancesOrdered(....) { .... Collections.sort(list, new Comparator() { @Override public int compare(LlapServiceInstance o1, LlapServiceInstance o2) { return o2.getWorkerIdentity().compareTo(o2.getWorkerIdentity()); // <= } }); .... } ``` Copy\-paste, невнимательность, спешка и много других причин сделать эту глупую ошибку\. При проверках open source проектов ошибки такого рода встречаются довольно часто\. Есть даже целая [статья](https://pvs-studio.ru/ru/blog/posts/cpp/0509/) про это\. [V6020](https://pvs-studio.ru/ru/docs/warnings/v6020/) Divide by zero\. The range of the 'divisor' denominator values includes zero\. SqlMathUtil\.java\(265\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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\) ```cpp public static Operator findSourceRS(....) { .... List> parents = ....; if (parents == null | parents.isEmpty()) { // reached end e.g. TS operator return null; } .... } ``` Вместо логического оператора \|\| написали битовый оператор \|\. Это означает, что правая часть будет выполнена независимо от результата левой части\. Такая опечатка, в случае _parents \=\= null_, сразу же приведет к NPE в следующем логическом подвыражении\. [V6042](https://pvs-studio.ru/ru/docs/warnings/v6042/) The expression is checked for compatibility with type 'A' but is cast to type 'B'\. VectorColumnAssignFactory\.java\(347\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6060/) The 'var' reference was utilized before it was verified against null\. Var\.java\(402\), Var\.java\(395\) ```cpp @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\)\. Код писался многими разработчиками долгие годы и, как следствие, статическому анализатору есть что находить\. Это еще раз подтверждает то, что статический анализ крайне важен и полезен при разработке средних и больших проектов\! Примечание\. Подобные разовые проверки демонстрируют возможности статического анализатора кода, но являются совершенно неправильным способом его использовать\. Более подробно эта мысль изложена [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0594/) и [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0639/)\. Используйте анализ регулярно\! При проверке Hive обнаружено достаточное количество дефектов и подозрительных моментов\. Если эта статья привлечет внимание команды разработчиков Apache Hive, то нам будем приятно внести свой вклад в это нелегкое дело\. Невозможно представить Apache Hive без Apache Hadoop, поэтому вполне вероятно, что единорог от PVS\-Studio заглянет и туда\. Но на сегодня это все, а пока [скачивайте](https://pvs-studio.ru/ru/pvs-studio/download/) анализатор и проверяйте собственные проекты\.