- PVSM.RU - https://www.pvsm.ru -
Последние десять лет движение open source является одним из ключевых факторов развития IT-отрасли и важной ее составной частью. Роль и место open source не только усиливается в виде роста количественных показателей, но происходит и изменение его качественного позиционирования на IT-рынке в целом. Не сидя сложа руки, бравая команда PVS-Studio активно способствует закреплению позиций open source проектов, находя затаившиеся баги в огромных толщах кодовых баз и предлагая для таких проектов бесплатные лицензии. Эта статья не исключение! Сегодня речь пойдет об Apache Hive! Отчет получен — есть на что посмотреть!
Статический анализатор кода PVS-Studio [1] существует более 10 лет на IT рынке и представляет собой многофункциональное и легко внедряемое программное решение. На данный момент анализатор поддерживает языки C, C++, C#, Java и работает на платформах Windows, Linux и macOS.
PVS-Studio является платным B2B-решением и используется большим количеством команд в различных компаниях. Если хотите посмотреть, на что способен анализатор, то скачать дистрибутив и запросить триальный ключ можно здесь [1].
Если Вы гик open source или, например, являетесь студентом, то вы можете воспользоваться одним из бесплатных вариантов [2] лицензирования PVS-Studio.
Объем данных в последние годы растет с большой скоростью. Стандартные базы данных больше не могут поддерживать работоспособность при таком темпе роста объёма информации, что послужило появлению термина 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].
Последовательность действий для анализа достаточно проста и не потребовала много времени:
Результаты анализа: 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 [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.
Еще схожие предупреждения строками ниже:
Как итог, никакие действия в конструкции if-else-if никогда выполняться не будут.
Некоторые другие предупреждения V6007 [14]:
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, выполнение заходит в условие, из которого возвращаем наружу пустой список. Чего стоит нам это возвращение? А стоит это того, что все пойманные исключения никогда не будут выброшены наружу и обработаны соответствующим способом. Все те исключения, что указаны в сигнатуре метода, никогда не будут выброшены и просто сбивают с толку.
Подобное предупреждение:
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.
Еще предупреждения:
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:
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. Увы, проверка есть, но не там.
Подобные подозрительные моменты с использованием объекта перед тем, как происходит проверка:
Кто хоть чуть-чуть интересовался 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
Нажмите здесь для печати.