ugeneunipro / ugene

UGENE is free open-source cross-platform bioinformatics software
http://ugene.net
GNU General Public License v2.0
207 stars 60 forks source link

UGENE-8138 QString throws std::bad_alloc #1665

Closed DmitriiSukhomlinov closed 2 weeks ago

DmitriiSukhomlinov commented 2 weeks ago

Решение креша 26025.

Проблема возникла, когда пользователь попытался скопировать кусок выравнивания неизвестного, но, судя по логу, вполне вероятно, что немаленького объема (по логу можно понять, что он делал выделение требуемого куска не с помощью одной рамки, а щелкнув в одно место, проскроллив в некоторое другое место и сделав выделение через Shift). Он попытался вырезать кусок выравнивания и QString, который должен был временно хранить всю выделяемую часть, кинул исключение qBadAlloc, которое никем не было обработано. Это произошло в функции MsaEditorSequenceArea::copySelection(...). Эта функция имеет механизм защиты от копирования выравнивания слишком большого объема и максимальный объем в нашем случае фиксирован. Однако, по факту, копируемое выравнивание может оказаться меньше заявленного нами лимита (т.е. проверка пропустит вычисления), а QString потом все равно кинет исключение (это может зависеть от целого ряда факторов - например малого объема оперативной памяти компьютера или его сильной загруженности в моменте). Я полагаю, что это и произошло в текущем креше, однако, стабильного сценария воспроизведения у данной проблемы найти не получится. Я предлагаю добавить в данное место try-catch, который будет обрабатывать исключение, если оно возникнет.

В аналогичных местах представлено такое же исправление.

DmitriiSukhomlinov commented 2 weeks ago

Решение неверное. Верное уже обсуждалось ранее и есть несколько примеров его реализации.

Верное решение состоит в том, чтобы не доводить UGENE до состояния падения, в результате которого возможно уже любое поведение.

Например, UGENE может упать не в этом участке кода, а в соседнем, так как этот участок кода ранее выделил столько памяти, что следующим уже ничего не осталось даже в разумных пределах. Либо система можен начать закрывать другие приложения, так как UGENE надо больше памяти.

Нужно сделать проверку на переполнение до падения и, при необходимости, отказаться выполнять потенциально опасную операцию вовсе.

Текущее решение не работает, на это указал пришедший креш. Мне нужны конкретные примеры реализации верного, по твоему мнению, решения.

yalgaer commented 2 weeks ago

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

Для MSA при копировании текста максимальный размер памяти не должен быть слишком большим: 10Мб? Иначе, пусть делают export в файл.

Также нужен тест демонстрирующий проблему: думаю его будет написать не сложно, если сгенерировать alignment выше установленного лимита.

DmitriiSukhomlinov commented 2 weeks ago

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

Для MSA при копировании текста максимальный размер памяти не должен быть слишком большим: 10Мб? Иначе, пусть делают export в файл.

Также нужен тест демонстрирующий проблему: думаю его будет написать не сложно, если сгенерировать alignment выше установленного лимита.

Так, я проговорю еще раз - все, что ты описала, на данный момент уже присутствует в UGENE. Существует специальная функция U2Clipboard::checkCopyToClipboardSize, которая вызывается для проверки размера данных, которые будут записаны в буфер обмена перед тем, как начать сам процесс записи. Ограничение составляет ~100Мб в общем случае и 1Мб для тестирования, чтобы можно было воспроизвести ошибку на тестах без использования больших данных. Проблема в том, что ограничение в 100Мб взято с потолка и никак не синхронизовано с объемом памяти, на котором QString кидает std::bad_alloc - и синхронизовать его, очевидно, невозможно, т.к. это исключение кидается оператором new, который вшит где-то глубоко внутри QString. В связи с чем, у нас пришел креш, где проверка проходит, а исключение все равно кидается. Исходя из этого, какие твои предложения? Уменьшить ограничение до 10 Мб?

yalgaer commented 2 weeks ago

Правильно, так как у нас уже есть специальная проверка которая не должна допускать этой ситуации то нужно понять, почему она не работает.

Не работать она может по двум причинам: неправильно реализован подсчёт копируемого размера, либо неправильно предполагается что 100мб это малый размер памяти, который не приведет к проблемам.

Если проверка размера реализована правильно, то остается только уменьшить размер памяти со 100Мб до 10Мб

Это будет вполне разумно, так как вставка 100Мб практически в любой текстовый редактор будет приводить к проблемам - это слишком большой размер.

Идеальным же решением будет сказать пользователю, что размер для вставки в clipboard очень большой и предложить ему сделать export выделенных данных в файл.

DmitriiSukhomlinov commented 2 weeks ago

Правильно, так как у нас уже есть специальная проверка которая не должна допускать этой ситуации то нужно понять, почему она не работает.

Не работать она может по двум причинам: неправильно реализован подсчёт копируемого размера, либо неправильно предполагается что 100мб это малый размер памяти, который не приведет к проблемам.

Если проверка размера реализована правильно, то остается только уменьшить размер памяти со 100Мб до 10Мб

Это будет вполне разумно, так как вставка 100Мб практически в любой текстовый редактор будет приводить к проблемам - это слишком большой размер.

Идеальным же решением будет сказать пользователю, что размер для вставки в clipboard очень большой и предложить ему сделать export выделенных данных в файл.

Выше я конкретно объяснил, почему она не работает. Само значение максимального объема буфера обмена в 100 Мб взято из головы, оператору new на это ограничение начихать, он работает на уровне языка и кидает std::bad_alloc когда сам посчитает нужным, в зависимости от миллиарда параметров, которые мы просчитать никак не можем. Правильное решение - это использовать инфраструктуру исключений, которую один умный чел по имени Бьерн Страуструп придумал и встроил в язык именно на такие случаи. Неправильное - это городить огород в виде кастомных ограничений, который мне изначально не нравился.

С кастомными ограничениями проблемы лично мне очевидны, но на всякий случай я проговрю их вслух:

Решение о кастомном ограничении изначально было плохим, сейчас ты предлагаешь сделать его еще хуже, введя еще более жесткое ограничение на объем - при этом, работоспособность решения все так же будет под вопросом (пункт два выше). Я готов реализовать только правильно решение, включающее систему обработки исключений, которая так заботливо предоставлена нам на уровне языка специально для таких случаев. Если тебя оно не устраивает - закрывай PR и переводи задачу на себя. Это креш, потому надо сделать до релиза.

yalgaer commented 2 weeks ago

Как суперкомпьютер, так и калькулятор не должны приводить к состоянию ошибки OutOfMemory ни в С++ ни в любом другом языке.

Защита от ошибок типа OutOfMemory или StackOverflow - это крайний случай защиты, когда использованы все остальные возможности.

Здесь же, у нас, есть еще потенциал для улучшения перед тем, как бросить все и начать полагаться на переполнение памяти.

Если тебя оно не устраивает - закрывай PR и переводи задачу на себя. Это креш, потому надо сделать до релиза.

Ок, закрываю