" + message + "
"); // <= .... } } ``` Срабатывание анализатора PVS\-Studio: [V5330](https://pvs-studio.ru/ru/docs/warnings/v5330/)\. Possible XSS injection\. Potentially tainted data in the 'message' variable might be used to execute a malicious script\. [HelloSessionServlet\.java 70](https://github.com/jetty/jetty.project/blob/787aa033145ca93eae870f4728cec3aabe2651d4/jetty-demos/jetty-servlet6-demos/jetty-servlet6-demo-embedded/src/main/java/org/eclipse/jetty/servlet6/embedded/HelloSessionServlet.java#L70) Анализатор говорит о том, что здесь возможна [XSS\-инъекция](https://owasp.org/www-community/attacks/xss/)\. Давайте посмотрим\. В коде мы получаем строку из запроса, на основе неё формируем сообщения, а потом на основе этих сообщений формируем html\-страницу\. Поскольку данные из запроса никак не [санитизируются](https://pvs-studio.ru/ru/blog/terms/6496/), здесь действительно возможен XSS\. Если переданный извне `message` будет представлять собой, к примеру, строку ``, то Java\-script код `alert("XSS Injection")` выполнится при открытии страницы\. Как и любой другой js код\. Но если речь идёт о реальной уязвимости, почему это не первое место топа? Всё дело в том, что мы имеем дело с демонстрационным примером\. То есть этот код нужен сугубо для ознакомления с возможностями jetty\. И тем не менее, стоит помнить о том, что демонстрационный пример не исключает фактор риска, поскольку он может стать источником заимствования\. Всё же "демонстрировать" небезопасный код — не лучшая практика\. За первое [taint](https://pvs-studio.ru/ru/blog/terms/6496/)\-срабатывание в реальном проекте, хоть и в демонстрационном примере, мы награждаем его 5\-ым местом в топе\. ## 4 место\. У нас в меню только Double Ошибка из проекта [Elasticsearch](https://github.com/elastic/elasticsearch)\. Фрагмент кода с ошибкой: ```cpp public static Number truncate(Number n, Number precision) { .... Double result = (((n.doubleValue() < 0) ? Math.ceil(g) : Math.floor(g)) / tenAtScale); return n instanceof Float ? result.floatValue() : result; // <= } ``` Срабатывание анализатора PVS\-Studio: [V6088](https://pvs-studio.ru/ru/docs/warnings/v6088/) Result of this expression will be implicitly cast to 'double'\. Check if program logic handles it correctly\. [Maths\.java 122](https://github.com/elastic/elasticsearch/blob/f2628392d1b89b1c96f5c835e70432ce568d5e9f/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/math/Maths.java#L122) Этот метод "усекает" переданное число в зависимости от заданной точности\. Для целых чисел — сокращает заданное количество разрядов, а для дробных — убирает лишние числа после запятой\. Выше приведён фрагмент, где работа ведётся с дробным числом\. Судя по тернарному оператору, планировалось, что в случае с дробными числами метод вернёт число того же типа, с котором оно пришло в метод \(либо `Float`, либо `Double`\)\. Но не самый очевидный нюанс здесь сыграл свою роль\. Сам по себе тернарный оператор является выражением\. Как следствие, возвращаемый им результат должен иметь один заранее определённый тип\. И тут вступают в игру правила из [Numeric Context](https://docs.oracle.com/javase/specs/jls/se24/html/jls-5.html#jls-5.6)\. Если кратко, то нам здесь важно следующее: 1. Если в выражении фигурируют объекты разных типов, их приводят к одному общему типу\. 1. В случае выше таким типом будет `Double`, поскольку он в себя вмещает и `Double`, и `Float`\. То есть, несмотря на ожидания, в случае с дробными числами метод всегда будет возвращать `Double` значение\. Компактная форма записи действительно подвела\. Возможный вариант исправления выглядит так: ```cpp public static Number truncate(Number n, Number precision) { .... if (n instanceof Float) { return result.floatValue(); } else { return result; } } ``` За неочевидный языковой нюанс ошибка награждается четвёртым местом в топе\. ## 3 место\. Соблюдайте очередь\! И мы переходим к подиуму\. Бронзовый призёр располагается в проекте [OpenIDE](https://gitflic.ru/company/openide)\. Итак, сам код: ```cpp public final class UsageType { .... public static final UsageType DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED = new UsageType( UsageViewBundle.messagePointer( "usage.type.delegate.to.another.instance.method.parameters.changed" ) ); private static final Logger LOG = Logger.getInstance(UsageType.class); public UsageType(@NotNull Supplier<....> nameComputable) { myNameComputable = nameComputable; if (ApplicationManager.getApplication().isUnitTestMode()) { String usageTypeString = myNameComputable.get(); if (usageTypeString.indexOf('{') != -1) { LOG.error(....); } } } .... } ``` Срабатывание анализатора PVS\-Studio: [V6050](https://pvs-studio.ru/ru/docs/warnings/v6050/) Class initialization cycle is present\. Initialization of 'DELEGATE\_TO\_ANOTHER\_INSTANCE\_PARAMETERS\_CHANGED' appears before the initialization of 'LOG'\. [UsageType\.java 46](https://gitflic.ru/project/openide/openide/blob?file=platform%2FusageView%2Fsrc%2Fcom%2Fintellij%2Fusages%2Fimpl%2Frules%2FUsageType.java&branch=252&commit=ea0f8f29691528fa7f4d58218f96a4f678367a73#line-num-46) Анализатор говорит, что мы имеем дело с циклической зависимостью при инициализации статических полей\. Снова не самый очевидный момент, связанный с нюансами языка Java\. Для инициализации поля `DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED` используется конструктор\. А в нём мы обращаемся к логгеру `LOG`\. Всё дело в том, что поля [инициализируются в порядке их объявления](https://docs.oracle.com/javase/specs/jls/se25/html/jls-12.html#jls-12.4.2)\. Как вы можете видеть, `DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED` располагается выше `LOG`, т\.е\. инициализируется раньше\. Как следствие, `LOG` на момент исполнения конструктора [будет равен](https://docs.oracle.com/javase/specs/jls/se25/html/jls-12.html#jls-12.3.2) `null`, и при обращении к нему мы столкнёмся с NPE\. Исправление, как и в большинстве случаев выше, будет донельзя простым\. Нужно просто разместить объявление `LOG` выше, чем `DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED`\. ## 2 место\. Потеряли целый блок Мы перешли к серебренному призёру\. И снова с нами Minecraft мод [CustomNPC\+](https://www.curseforge.com/minecraft/mc-mods/customnpc-plus)\. Фрагмент кода, в котором допущена ошибка: ```cpp public int range = 5; public int speed = 5; .... public boolean aiShouldExecute() { healTicks++; if (healTicks < speed * 10) return false; for (Object plObj : npc.worldObj.getEntitiesWithinAABB( EntityLivingBase.class, npc.boundingBox.expand( range, range / 2, // <= range)) ) { .... } healTicks = 0; return !toHeal.isEmpty(); } ``` Срабатывание анализатора PVS\-Studio: [V6094](https://pvs-studio.ru/ru/docs/warnings/v6094/) The expression was implicitly cast from 'int' type to 'double' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. [JobHealer\.java 41](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/roles/JobHealer.java#L41) В этом фрагменте кода с промежутком времени, указанным в поле `speed`, выполняется поиск всех сущностей в прямоугольнике с радиусом `range` вокруг NPC с профессией лекаря\. Поскольку у метода `expand` сигнатура `expand(double x, double y, double z)`, срабатывание анализатора здесь по делу: методу передаётся `int`, когда ожидается `double`\. "Ну и что с того?" — спросите вы\. Нам это показалось странным, и мы решили разобраться\. Такой небольшой недочёт приводит к тому, что если радиус — нечётное число, при расчёте координаты Y мы теряем по половине блока сверху и снизу \(то есть суммарно целый блок\)\. Убедиться в этом нам помогла визуализация коллизии, которая рассчитывается на основе радиуса\. Поделимся этой визуализацией с вами:  * Белым выделена коллизия NPC\-лекаря\. * Красным выделена коллизия, реализованная сейчас \(5 / 2\)\. * Зелёным выделена коллизия без целочисленного деления \(5 / 2\.0\)\. И да, как вы можете заметить, если использовать `double` литерал `2.0`, то деление перестаёт быть целочисленным, и как следствие, коллизия охватит именно тот радиус, который мы желаем\. А ещё автору [статьи](https://pvs-studio.ru/ru/blog/posts/java/1219/) ради подтверждения правильности срабатывания пришлось "совершить запрос в космос": > Очень важно то, что в оригинальной версии модификации для Minecraft 1\.12\.2 эту ошибку исправили\. К сожалению, я не смогу предоставить прямые доказательства, поэтому давайте представим, что я совершил запрос в космос, который мне вернул фрагмент кода: ```cpp .... this.npc.world.getEntitiesWithinAABB( EntityLivingBase.class, npc.getEntityBoundingBox().expand(range, range / 2.0, range) ); .... ``` > То есть баг был исправлен в оригинальной модификации, и тут надо сделать то же самое\. ## 1 место\. Отряд не заметил потери бонуса Мы переходим к нашему золотому призёру\. И им будет\.\.\. также фрагмент из [CustomNPC\+](https://www.curseforge.com/minecraft/mc-mods/customnpc-plus)\. Связано ли обилие Minecraft ошибок с тем, что половина нашей команды занималась модами и плагинами для него? Кто знает\. Переходим к коду: ```cpp .... int startIndex = -1; boolean number = false; try { startIndex = Integer.parseInt(bonusID); number = true; } catch (Exception var34) { number = false; } for (startIndex = 0; startIndex < bonuses.length; ++startIndex) { .... if (number && startIndex == startIndex || // <= !number && bonusValues[startIndex][0].equals(bonusID) ) { noNBTText = bonusValues[startIndex][0] + ";" + bonusValueString; bonuses[startIndex] = ""; bonuses[startIndex] = noNBTText; .... break; } } .... ``` Срабатывания анализатора PVS\-Studio: V6001 There are identical sub\-expressions 'startIndex' to the left and to the right of the '\=\=' operator\. [ScriptDBCPlayer\.java 289](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L289) V6007 Expression '\!number' is always true\. [ScriptDBCPlayer\.java 289](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L289) V6033 An item with the same key 'startIndex' has already been changed\. [ScriptDBCPlayer\.java 293](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L293) Действительно ли срабатывания на этот код указывают на ошибку? И если да, то к чему она приводит? Здесь нас ожидает целое детективное расследование с интересным финалом\. Для начала давайте рассмотрим первое срабатывание\. Сравнение `startIndex` с самим собой действительно кажется странным\. Автор статьи, надев свою детективную шляпу и длинный плащ, пошёл искать улики в окрестностях этого класса\. Улики лежали совсем неподалёку: Файл: [ScriptDBCPlayer\.java\(224\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L224): ```cpp .... int num = -1; boolean number; try { num = Integer.parseInt(bonusID); number = true; } catch (Exception var33) { number = false; } for (int i = 0; i < bonuses.length; ++i) { .... if (number && i == num || !number && bonusValues[i][0].equals(bonusID) ) { bonuses[i] = ""; break; } } .... ``` Очень похожий код, но не зря автор статьи имеет прозвище "зоркий глаз"\. Отличие заключается в том, что во втором случае переменные в `try` блоке и в цикле `for` **разные**: `num` и `i`\. В первом же фрагменте мы сначала инициализируем `startIndex` в `try` блоке, а потом перетираем её в цикле `for`\. Поэтому во втором фрагменте в идентичной проверке сравниваются две разные переменные: `num` и `i`\. А в первом имеем то, на что указал анализатор — `startIndex == startIndex`\. Хорошо, мы поняли, как первый фрагмент должен был выглядеть\. **Но** **на что ошибка** **влияет**? Тут детектив обратил внимание на название самого класса: префикс `Script` означает, что класс применяется в механизме игровых сценариев\. Сам метод, в котором обнаружена ошибка, ответственен за выставление бонусов игровым атрибутам, таким как сила, ловкость и т\.д\. По логике этого метода ожидалось, что во внутриигровом скрипте мы выставим определённый индекс бонуса атрибута\. Как оно должно было быть? Сначала мы должны были в блоке `try` спарсить индекс бонуса: ```cpp try { num = Integer.parseInt(bonusID); number = true; } .... ``` А потом в цикле выставить соответствующему бонусу переданное значение: ```cpp for (int i = 0; i < bonuses.length; ++i) { .... if (number && i == num || ....) { noNBTText = bonusValues[i][0] + ";" + bonusValueString; bonuses[i] = ""; bonuses[i] = noNBTText; .... break; } } ``` Но во фрагменте с ошибкой полученный из скрипта индекс атрибута перетирается нулём, а потом сравнивается с самим собой: ```cpp try { startIndex = Integer.parseInt(bonusID); number = true; } catch (Exception var34) { number = false; } for (startIndex = 0; startIndex < bonuses.length; ++startIndex) { .... if (number && startIndex == startIndex || // <= !number && bonusValues[startIndex][0].equals(bonusID) ) { noNBTText = bonusValues[startIndex][0] + ";" + bonusValueString; .... } } ``` То есть переданное в скрипте значение бонуса всегда выставляется** первому** бонусу\. А всё из\-за того, что `startIndex` в `for` условии перетирается и становится равным нулю, а потом сравнивается сам с собой\. А теперь, раз проблема понятна, предлагаю посмотреть на последствия ошибки\. В первую очередь нам нужно написать внутриигровой скрипт, который создаст бонусы атрибуту "Сила"\. Сначала создадим ему два бонуса со значениями 1, а потом выставим каждому из них значения 5 и 15 соответственно:  Вот так выглядят внутриигровые скрипты\. Мы создали бонусы для "Силы" с индексами 0 и 1, а потом выставили им желаемые значения\. Теперь, по идее, если вывести эти два бонуса у атрибута "Сила", мы увидим 5 и 15? Если бы ошибки в коде не было, так бы и произошло\. Однако, как мы поняли из блока выше, ошибка приводит к перетиранию значения у первого бонуса\. Чтобы проверить, давайте напишем следующий внутриигровой скрипт для вывода значений бонусов:  Результат:  Действительно, второй бонус у "Силы" также имеет начальное значение 1\. В то время как первый бонус — 15\. Получается, что сначала мы выставили первому бонусу значение 5, а потом перетёрли его значением 15\. Наша гипотеза выше оказалась правдивой\. Дело раскрыто\. За необходимость повозиться и возможность таким вот образом воспроизвести эту ошибку мы награждаем её почётным первым местом\! ## Итоги Вот мы и рассмотрели 10, на наш взгляд, самых интересных ошибок\. Надеюсь, вам понравилось\! Если есть мысли по поводу этого топа, и вы хотите ими поделиться, будем рады видеть вас в комментариях\. Также, если вам интересна проверка отдельных проектов, то можете найти их в нашем [блоге](https://pvs-studio.ru/ru/blog/posts/) или на [этой странице](https://pvs-studio.ru/ru/blog/inspections/)\. Если хотите попробовать сам анализатор PVS\-Studio, сделать это можно по [этой ссылке](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. А если вы студент, преподаватель или обладатель Open Source проекта на C, C\+\+, C\# или Java, вы можете использовать PVS\-Studio бесплатно\. Все подробности [тут](https://pvs-studio.ru/ru/blog/posts/1286/)\. Мне остаётся лишь попрощаться с вами и пожелать вам счастливого Нового Года\! До скорых встреч\!