Русский
Русский
English
Статистика
Реклама

Баги

Почему обзоры кода это хорошо, но недостаточно

23.09.2020 18:06:49 | Автор: admin
image1.png

Обзоры кода однозначно нужны и полезны. Это возможность передать знания, обучение, контроль выполнения задачи, улучшение качества и оформления кода, исправление ошибок. Причем можно замечать высокоуровневые ошибки, связанные с используемой архитектурой и алгоритмами. В общем всё хорошо, но люди быстро устают. Поэтому статический анализ великолепно дополняет обзоры и помогает выявлять разнообразнейшие неприметные на глаз ошибки и опечатки. Рассмотрим хороший пример на эту тему.

Попробуйте найти ошибку в коде функции, взятой из библиотеки structopt:

static inline bool is_valid_number(const std::string &input) {  if (is_binary_notation(input) ||      is_hex_notation(input) ||      is_octal_notation(input)) {    return true;  }  if (input.empty()) {    return false;  }  std::size_t i = 0, j = input.length() - 1;  // Handling whitespaces  while (i < input.length() && input[i] == ' ')    i++;  while (input[j] == ' ')    j--;  if (i > j)    return false;  // if string is of length 1 and the only  // character is not a digit  if (i == j && !(input[i] >= '0' && input[i] <= '9'))    return false;  // If the 1st char is not '+', '-', '.' or digit  if (input[i] != '.' && input[i] != '+' && input[i] != '-' &&      !(input[i] >= '0' && input[i] <= '9'))    return false;  // To check if a '.' or 'e' is found in given  // string. We use this flag to make sure that  // either of them appear only once.  bool dot_or_exp = false;  for (; i <= j; i++) {    // If any of the char does not belong to    // {digit, +, -, ., e}    if (input[i] != 'e' && input[i] != '.' &&        input[i] != '+' && input[i] != '-' &&        !(input[i] >= '0' && input[i] <= '9'))      return false;    if (input[i] == '.') {      // checks if the char 'e' has already      // occurred before '.' If yes, return false;.      if (dot_or_exp == true)        return false;      // If '.' is the last character.      if (i + 1 > input.length())        return false;      // if '.' is not followed by a digit.      if (!(input[i + 1] >= '0' && input[i + 1] <= '9'))        return false;    }    else if (input[i] == 'e') {      // set dot_or_exp = 1 when e is encountered.      dot_or_exp = true;      // if there is no digit before 'e'.      if (!(input[i - 1] >= '0' && input[i - 1] <= '9'))        return false;      // If 'e' is the last Character      if (i + 1 > input.length())        return false;      // if e is not followed either by      // '+', '-' or a digit      if (input[i + 1] != '+' && input[i + 1] != '-' &&          (input[i + 1] >= '0' && input[i] <= '9'))        return false;    }  }  /* If the string skips all above cases, then  it is numeric*/  return true;}

Чтобы случайно сразу не прочитать ответ, добавлю картинку.

image2.png

Не знаю, нашли Вы ошибку или нет. Даже если нашли, то уверен согласитесь, что найти такую опечатку непросто. Тем более, вы знали, что в функции есть ошибка. Если не знать, то сложно заставить внимательно читать и проверять весь этот код.

В таких ситуациях статический анализатор кода отлично дополнит классический обзор кода. Анализатор не устаёт и дотошно проверит весь код. Как результат, в этой функции анализатор PVS-Studio замечает аномалию и выдаёт предупреждение:

V560 A part of conditional expression is always false: input[i] <= '9'. structopt.hpp 1870

Для тех, кто не заметил ошибку, дам пояснения. Самое главное:

else if (input[i] == 'e') {  ....  if (input[i + 1] != '+' && input[i + 1] != '-' &&      (input[i + 1] >= '0' && input[i] <= '9'))      return false;}

Вышестоящее условие проверяет, что i-тый элемент является буквой 'e'. Соответственно следующая проверка input[i] <= '9' не имеет смысла. Результат второй проверки всегда false, о чём и предупреждает инструмент статического анализа. Причина ошибки проста: человек поспешил и опечатался, забыв написать +1.

Фактически получается, что функция не до конца выполняет свою работу по проверке корректности введённых чисел. Правильный вариант:

else if (input[i] == 'e') {  ....  if (input[i + 1] != '+' && input[i + 1] != '-' &&      (input[i + 1] >= '0' && input[i + 1] <= '9'))      return false;}

Интересное наблюдение. Эту ошибку можно рассматривать как разновидность "эффекта последней строки". Ошибка допущена в самом последнем условии функции. В конце внимание программиста ослабло, и он допустил эту малозаметную ошибку.


Если вам понравится статья про эффект последней строки, то рекомендую познакомиться с другими аналогичными наблюдениями: 0-1-2, memset, сравнения.

Всем пока. Ставлю лайк тем, кто самостоятельно нашёл ошибку.
Подробнее..

Проверка коллекции header-only C библиотек (awesome-hpp)

22.10.2020 12:07:47 | Автор: admin
PVS-Studio и Awesome hpp

Волею судьбы мы проверили большинство библиотек, входящих в коллекцию под названием "Awesome hpp". Это небольшие проекты на языке C++, состоящие только из заголовочных файлов. Надеемся, найденные ошибки помогут сделать эти библиотеки немного лучше. Также мы будем рады, если их авторы начнут бесплатно использовать анализатор PVS-Studio на регулярной основе.

Предлагаю вашему вниманию обзор результатов проверки различных библиотек, перечисленных в списке awesome-hpp (A curated list of awesome header-only C++ libraries).

Впервые про этот список я узнал из подкаста "Cross Platform Mobile Telephony". Пользуясь случаем, рекомендую всем C++ программистам познакомиться с CppCast. CppCast is the first podcast for C++ developers by C++ developers!

Несмотря на большое количество проектов в списке, ошибок нашлось совсем немного. Тому есть три причины:

  • Это очень маленькие проекты. Многие буквально состоят из одного заголовочного файла.
  • Мы проверили не все проекты. С компиляцией некоторых из них возникли проблемы, и мы решили их пропустить.
  • Часто, чтобы понять, есть ли ошибки в шаблонных классах/функциях или нет, они должны инстанцироваться. Соответственно многие ошибки смогут быть выявлены анализатором только в настоящем проекте, когда библиотека активно используется. Мы же просто включали заголовочные файлы в пустой .cpp файл и проверяли, что делает проверку малоэффективной.

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

Примечание для моих коллег
Я люблю совмещать и достигать несколько полезных результатов, занимаясь каким-то делом. Призываю брать пример. Узнав про существование коллекции awesome-hpp, я смог реализовать следующие полезные дела:



Примечание для разработчиков библиотек. Желающие могут бесплатно использовать анализатор PVS-Studio для проверки открытых проектов. Для получения лицензии для вашего открытого проекта заполните, пожалуйста, эту форму.

Теперь давайте наконец посмотрим, что нашлось в некоторых библиотеках.

Найденные ошибки


Библиотека iutest


Краткое описание библиотеки iutest:
iutest is framework for writing C++ tests.
template<typename Event>pool_handler<Event> & assure() {  ....  return static_cast<pool_handler<Event> &>(it == pools.cend() ?    *pools.emplace_back(new pool_handler<Event>{}) : **it);  ....}

Предупреждение PVS-Studio: V1023 A pointer without owner is added to the 'pools' container by the 'emplace_back' method. A memory leak will occur in case of an exception. entt.hpp 17114

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

Пожалуй, для тестов эта ситуация маловероятна и некритична. Однако я решил упомянуть этот недостаток в образовательных целях :).

Правильный вариант:

pools.emplace_back(std::make_unique<pool_handler<Event>>{})

Ещё одно такое же место: V1023 A pointer without owner is added to the 'pools' container by the 'emplace_back' method. A memory leak will occur in case of an exception. entt.hpp 17407

Библиотека jsoncons


Краткое описание библиотеки jsoncons:
A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON.
Первая ошибка

static constexpr uint64_t basic_type_bits = sizeof(uint64_t) * 8;uint64_t* data() {  return is_dynamic() ? dynamic_stor_.data_ : short_stor_.values_;}basic_bigint& operator<<=( uint64_t k ){  size_type q = (size_type)(k / basic_type_bits);  ....  if ( k )  // 0 < k < basic_type_bits:  {    uint64_t k1 = basic_type_bits - k;    uint64_t mask = (1 << k) - 1;             // <=    ....    data()[i] |= (data()[i-1] >> k1) & mask;    ....  }  reduce();  return *this;}

Предупреждение PVS-Studio: V629 Consider inspecting the '1 << k' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bigint.hpp 744

Эта ошибка подробно уже рассматривалась в статье "Почему важно проводить статический анализ открытых библиотек, которые вы добавляете в свой проект". Если совсем кратко, то, чтобы получать корректные значения маски, нужно написать так:

uint64_t mask = (static_cast<uint64_t>(1) << k) - 1;

Или так:

uint64_t mask = (1ull << k) - 1;

Точно такую же ошибку, как первая, можно увидеть здесь: V629 Consider inspecting the '1 << k' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bigint.hpp 779

Вторая ошибка

template <class CharT = typename std::iterator_traits<Iterator>::value_type>typename std::enable_if<sizeof(CharT) == sizeof(uint16_t)>::type next() UNICONS_NOEXCEPT{    begin_ += length_;    if (begin_ != last_)    {        if (begin_ != last_)        {  ....}

Предупреждение PVS-Studio: V571 Recurring check. The 'if (begin_ != last_)' condition was already verified in line 1138. unicode_traits.hpp 1140

Странная повторная проверка. Есть подозрение, что здесь какая-то опечатка и второе условие должно выглядеть как-то иначе.

Библиотека clipp


Краткое описание библиотеки clipp:
clipp command line interfaces for modern C++. Easy to use, powerful and expressive command line argument handling for C++11/14/17 contained in a single header file.
inline boolfwd_to_unsigned_int(const char*& s){  if(!s) return false;  for(; std::isspace(*s); ++s);  if(!s[0] || s[0] == '-') return false;  if(s[0] == '-') return false;  return true;}

Предупреждение PVS-Studio: V547 Expression 's[0] == '-'' is always false. clipp.h 303

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

Библиотека SimpleIni


Краткое описание библиотеки SimpleIni:
A cross-platform library that provides a simple API to read and write INI-style configuration files. It supports data files in ASCII, MBCS and Unicode.
#if defined(SI_NO_MBSTOWCS_NULL) || (!defined(_MSC_VER) && !defined(_linux))

Предупреждение PVS-Studio: V1040 Possible typo in the spelling of a pre-defined macro name. The '_linux' macro is similar to '__linux'. SimpleIni.h 2923

Скорее всего, в имени макроса _linux не хватает одного подчёркивания и должно использоваться имя __linux. Впрочем, в POSIX этот макрос объявлен устаревшим и лучше использовать __linux__.

Библиотека CSV Parser


Краткое описание библиотеки CSV Parser:
A modern C++ library for reading, writing, and analyzing CSV (and similar) files.
CSV_INLINE void CSVReader::read_csv(const size_t& bytes) {  const size_t BUFFER_UPPER_LIMIT = std::min(bytes, (size_t)1000000);  std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);  auto * HEDLEY_RESTRICT line_buffer = buffer.get();  line_buffer[0] = '\0';  ....  this->feed_state->feed_buffer.push_back(    std::make_pair<>(std::move(buffer), line_buffer - buffer.get())); // <=  ....}

Предупреждение PVS-Studio: V769 The 'buffer.get()' pointer in the 'line_buffer buffer.get()' expression equals nullptr. The resulting value is senseless and it should not be used. csv.hpp 4957

Интересная ситуация, которая требует внимательного рассмотрения. Поэтому я решил, что напишу про это отдельную маленькую заметку. Плюс, ставя эксперименты с аналогичным кодом, я выявил недоработку в самом PVS-Studio :). В некоторых случаях он молчит, хотя должен выдавать предупреждения.

Если совсем кратко, работает этот код или нет, зависит от порядка вычисления аргументов. А в каком порядке вычисляются аргументы, зависит от компилятора.

Библиотека PPrint


Краткое описание библиотеки PPrint:.
Pretty Printer for Modern C++.
template <typename Container>typename std::enable_if<......>::type print_internal(......) {  ....  for (size_t i = 1; i < value.size() - 1; i++) {    print_internal(value[i], indent + indent_, "", level + 1);    if (is_container<T>::value == false)      print_internal_without_quotes(", ", 0, "\n");    else      print_internal_without_quotes(", ", 0, "\n");  }  ....}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 715

Очень странно, что независимо от условия выполняется одно и то же действие. Нет и какого-то специального поясняющего комментария. Всё это очень похоже на Copy-Paste ошибку.

Аналогичные предупреждения:

  • V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 780
  • V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 851
  • V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 927
  • V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 1012

Библиотека Strf


Краткое описание библиотеки Strf:
A fast C++ formatting library that supports encoding conversion.
Первая ошибка

template <int Base>class numpunct: private strf::digits_grouping{  ....  constexpr STRF_HD numpunct& operator=(const numpunct& other) noexcept  {    strf::digits_grouping::operator=(other);    decimal_point_ = other.decimal_point_;    thousands_sep_ = other.thousands_sep_;  }  ....};

Предупреждение PVS-Studio: V591 Non-void function should return a value. numpunct.hpp 402

В конце функции забыли написать "return *this;".

Вторая аналогичная ошибка

template <int Base>class no_grouping final{  constexpr STRF_HD no_grouping& operator=(const no_grouping& other) noexcept  {    decimal_point_ = other.decimal_point_;  }  ....}

Предупреждение PVS-Studio: V591 Non-void function should return a value. numpunct.hpp 528.

Библиотека Indicators


Краткое описание библиотеки Indicators:
Activity Indicators for Modern C++.
static inline void move_up(int lines) { move(0, -lines); }static inline void move_down(int lines) { move(0, -lines); }   // <=static inline void move_right(int cols) { move(cols, 0); }static inline void move_left(int cols) { move(-cols, 0); }

Предупреждение PVS-Studio: V524 It is odd that the body of 'move_down' function is fully equivalent to the body of 'move_up' function. indicators.hpp 983

Я не уверен, что это ошибка. Но код очень подозрительный. Высока вероятность, что была скопирована функция move_up и заменено её имя на move_down. А вот минус удалить забыли. Стоит проверить этот код.

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

Библиотека manif


Краткое описание библиотеки manif:
manif is a header-only C++11 Lie theory library for state-estimation targeted at robotics applications.
template <typename _Derived>typename LieGroupBase<_Derived>::Scalar*LieGroupBase<_Derived>::data(){  return derived().coeffs().data();}template <typename _Derived>const typename LieGroupBase<_Derived>::Scalar*LieGroupBase<_Derived>::data() const{  derived().coeffs().data(); // <=}

Предупреждение PVS-Studio: V591 Non-void function should return a value. lie_group_base.h 347

Неконстантная функция реализована правильно, а константная нет. Интересно даже, как так получилось

Библиотека FakeIt


Краткое описание библиотеки FakeIt:
FakeIt is a simple mocking framework for C++. It supports GCC, Clang and MS Visual C++. FakeIt is written in C++11 and can be used for testing both C++11 and C++ projects.
template<typename ... arglist>struct ArgumentsMatcherInvocationMatcher :         public ActualInvocation<arglist...>::Matcher {  ....  template<typename A>  void operator()(int index, A &actualArg) {      TypedMatcher<typename naked_type<A>::type> *matcher =        dynamic_cast<TypedMatcher<typename naked_type<A>::type> *>(          _matchers[index]);      if (_matching)        _matching = matcher->matches(actualArg);  }  ....  const std::vector<Destructible *> _matchers;};

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'matcher'. fakeit.hpp 6720

Указатель matcher инициализируется значением, которое возвращает оператор dynamic_cast. А этот оператор может возвращать nullptr, и это весьма вероятный сценарий. Иначе вместо dynamic_cast эффективнее использовать static_cast.

Есть подозрение, что в условии допущена опечатка и на самом деле должно быть написано:

if (matcher)  _matching = matcher->matches(actualArg);

Библиотека GuiLite


Краткое описание библиотеки GuiLite:
The smallest header-only GUI library(4 KLOC) for all platforms.
#define CORRECT(x, high_limit, low_limit)  {\  x = (x > high_limit) ? high_limit : x;\  x = (x < low_limit) ? low_limit : x;\}while(0)void refresh_wave(unsigned char frame){  ....  CORRECT(y_min, m_wave_bottom, m_wave_top);  ....}

Предупреждение PVS-Studio: V529 Odd semicolon ';' after 'while' operator. GuiLite.h 3413

К какой-то проблеме ошибка в макросе не приводит. Но всё равно это ошибка, поэтому я решил описать её в статье.

В макросе планировалось использовать классический паттерн do { } while(....). Это позволяет выполнить несколько действий в одном блоке и при этом иметь возможность для красоты после макроса писать точку с запятой ';', как будто это вызов функции.

Но в рассмотренном макросе случайно забыли написать ключевое слово do. В результате макрос как-бы разделился на две части. Первая это блок. Вторая пустой не выполняющийся цикл: while (0);.

А в чём, собственно, проблема?

Например, такой макрос нельзя использовать в конструкции вида:

if (A)  CORRECT(y_min, m_wave_bottom, m_wave_top);else  Foo();

Этот код не скомпилируется, так как он будет раскрыт в:

if (A)  { ..... }while(0);else  Foo();

Согласитесь, такую проблему лучше найти и исправить на этапе разработки библиотеки, а не на этапе её использования. Применяйте статический анализ кода :).


Библиотека PpluX


Краткое описание библиотеки PpluX:
Single header C++ Libraries for Thread Scheduling, Rendering, and so on...
struct DisplayList {  DisplayList& operator=(DisplayList &&d) {    data_ = d.data_;    d.data_ = nullptr;  }  ....}

Предупреждение PVS-Studio: V591 Non-void function should return a value. px_render.h 398

Библиотека Universal


Краткое описание библиотеки Universal:
The goal of Universal Numbers, or unums, is to replace IEEE floating-point with a number system that is more efficient and mathematically consistent in concurrent execution environments.
Первая ошибка

template<typename Scalar>vector<Scalar> operator*(double scalar, const vector<Scalar>& v) {  vector<Scalar> scaledVector(v);  scaledVector *= scalar;  return v;}

Предупреждение PVS-Studio: V1001 The 'scaledVector' variable is assigned but is not used by the end of the function. vector.hpp 124

Опечатка. Вместо исходного вектора v из функции нужно вернуть новый вектор scaledVector.

Аналогичную опечатку можно увидеть здесь: V1001 The 'normalizedVector' variable is assigned but is not used by the end of the function. vector.hpp 131

Вторая ошибка

template<typename Scalar>class matrix {  ....  matrix& diagonal() {  }  ....};

Предупреждение PVS-Studio: V591 Non-void function should return a value. matrix.hpp 109

Третья ошибка

template<size_t fbits, size_t abits>void module_subtract_BROKEN(  const value<fbits>& lhs, const value<fbits>& rhs, value<abits + 1>& result){  if (lhs.isinf() || rhs.isinf()) {    result.setinf();    return;  }  int lhs_scale = lhs.scale(),      rhs_scale = rhs.scale(),      scale_of_result = std::max(lhs_scale, rhs_scale);  // align the fractions  bitblock<abits> r1 =    lhs.template nshift<abits>(lhs_scale - scale_of_result + 3);  bitblock<abits> r2 =    rhs.template nshift<abits>(rhs_scale - scale_of_result + 3);  bool r1_sign = lhs.sign(), r2_sign = rhs.sign();  //bool signs_are_equal = r1_sign == r2_sign;  if (r1_sign) r1 = twos_complement(r1);  if (r1_sign) r2 = twos_complement(r2);  // <=  ....}

Предупреждение PVS-Studio: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 789, 790. value.hpp 790

Классическая ошибка, возникшая из-за Copy-Paste. Взяли и размножили строчку:

if (r1_sign) r1 = twos_complement(r1);

Поменяли в ней r1 на r2:

if (r1_sign) r2 = twos_complement(r2);

А поменять r1_sign забыли. Правильный вариант:

if (r2_sign) r2 = twos_complement(r2);

Библиотека Chobo Single-Header Libraries


Краткое описание библиотеки Chobo Single-Header Libraries:
A collection of small single-header C++11 libraries by Chobolabs.
Первая ошибка

template <typename T, typename U, typename Alloc = std::allocator<T>>class vector_view{  ....  vector_view& operator=(vector_view&& other)  {    m_vector = std::move(other.m_vector);  }  ....}

Предупреждение PVS-Studio: V591 Non-void function should return a value. vector_view.hpp 163

Вторая ошибка

template <typename UAlloc>vector_view& operator=(const std::vector<U, UAlloc>& other){  size_type n = other.size();  resize(n);  for (size_type i = 0; i < n; ++i)  {    this->at(i) = other[i];  }}

Предупреждение PVS-Studio: V591 Non-void function should return a value. vector_view.hpp 184

Библиотека PGM-index


Краткое описание библиотеки PGM-index:
The Piecewise Geometric Model index (PGM-index) is a data structure that enables fast lookup, predecessor, range searches and updates in arrays of billions of items using orders of magnitude less space than traditional indexes while providing the same worst-case query time guarantees.
Первая ошибка

char* str_from_errno(){#ifdef MSVC_COMPILER  #pragma warning(disable:4996)  return strerror(errno);#pragma warning(default:4996)#else  return strerror(errno);#endif}

Предупреждение PVS-Studio: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 9170, 9172. sdsl.hpp 9172

Неправильное временное отключение предупреждения компилятора. Подобные неаккуратности ещё как-то простительны пользовательскому коду. Но это точно недопустимо в header-only библиотеках.

Вторая ошибка

template<class t_int_vec>t_int_vec rnd_positions(uint8_t log_s, uint64_t& mask,                        uint64_t mod=0, uint64_t seed=17){  mask = (1<<log_s)-1;         // <=  t_int_vec rands(1<<log_s ,0);  set_random_bits(rands, seed);  if (mod > 0) {    util::mod(rands, mod);  }  return rands;}

Предупреждение PVS-Studio: V629 Consider inspecting the '1 << log_s' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. sdsl.hpp 1350

Один из правильных вариантов:

mask = ((uint64_t)(1)<<log_s)-1;

Библиотека Hnswlib


Краткое описание библиотеки Hnswlib:
Header-only C++ HNSW implementation with python bindings. Paper's code for the HNSW 200M SIFT experiment.
template<typename dist_t>class BruteforceSearch : public AlgorithmInterface<dist_t> {public:  BruteforceSearch(SpaceInterface <dist_t> *s, size_t maxElements) {    maxelements_ = maxElements;    data_size_ = s->get_data_size();    fstdistfunc_ = s->get_dist_func();    dist_func_param_ = s->get_dist_func_param();    size_per_element_ = data_size_ + sizeof(labeltype);    data_ = (char *) malloc(maxElements * size_per_element_);    if (data_ == nullptr)      std::runtime_error(        "Not enough memory: BruteforceSearch failed to allocate data");    cur_element_count = 0;  }  ....}

Предупреждение PVS-Studio: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); bruteforce.h 26

Забыли перед std::runtime_error написать оператор throw.

Ещё одна такая ошибка: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); bruteforce.h 161

Библиотека tiny-dnn


Краткое описание библиотеки tiny-dnn:
tiny-dnn is a C++14 implementation of deep learning. It is suitable for deep learning on limited computational resource, embedded systems and IoT devices.
Первая ошибка

class nn_error : public std::exception { public:  explicit nn_error(const std::string &msg) : msg_(msg) {}  const char *what() const throw() override { return msg_.c_str(); } private:  std::string msg_;};inline Device::Device(device_t type, const int platform_id, const int device_id)  : type_(type),    has_clcuda_api_(true),    platform_id_(platform_id),    device_id_(device_id) {  ....#else  nn_error("TinyDNN has not been compiled with OpenCL or CUDA support.");#endif}

Предупреждение PVS-Studio: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw nn_error(FOO); device.h 68

nn_error это не функция, генерирующая исключение, а просто класс. Поэтому правильно его использовать так:

throw nn_error("TinyDNN has not been compiled with OpenCL or CUDA support.");

Ещё одно неправильное использование этого класса: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw nn_error(FOO); conv2d_op_opencl.h 136

Вторая ошибка

inline std::string format_str(const char *fmt, ...) {  static char buf[2048];#ifdef _MSC_VER#pragma warning(disable : 4996)#endif  va_list args;  va_start(args, fmt);  vsnprintf(buf, sizeof(buf), fmt, args);  va_end(args);#ifdef _MSC_VER#pragma warning(default : 4996)#endif  return std::string(buf);}

Предупреждение PVS-Studio: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 139, 146. util.h 146

Библиотека Dlib


Краткое описание библиотеки Dlib:
TDLib (Telegram Database library) is a cross-platform library for building Telegram clients. It can be easily used from almost any programming language.
Первая ошибка

Ради интереса попробуйте найти эту ошибку самостоятельно.

class bdf_parser{public:  enum bdf_enums  {    NO_KEYWORD = 0,    STARTFONT = 1,    FONTBOUNDINGBOX = 2,    DWIDTH = 4,    DEFAULT_CHAR = 8,    CHARS = 16,    STARTCHAR = 32,    ENCODING = 64,    BBX = 128,    BITMAP = 256,    ENDCHAR = 512,    ENDFONT = 1024  };  ....  bool parse_header( header_info& info )  {    ....    while ( 1 )    {      res = find_keywords( find | stop );      if ( res & FONTBOUNDINGBOX )      {          in_ >> info.FBBx >> info.FBBy >> info.Xoff >> info.Yoff;          if ( in_.fail() )              return false;    // parse_error          find &= ~FONTBOUNDINGBOX;          continue;      }      if ( res & DWIDTH )      {          in_ >> info.dwx0 >> info.dwy0;          if ( in_.fail() )              return false;    // parse_error          find &= ~DWIDTH;          info.has_global_dw = true;          continue;      }      if ( res & DEFAULT_CHAR )      {          in_ >> info.default_char;          if ( in_.fail() )              return false;    // parse_error          find &= ~DEFAULT_CHAR;          continue;      }      if ( res & NO_KEYWORD )          return false;    // parse_error: unexpected EOF      break;    }  ....};

Нашли?

Растерянный Траволта-Единорог

Она здесь:

if ( res & NO_KEYWORD )

Предупреждение PVS-Studio: V616 The 'NO_KEYWORD' named constant with the value of 0 is used in the bitwise operation. fonts.cpp 288

Именованная константа NO_KEYWORD имеет значение 0. А следовательно условие не имеет смысла. Правильно было бы написать:

if ( res == NO_KEYWORD )

Ещё одна неправильная проверка находится здесь: V616 The 'NO_KEYWORD' named constant with the value of 0 is used in the bitwise operation. fonts.cpp 334

Вторая ошибка

void set(std::vector<tensor*> items){  ....  epa.emplace_back(new enable_peer_access(*g[0], *g[i]));  ....}

Предупреждение PVS-Studio: V1023 A pointer without owner is added to the 'epa' container by the 'emplace_back' method. A memory leak will occur in case of an exception. tensor_tools.h 1665

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

Третья ошибка

template <    typename detection_type,     typename label_type     >bool is_track_association_problem (  const std::vector<    std::vector<labeled_detection<detection_type,label_type> > >& samples){  if (samples.size() == 0)    return false;  unsigned long num_nonzero_elements = 0;  for (unsigned long i = 0; i < samples.size(); ++i)  {    if (samples.size() > 0)      ++num_nonzero_elements;  }  if (num_nonzero_elements < 2)    return false;  ....}

Предупреждение PVS-Studio: V547 Expression 'samples.size() > 0' is always true. svm.h 360

Это очень, очень странный код! Если запускается цикл, то значит условие (samples.size() > 0) всегда истинно. Следовательно, цикл можно упростить:

for (unsigned long i = 0; i < samples.size(); ++i){  ++num_nonzero_elements;}

После этого становится понятно, что цикл вообще не нужен. Можно написать гораздо проще и эффективнее:

unsigned long num_nonzero_elements = samples.size();

Но это ли планировалось сделать? Код явно заслуживает внимательного изучения программистом.

Четвёртая ошибка

class console_progress_indicator{  ....  double seen_first_val;  ....};bool console_progress_indicator::print_status (  double cur, bool always_print){  ....  if (!seen_first_val)  {    start_time = cur_time;    last_time = cur_time;    first_val = cur;    seen_first_val = true;  // <=    return false;  }  ....}

Предупреждение PVS-Studio: V601 The bool type is implicitly cast to the double type. console_progress_indicator.h 136

В член класса, имеющий тип double, записывают значение true. Хм

Пятая ошибка

void file::init(const std::string& name){  ....  WIN32_FIND_DATAA data;  HANDLE ffind = FindFirstFileA(state.full_name.c_str(), &data);  if (ffind == INVALID_HANDLE_VALUE ||      (data.dwFileAttributes&FILE_ATTRIBUTE_DIRECTORY) != 0)  {    throw file_not_found("Unable to find file " + name);                  }  else  {    ....  } }

Предупреждение PVS-Studio: V773 The exception was thrown without closing the file referenced by the 'ffind' handle. A resource leak is possible. dir_nav_kernel_1.cpp 60

Если найдена директория, то генерируется исключение. Но кто будет закрывать дескриптор?

Шестая ошибка

Ещё одно очень странное место.

inline double poly_min_extrap(double f0, double d0,                              double x1, double f_x1,                              double x2, double f_x2){  ....  matrix<double,2,2> m;  matrix<double,2,1> v;  const double aa2 = x2*x2;  const double aa1 = x1*x1;  m =  aa2,       -aa1,      -aa2*x2, aa1*x1;     v = f_x1 - f0 - d0*x1,      f_x2 - f0 - d0*x2;  ....}

Предупреждение PVS-Studio: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. optimization_line_search.h 211

Планируется инициализировать матрицы. Но ведь все эти aa2, f_x1, d0 и так далее это просто переменные типа double. Значит, запятые не разделяют аргументы, предназначенные для создания матриц, а являются обыкновенными comma operator, которые возвращают значение правой части.

Заключение


В начале статьи я привёл пример, как можно совместить несколько полезных дел сразу. Использование статического анализатора тоже одновременно полезно по нескольким причинам:

  • Повышение квалификации. Изучая предупреждения анализатора можно узнать много нового и полезного. Примеры: memset, #pragma warning, emplace_back, strictly aligned.
  • Выявление опечаток, ошибок и потенциальных уязвимостей на ранних этапах.
  • Код постепенно становится качественнее, проще, понятней.
  • Вы можете гордиться и всем рассказывать, что используете современные технологии при разработке проектов :). И это юмор только отчасти. Это настоящее конкурентное преимущество.

Вопрос только в том, как начать, как безболезненно внедрить и как правильно использовать? С этим вам помогут следующие статьи:



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking a Header-Only C++ Library Collection (awesome-hpp).
Подробнее..

Почему PVS-Studio не предлагает автоматические правки кода

19.11.2020 10:11:44 | Автор: admin
Почему PVS-Studio не предлагает автоматические правки кода

Статический анализатор PVS-Studio обнаруживает достаточно сложные и хитрые фрагменты кода, содержащие ошибки. И как их исправить, не всегда понятно даже человеку, и сейчас мы рассмотрим пару примеров. Поэтому лучше вообще не генерировать никаких предположений по автоматическому исправлению кода.

Иногда программисты, которые начинают пробовать PVS-Studio спрашивают: почему инструмент не предлагает автоматически исправить ошибку? Что интересно, пользователи такой вопрос уже не задают. После некоторого времени использования анализатора, им становится понятно, что для подавляющего большинства обнаруживаемых ошибок никакая автоматическая замена невозможна. По крайней мере, пока не изобретут искусственный интеллект :).

Причина в том, что PVS-Studio не является анализатором стиля кода. Он не предлагает изменения, связанные с форматированием или именованием. Не предлагает он (по крайней мере на момент написания статьи :) заменить в C++ коде все NULL на nullptr. Это хоть и хорошее предложение, но оно не имеет практически ничего общего с поиском и устранением ошибок.

PVS-Studio выявляет ошибки и потенциальные уязвимости. Многие ошибки заставляют задуматься и требуют изменения поведения программы. И только программист может решить, как исправить ту или иную ошибку.

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

Рассмотрим ошибку, которую я разбирал в статье "31 февраля".

static const int kDaysInMonth[13] = {  0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};bool ValidateDateTime(const DateTime& time) {  if (time.year < 1 || time.year > 9999 ||      time.month < 1 || time.month > 12 ||      time.day < 1 || time.day > 31 ||      time.hour < 0 || time.hour > 23 ||      time.minute < 0 || time.minute > 59 ||      time.second < 0 || time.second > 59) {    return false;  }  if (time.month == 2 && IsLeapYear(time.year)) {    return time.month <= kDaysInMonth[time.month] + 1;  } else {    return time.month <= kDaysInMonth[time.month];  }}

Анализатор понимает, что обе проверки являются истинными. Но почему, анализатору непонятно. Он ничего не знает про дни, месяцы и прочие сущности. И научить понимать такое ой как непросто. Единственное, что реально сделать это чтобы анализатор предлагал упростить функцию:

bool ValidateDateTime(const DateTime& time) {  if (time.year < 1 || time.year > 9999 ||      time.month < 1 || time.month > 12 ||      time.day < 1 || time.day > 31 ||      time.hour < 0 || time.hour > 23 ||      time.minute < 0 || time.minute > 59 ||      time.second < 0 || time.second > 59) {    return false;  }  if (time.month == 2 && IsLeapYear(time.year)) {    return true;  } else {    return true;  }}

Или, чего уж мелочиться, пусть он предложит такую автоматическую замену:

bool ValidateDateTime(const DateTime& time) {  if (time.year < 1 || time.year > 9999 ||      time.month < 1 || time.month > 12 ||      time.day < 1 || time.day > 31 ||      time.hour < 0 || time.hour > 23 ||      time.minute < 0 || time.minute > 59 ||      time.second < 0 || time.second > 59) {    return false;  }  return true;}

Прикольно, но бессмысленно ;). Анализатор убрал код, который с точки зрения языка C++ является лишним. И только человек может понять, является код действительно избыточным (а такое тоже часто бывает), или в коде допущена опечатка и надо заменить month на day.

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

В августе этого вирусного года я написал статью о проверки библиотеки PMDK. Среди прочего в статье рассматривалась ошибка неправильной защиты от переполнения:

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Раз переменная rel_wait имеет беззнаковый тип, то последующая проверка rel_wait < 0 не имеет смысла. Предупреждение PVS-Studio: V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

Кто-то воодушевился статьёй и начал массово исправлять описанные в ней ошибки: Fix various issues reported by PVS-Studio analysis.

И как же было предложено исправить код? Весьма бесхитростно: core: simplify windows timer implementation.

Неудачное исправление кода

Но код был упрощен, а не исправлен! Это заметили и началась соответствующая дискуссия: ISSUE: os_thread_windows.c get_rel_wait() will block if abstime is in the past.

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

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


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Why PVS-Studio Doesn't Offer Automatic Fixes.
Подробнее..

Устранена уязвимость в плеере VLC, допускающая удаленное выполнение кода

25.01.2021 14:08:49 | Автор: admin
Unsplash

Вышло обновление медиаплеера VLC, устраняющее внезапный сбой плеера, а также удаленное выполнение кода.

Обновление медиаплеера VLC Media Player 3.0.12 вышло не так давно. Из важного в обновлении оно улучшит пользовательский опыт для владельцев Mac. Но об этом уже рассказывали. Однако есть еще один важный момент уже в отношении информационной безопасности. Обновление исправляет критичные для безопасности проблемы.

Что за проблемы у плеера




Уязвимости обнаружил Чжэнь Чжоу из группы NSFOCUS. Они давали возможность злоумышленнику запустить удаленное выполнение кода на других компьютерах.

Речь о переполнении буфера. Причина проблемы переполнение буфера, следствие записи данных вне выделенной области памяти. Подобная проблема вызвана ошибочной работой с вводимыми данными и памятью. В итоге могут быть повреждены данные находящиеся перед или следом за буфером.

Основные проблемы


Ранее эксперты обнаружили сразу несколько проблем с безопасностью плеера.

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

Вторая. Она приводит к аварийному завершению работы медиаплеера, приводящее к отказу в обслуживании.

Согласно данным бюллетеня безопасности плеера обе ошибки были исправлены.

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

Для эксплуатации уязвимостей злоумышленнику необходимо запустить специальный файл или подключиться к специальным образом сформированному стриму. После этого развивались два сценария: плеер VLC внезапно отключается или мошенники удаленно запускают произвольный код.

Источник

Разработчики заявляют, что не обнаружили попыток эксплуатации найденных уязвимостей. Но они рекомендуют установить версию VLC Media Player 3.0.12 для Windows, macOS или Linux.

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

Немного о VLC


VLC бесплатный и один из самых популярных проигрывателей с открытым исходным кодом. Медиаплеер поддерживает большое количество форматов. VLC воспроизводит множество мультимедийных файлов и сетевые трансляции по самым разным протоколам.

Плеер разрабатывает команда проекта VideoLan. В основном это добровольцы, которые верят в силу открытого исходного кода.

Проект VideoLAN стартовал как студенческая инициатива в 1996 году во Франции. Сейчас в проекте принимают участие разработчики из 40 стран.

Подробнее..

Обработка дат притягивает ошибки или 77 дефектов в Qt 6

16.02.2021 22:10:00 | Автор: admin

PVS-Studio проверяет Qt 6
Относительно недавно состоялся релиз фреймворка Qt 6, и это стало поводом вновь проверить его с помощью PVS-Studio. В статье будут рассмотрены различные интересные ошибки, например, связанные с обработкой дат. Обнаружение всех этих ошибок хорошо демонстрирует пользу, которую может получить проект от использования таких инструментов, как PVS-Studio, особенно если они применяются регулярно.


Это классическая статья о проверке открытого проекта, которая пополнит нашу "доказательную базу" полезности и эффективности использования PVS-Studio для контроля качества кода. Хотя мы уже писали про поверку проекта Qt (в 2011, в 2014 и в 2018), очень полезно сделать это вновь. Так, мы на практике демонстрируем простую, но очень важную мысль: статический анализ должен применяться регулярно!


Наши статьи показывают, что анализатор PVS-Studio умеет находить множество разнообразнейших ошибок. И, как правило, авторы проектов быстро исправляют описанные нами ошибки. Однако всё это не имеет ничего общего с правильной и полезной методикой регулярного применения статических анализаторов. Когда анализатор встроен в процесс разработки, это позволяет быстро находить ошибки в новом или изменённом коде и тем самым их исправление обходится максимально дёшево.


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


Даты


Кажется, намечается ещё один паттерн кода, где любят собираться ошибки. Он, конечно, не такой масштабный, как функции сравнения или последние строки в однотипных блоках. Речь идёт про фрагменты кода, работающие с датами. Наверное, такой код сложно тестировать, и в итоге эти недотестированные функции будут давать некорректный результат на определённых наборах входных данных. Про пару таких случаев уже было рассказано в статье "Почему PVS-Studio не предлагает автоматические правки кода".


Встретились ошибки обработки дат и в Qt. Давайте с них и начнём.


Фрагмент N1: неправильная интерпретация статуса ошибки


Для начала нам следует посмотреть, как устроена функция, возвращающая номер месяца по его сокращённому названию.


static const char qt_shortMonthNames[][4] = {    "Jan", "Feb", "Mar", "Apr", "May", "Jun",    "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};static int fromShortMonthName(QStringView monthName){  for (unsigned int i = 0;       i < sizeof(qt_shortMonthNames) / sizeof(qt_shortMonthNames[0]); ++i)  {    if (monthName == QLatin1String(qt_shortMonthNames[i], 3))      return i + 1;  }  return -1;}

В случае успеха функция возвращает номер месяца (значение от 1 до 12). Если имя месяца некорректно, то функция возвращает отрицательное значение (-1). Обратите внимание, что функция не может вернуть значение 0.


Но там, где только что рассмотренная функция используется, программист как раз рассчитывает, что в качестве статуса ошибки ему будет возвращено нулевое значение. Фрагмент кода, некорректно использующий функцию fromShortMonthName:


QDateTime QDateTime::fromString(QStringView string, Qt::DateFormat format){  ....  month = fromShortMonthName(parts.at(1));  if (month)    day = parts.at(2).toInt(&ok);  // If failed, try day then month  if (!ok || !month || !day) {    month = fromShortMonthName(parts.at(2));    if (month) {      QStringView dayPart = parts.at(1);      if (dayPart.endsWith(u'.'))        day = dayPart.chopped(1).toInt(&ok);    }  }  ....}

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


  • V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4907
  • V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4911
  • V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4913
  • V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4921

Фрагмент N2: ошибка логики обработки даты


Для начала посмотрим на реализацию функции, возвращающей количество секунд.


enum {  ....  MSECS_PER_DAY = 86400000,  ....  SECS_PER_MIN = 60,};int QTime::second() const{    if (!isValid())        return -1;    return (ds() / 1000)%SECS_PER_MIN;}

Рассмотренная функция может вернуть значение в диапазоне [0..59] или статус ошибки -1.


В одном месте эта функция используется очень странным образом:


static qint64 qt_mktime(QDate *date, QTime *time, ....){  ....  } else if (yy == 1969 && mm == 12 && dd == 31             && time->second() == MSECS_PER_DAY - 1) {      // There was, of course, a last second in 1969, at time_t(-1); we won't      // rescue it if it's not in normalised form, and we don't know its DST      // status (unless we did already), but let's not wantonly declare it      // invalid.  } else {  ....}

Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: time->second() == MSECS_PER_DAY 1. qdatetime.cpp 2488


Согласно комментарию, если что-то пошло не так, то лучше ничего не делать. Однако, условие всегда будет ложным, поэтому всегда выполняется else-ветка.


Ошибочно вот это сравнение:


time->second() == MSECS_PER_DAY - 1

MSECS_PER_DAY 1 это 86399999. Функция second, как мы уже знаем, никак не может вернуть такое значение. Таким образом, здесь какая-то логическая ошибка и код заслуживает пристального внимания разработчиков.


Сильная сторона статических анализаторов кода в том, что они проверяют все ситуации, независимо от частоты их выполнения. Таким образом, статический анализ хорошо дополняет юнит-тесты и другие методы контроля качества кода.


Опечатки


Фрагмент N3: неожиданно, мы поговорим о HTML!


QString QPixelTool::aboutText() const{  const QList<QScreen *> screens = QGuiApplication::screens();  const QScreen *windowScreen = windowHandle()->screen();  QString result;  QTextStream str(&result);  str << "<html></head><body><h2>Qt Pixeltool</h2><p>Qt " << QT_VERSION_STR    << "</p><p>Copyright (C) 2017 The Qt Company Ltd.</p><h3>Screens</h3><ul>";  for (const QScreen *screen : screens)    str << "<li>" << (screen == windowScreen ? "* " : "  ")        << screen << "</li>";  str << "<ul></body></html>";  return result;}

Предупреждение PVS-Studio: V735 Possibly an incorrect HTML. The "</ body>" closing tag was encountered, while the "</ ul>" tag was expected. qpixeltool.cpp 707


В PVS-Studio есть диагностики, которые не только проверят сам код, но и выискивают аномалии в строковых константах. Здесь как раз сработала одна из таких диагностик. Это достаточно редкий случай, зато этим он и примечательный.


Дважды используется тег открытия списка. Это явная опечатка. Первый тег, должен открывать список, а второй закрывать. Правильный код:


str << "</ul></body></html>";

Фрагмент N4: повторная проверка в условии


class Node{  ....  bool isGroup() const { return m_nodeType == Group; }  ....};void DocBookGenerator::generateDocBookSynopsis(const Node *node){  ....  if (node->isGroup() || node->isGroup()      || node->isSharedCommentNode() || node->isModule()      || node->isJsModule() || node->isQmlModule() || node->isPageNode())    return;  ....}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the '||' operator: node->isGroup() || node->isGroup() docbookgenerator.cpp 2599


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


Фрагмент N5: создание лишней локальной переменной


void MainWindow::addToPhraseBook(){  ....  QString selectedPhraseBook;  if (phraseBookList.size() == 1) {    selectedPhraseBook = phraseBookList.at(0);    if (QMessageBox::information(this, tr("Add to phrase book"),          tr("Adding entry to phrasebook %1").arg(selectedPhraseBook),           QMessageBox::Ok | QMessageBox::Cancel, QMessageBox::Ok)                          != QMessageBox::Ok)      return;  } else {    bool okPressed = false;    QString selectedPhraseBook =       QInputDialog::getItem(this, tr("Add to phrase book"),                            tr("Select phrase book to add to"),                            phraseBookList, 0, false, &okPressed);    if (!okPressed)      return;  }  MessageItem *currentMessage = m_dataModel->messageItem(m_currentIndex);  Phrase *phrase = new Phrase(currentMessage->text(),                              currentMessage->translation(),                              QString(), nullptr);  phraseBookHash.value(selectedPhraseBook)->append(phrase);}

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


Единорог из старой коллекции


Предупреждение PVS-Studio: V561 [CWE-563] It's probably better to assign value to 'selectedPhraseBook' variable than to declare it anew. Previous declaration: mainwindow.cpp, line 1303. mainwindow.cpp 1313


В обоих ветках условного оператора формируется текст, который должен быть записан в переменную selectedPhraseBook. Имя переменной длинное, и программисту было лень писать его заново. Поэтому он скопировал имя переменной из того места, где она объявляется. При этом он чуть поспешил и вместе с именем скопировал и тип переменной:


QString selectedPhraseBook =

В результате в else-блоке появилась ещё одна локальная строковая переменная, которая инициализируется, но не используется. Тем временем переменная с таким же именем во внешнем блоке останется пустой.


Фрагмент N6: приоритет операций


Классический паттерн ошибки, который встречается весьма часто.


bool QQmlImportInstance::resolveType(....){  ....  if (int icID = containingType.lookupInlineComponentIdByName(typeStr) != -1)  {    *type_return = containingType.lookupInlineComponentById(icID);  } else {    auto icType = createICType();    ....  }  ....}

Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. qqmlimport.cpp 754


Значение переменной icID всегда будет иметь значение 0 или 1. Это явно не то, что задумывалось. Причина: в начале происходит сравнение с -1, а только затем инициализация переменной icID.


Современный синтаксис C++ позволяет корректно записать это условие следующим образом:


if (int icID = containingType.lookupInlineComponentIdByName(typeStr);    icID != -1)

Кстати, очень похожую ошибку мы уже обнаруживали в Qt:


char ch;while (i < dataLen && ((ch = data.at(i) != '\n') && ch != '\r'))  ++i;

Но пока на вооружение не будет взят такой анализатор кода, как PVS-Studio, программисты вновь и вновь будут допускать такие ошибки. Никто не совершенен. И да, это тонкий намёк внедрить PVS-Studio :).


Фрагмент N7: коварное деление по модулю


Часто бывает необходимо определить, делится число без остатка на 2 или нет. Правильный вариант это поделить по модулю два и проверить результат:


if (A % 2 == 1)

Но программисты вновь и вновь ошибаются и пишут что-то типа этого:


if (A % 1 == 1)

Это естественно неправильно, так как остаток от деления по модулю на один это всегда ноль. Не обошлось без этой ошибки и в Qt:


bool loadQM(Translator &translator, QIODevice &dev, ConversionData &cd){  ....  case Tag_Translation: {    int len = read32(m);    if (len % 1) {                                             // <=      cd.appendError(QLatin1String("QM-Format error"));      return false;    }    m += 4;    QString str = QString((const QChar *)m, len/2);  ....}

Предупреждение PVS-Studio: V1063 The modulo by 1 operation is meaningless. The result will always be zero. qm.cpp 549


Фрагмент N8: перезаписывание значения


QString Node::qualifyQmlName(){  QString qualifiedName = m_name;  if (m_name.startsWith(QLatin1String("QML:")))    qualifiedName = m_name.mid(4);  qualifiedName = logicalModuleName() + "::" + m_name;  return qualifiedName;}

Предупреждение PVS-Studio: V519 [CWE-563] The 'qualifiedName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1227, 1228. node.cpp 1228


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


QString qualifiedName = m_name;if (m_name.startsWith(QLatin1String("QML:")))  qualifiedName = m_name.mid(4);qualifiedName = logicalModuleName() + "::" + qualifiedName;return qualifiedName;

Фрагмент N9: copy-paste


class Q_CORE_EXPORT QJsonObject{  ....  bool operator<(const iterator& other) const  { Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }  bool operator<=(const iterator& other) const  { Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }  ....}

Прудпреждение PVS-Studio: V524 It is odd that the body of '<=' function is fully equivalent to the body of '<' function. qjsonobject.h 155


Такие скучные функции, как операторы сравнения, никто не проверяет. На них обычно не пишут тесты. Их не просматривают или делают это очень быстро на code review. А зря. Статический анализ кода здесь как нельзя кстати. Анализатор не устаёт и с удовольствием проверяет даже такой скучный код.


Реализация операторов < и <= совпадают. Это явно неправильно. Скорее всего, код писался методом Copy-Paste, и затем забыли изменить всё что нужно в скопированном коде. Правильно:


bool operator<(const iterator& other) const{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }bool operator<=(const iterator& other) const{ Q_ASSERT(item.o == other.item.o); return item.index <= other.item.index; }

Фрагмент N10: static_cast / dynamic_cast


void QSGSoftwareRenderThread::syncAndRender(){  ....  bool canRender = wd->renderer != nullptr;  if (canRender) {     auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);     if (softwareRenderer)       softwareRenderer->setBackingStore(backingStore);  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'softwareRenderer' is always true. qsgsoftwarethreadedrenderloop.cpp 510


В начале рассмотрим вот эту проверку:


bool canRender = wd->renderer != nullptr;if (canRender) {

Благодаря ей можно быть уверенным, что внутри тела условного оператора значение указателя wd->renderer всегда точно ненулевое. Тогда непонятно, что же хотят проверить следующим кодом?


auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);if (softwareRenderer)

Если указатель wd->renderer ненулевой, то и указатель softwareRenderer точно ненулевой. Есть подозрение, что здесь опечатка, которая состоит в том, что на самом деле следовало использовать dynamic_cast. В этом случае код начинает приобретать смысл. Если преобразование типа невозможно, оператор dynamic_cast возвращает nullptr, и это возвращенное значение, естественно, следует проверять. Впрочем, возможно, я неправильно интерпретировал ситуацию и код нужно исправлять другим способом.


Фрагмент N11: скопировали блок кода и забыли изменить


void *QQuickPath::qt_metacast(const char *_clname){  if (!_clname) return nullptr;  if (!strcmp(_clname, qt_meta_stringdata_QQuickPath.stringdata0))    return static_cast<void*>(this);  if (!strcmp(_clname, "QQmlParserStatus"))    return static_cast< QQmlParserStatus*>(this);  if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))   // <=    return static_cast< QQmlParserStatus*>(this);  if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))   // <=    return static_cast< QQmlParserStatus*>(this);  return QObject::qt_metacast(_clname);}

Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2719, 2721. moc_qquickpath_p.cpp 2721


Эти две строчки:


if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))  return static_cast< QQmlParserStatus*>(this);

Были размножены с помощью Copy-Paste. После чего они не были модифицированы и не имеют смысла.


Фрагмент N12: переполнение из-за не там поставленной скобки


int m_offsetFromUtc;....void QDateTime::setMSecsSinceEpoch(qint64 msecs){  ....  if (!add_overflow(msecs, qint64(d->m_offsetFromUtc * 1000), &msecs))    status |= QDateTimePrivate::ValidWhenMask;  ....}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands of the 'd->m_offsetFromUtc * 1000' operator to the 'qint64' type, not the result. qdatetime.cpp 3922


Программист предвидит ситуацию, что при умножении переменной типа int на 1000 может произойти переполнение. Чтобы этого избежать, он планирует использовать при умножении 64-битный тип qint64. И использует явное приведение типа.


Вот только толку от этого приведения типа никакого нет. В начале всё равно произойдёт переполнение. И только затем выполнится приведение типа. Правильный вариант:


add_overflow(msecs, qint64(d->m_offsetFromUtc) * 1000, &msecs)

Фрагмент N13: не полностью инициализированный массив


class QPathEdge{  ....private:  int m_next[2][2];  ....};inline QPathEdge::QPathEdge(int a, int b)    : flag(0)    , windingA(0)    , windingB(0)    , first(a)    , second(b)    , angle(0)    , invAngle(0){    m_next[0][0] = -1;    m_next[1][0] = -1;    m_next[0][0] = -1;    m_next[1][0] = -1;}

Предупреждения PVS-Studio:


  • V1048 [CWE-1164] The 'm_next[0][0]' variable was assigned the same value. qpathclipper_p.h 301
  • V1048 [CWE-1164] The 'm_next[1][0]' variable was assigned the same value. qpathclipper_p.h 302

Перед нами неудачная попытка инициализировать массив размером 2x2. Два элемента инициализируются повторно, и два остаются неинициализированными. Правильный вариант:


m_next[0][0] = -1;m_next[0][1] = -1;m_next[1][0] = -1;m_next[1][1] = -1;

Я очень люблю такие примеры ошибок, которые встречаются в коде профессиональных разработчиков. Это как раз такой случай. Он показывает, что любой может опечататься, и поэтому статический анализ является вашим другом. Дело в том, что я уже десяток лет веду бой со скептиками, которые уверены, что такие ошибки можно встретить только в лабораторных работах студентов и что они такие ошибки никогда не делают :). Ещё 10 лет назад я написал заметку "Миф второй профессиональные разработчики не допускают глупых ошибок", и с тех пор, естественно, ничего не изменилось. Люди всё также делают такие ошибки и всё также утверждают, что это не так :).


Будь мудр - используй статический анализатор кода


Ошибки в логике


Фрагмент N14: недостижимый код


void QmlProfilerApplication::tryToConnect(){  Q_ASSERT(!m_connection->isConnected());  ++ m_connectionAttempts;  if (!m_verbose && !(m_connectionAttempts % 5)) {// print every 5 seconds    if (m_verbose) {      if (m_socketFile.isEmpty())        logError(          QString::fromLatin1("Could not connect to %1:%2 for %3 seconds ...")          .arg(m_hostName).arg(m_port).arg(m_connectionAttempts));      else        logError(          QString::fromLatin1("No connection received on %1 for %2 seconds ...")          .arg(m_socketFile).arg(m_connectionAttempts));    }  }  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'm_verbose' is always false. qmlprofilerapplication.cpp 495


Этот код никогда ничего не запишет в лог. Причиной являются противоположные условия:


if (!m_verbose && ....) {  if (m_verbose) {

Фрагмент N15: перетирание значения переменной


void QRollEffect::scroll(){  ....  if (currentHeight != totalHeight) {      currentHeight = totalHeight * (elapsed/duration)          + (2 * totalHeight * (elapsed%duration) + duration)          / (2 * duration);      // equiv. to int((totalHeight*elapsed) / duration + 0.5)      done = (currentHeight >= totalHeight);  }  done = (currentHeight >= totalHeight) &&         (currentWidth >= totalWidth);  ....}

Предупреждение PVS-Studio: V519 [CWE-563] The 'done' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 509, 511. qeffects.cpp 511


Весь условный оператор не имеет смысла, так как значение переменной done всё равно тут же перезаписывается. Возможно, здесь не хватает ключевого слова else.


Фрагмент N16-N20: перетирание значения переменной


Альтернативный вариант перетирания значения переменной.


bool QXmlStreamWriterPrivate::finishStartElement(bool contents){  ....  if (inEmptyElement) {    ....    lastNamespaceDeclaration = tag.namespaceDeclarationsSize;   // <=    lastWasStartElement = false;  } else {    write(">");  }  inStartElement = inEmptyElement = false;  lastNamespaceDeclaration = namespaceDeclarations.size();      // <=  return hadSomethingWritten;}

Предупреждение PVS-Studio: V519 [CWE-563] The 'lastNamespaceDeclaration' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3030, 3036. qxmlstream.cpp 3036


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


Есть ещё 4 предупреждения, указывающих на такой же паттерн ошибки:


  • V519 [CWE-563] The 'last' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 609, 637. qtextengine.cpp 637
  • V519 [CWE-563] The 'm_dirty' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1014, 1017. qquickshadereffect.cpp 1017
  • V519 [CWE-563] The 'changed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 122, 128. qsgdefaultspritenode.cpp 128
  • V519 [CWE-563] The 'eaten' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 299, 301. qdesigner.cpp 301

Фрагмент N21: путаница между нулевым указателем и пустой строкой


// this could become a list of all languages used for each writing// system, instead of using the single most common language.static const char languageForWritingSystem[][6] = {    "",     // Any    "en",  // Latin    "el",  // Greek    "ru",  // Cyrillic    ...... // Нулевых указателей нет. Используются пустые строковые литералы.    "", // Symbol    "sga", // Ogham    "non", // Runic    "man" // N'Ko};static void populateFromPattern(....){  ....  for (int j = 1; j < QFontDatabase::WritingSystemsCount; ++j) {    const FcChar8 *lang = (const FcChar8*) languageForWritingSystem[j];    if (lang) {  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'lang' is always true. qfontconfigdatabase.cpp 462


В массиве languageForWritingSystem нет нулевых указателей. Поэтому проверка if(lang) не имеет смысла. Зато в массиве есть пустые строки. Быть может, хотелось сделать проверку именно на пустую строку? Если да, тогда корректный код должен выглядеть так:


if (strlen(lang) != 0) {

Или можно ещё проще написать:


if (lang[0] != '\0') {

Фрагмент N22: странная проверка


bool QNativeSocketEnginePrivate::createNewSocket(....){  ....  int socket = qt_safe_socket(domain, type, protocol, O_NONBLOCK);  ....  if (socket < 0) {    ....    return false;  }  socketDescriptor = socket;  if (socket != -1) {    this->socketProtocol = socketProtocol;    this->socketType = socketType;  }  return true;}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'socket != 1' is always true. qnativesocketengine_unix.cpp 315


Условие socket != -1 всегда истинно, так как выше происходит выход из функции, если значение переменной socket отрицательное.


Фрагмент N23: так что же всё-таки должна вернуть функция?


bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent){  Q_D(QSqlTableModel);  if (parent.isValid() || row < 0 || count <= 0)    return false;  else if (row + count > rowCount())    return false;  else if (!count)    return true;  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression '!count' is always false. qsqltablemodel.cpp 1110


Для упрощения выделю самое главное:


if (.... || count <= 0)  return false;....else if (!count)  return true;

Первая проверка говорит нам, что если значение count меньше или равно 0, то это ошибочное состояние и функция должна вернуть false. Однако ниже мы видим точное сравнение этой переменной с нулём, и этот случай уже интерпретируется по-другому: функция должна вернуть true.


Здесь явно что-то не так. Я подозреваю, что на самом деле проверка должна быть не <=, а просто <. Тогда код обретает смысл:


bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent){  Q_D(QSqlTableModel);  if (parent.isValid() || row < 0 || count < 0)    return false;  else if (row + count > rowCount())    return false;  else if (!count)    return true;  ....}

Фрагмент N24: лишний статус?


В следующем коде переменная identifierWithEscapeChars выглядит просто как лишняя сущность. Или это логическая ошибка? Или код не дописан? К моменту второй проверки эта переменная в любом случае всегда будет равна true.


int Lexer::scanToken(){  ....  bool identifierWithEscapeChars = false;  ....  if (!identifierWithEscapeChars) {    identifierWithEscapeChars = true;    ....  }  ....  if (identifierWithEscapeChars) {    // <=    ....  }  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'identifierWithEscapeChars' is always true. qqmljslexer.cpp 817


Фрагмент N25: что делать с девятью объектами?


bool QFont::fromString(const QString &descrip){  ....  const int count = l.count();  if (!count || (count > 2 && count < 9) || count == 9 || count > 17 ||      l.first().isEmpty()) {    qWarning("QFont::fromString: Invalid description '%s'",             descrip.isEmpty() ? "(empty)" : descrip.toLatin1().data());    return false;  }  setFamily(l[0].toString());  if (count > 1 && l[1].toDouble() > 0.0)    setPointSizeF(l[1].toDouble());  if (count == 9) {                           // <=    setStyleHint((StyleHint) l[2].toInt());    setWeight(QFont::Weight(l[3].toInt()));    setItalic(l[4].toInt());    setUnderline(l[5].toInt());    setStrikeOut(l[6].toInt());    setFixedPitch(l[7].toInt());  } else if (count >= 10) {  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'count == 9' is always false. qfont.cpp 2142


Как должна себя вести функция, если переменная count равна 9? С одной стороны, функция должна выдать предупреждение и завершить свою работу. Ведь явно написано:


if (.... || count == 9 || ....) {  qWarning(....);  return false;}

С другой стороны, для 9 объектов предусмотрено выполнение специального кода:


if (count == 9) {  setStyleHint((StyleHint) l[2].toInt());  setWeight(QFont::Weight(l[3].toInt()));  setItalic(l[4].toInt());  ....}

Этот код, конечно, никогда не выполняется. Код ждёт, чтобы его пришли и исправили :).


Нулевые указатели


Фрагмент N26-N42: использование указателя до его проверки


class __attribute__((visibility("default"))) QMetaType {  ....  const QtPrivate::QMetaTypeInterface *d_ptr = nullptr;};QPartialOrdering QMetaType::compare(const void *lhs, const void *rhs) const{    if (!lhs || !rhs)        return QPartialOrdering::Unordered;    if (d_ptr->flags & QMetaType::IsPointer)        return threeWayCompare(*reinterpret_cast<const void * const *>(lhs),                               *reinterpret_cast<const void * const *>(rhs));    if (d_ptr && d_ptr->lessThan) {        if (d_ptr->equals && d_ptr->equals(d_ptr, lhs, rhs))            return QPartialOrdering::Equivalent;        if (d_ptr->lessThan(d_ptr, lhs, rhs))            return QPartialOrdering::Less;        if (d_ptr->lessThan(d_ptr, rhs, lhs))            return QPartialOrdering::Greater;        if (!d_ptr->equals)            return QPartialOrdering::Equivalent;    }    return QPartialOrdering::Unordered;}

Предупреждение PVS-Studio: V595 [CWE-476] The 'd_ptr' pointer was utilized before it was verified against nullptr. Check lines: 710, 713. qmetatype.cpp 710


Ошибка на первый взгляд может быть не заметна. Но на самом деле всё просто. Проследим, как работают с указателем d_ptr:


if (d_ptr->flags & ....)if (d_ptr && ....)

Вначале выполняется условие, в котором указатель разыменовывается. И только из следующей проверки становится ясно, что этот указатель, оказывается, может быть нулевой.


Это один из самых распространённых паттернов ошибки в языке C и С++. Пруфы. В исходных Qt кодах тоже встречается немало ошибок этой разновидности:


  • V595 [CWE-476] The 'self' pointer was utilized before it was verified against nullptr. Check lines: 1346, 1351. qcoreapplication.cpp 1346
  • V595 [CWE-476] The 'currentTimerInfo' pointer was utilized before it was verified against nullptr. Check lines: 636, 641. qtimerinfo_unix.cpp 636
  • V595 [CWE-476] The 'lib' pointer was utilized before it was verified against nullptr. Check lines: 325, 333. qlibrary.cpp 325
  • V595 [CWE-476] The 'fragment.d' pointer was utilized before it was verified against nullptr. Check lines: 2262, 2266. qtextcursor.cpp 2262
  • V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1581, 1583. qapplication.cpp 1581
  • V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1593, 1595. qapplication.cpp 1593
  • V595 [CWE-476] The 'newHandle' pointer was utilized before it was verified against nullptr. Check lines: 873, 879. qsplitter.cpp 873
  • V595 [CWE-476] The 'targetModel' pointer was utilized before it was verified against nullptr. Check lines: 454, 455. qqmllistmodel.cpp 454
  • V595 [CWE-476] The 'childIface' pointer was utilized before it was verified against nullptr. Check lines: 102, 104. qaccessiblequickitem.cpp 102
  • V595 [CWE-476] The 'e' pointer was utilized before it was verified against nullptr. Check lines: 94, 98. qquickwindowmodule.cpp 94
  • V595 [CWE-476] The 'm_texture' pointer was utilized before it was verified against nullptr. Check lines: 235, 239. qsgplaintexture.cpp 235
  • V595 [CWE-476] The 'm_unreferencedPixmaps' pointer was utilized before it was verified against nullptr. Check lines: 1140, 1148. qquickpixmapcache.cpp 1140
  • V595 [CWE-476] The 'camera' pointer was utilized before it was verified against nullptr. Check lines: 263, 264. assimpimporter.cpp 263
  • V595 [CWE-476] The 'light' pointer was utilized before it was verified against nullptr. Check lines: 273, 274. assimpimporter.cpp 273
  • V595 [CWE-476] The 'channel' pointer was utilized before it was verified against nullptr. Check lines: 337, 338. assimpimporter.cpp 337
  • V595 [CWE-476] The 'm_fwb' pointer was utilized before it was verified against nullptr. Check lines: 2492, 2500. designerpropertymanager.cpp 2492

Фрагмент N43: использование указателя до его проверки в рамках одного выражения


Та же самая ситуация, что и выше. Однако в этот раз разыменование и проверка указателя находятся в одном выражении. Классическая ошибка, возникающая из-за невнимательности при написании кода и при последующем code-review.


void QFormLayoutPrivate::updateSizes(){  ....  QFormLayoutItem *field = m_matrix(i, 1);  ....  if (userHSpacing < 0 && !wrapAllRows && (label || !field->fullRow) && field)  ....}

Предупреждение PVS-Studio: V713 [CWE-476] The pointer 'field' was utilized in the logical expression before it was verified against nullptr in the same logical expression. qformlayout.cpp 405


Минутка отдыха


Я устал. Думаю, устали и читатели. Тут устанешь, даже если просто бегло просматривать текст статьи :). Поэтому я пошёл за второй чашечкой кофе. Первую я выпил где-то на рубеже 12-ого фрагмента. Приглашаю и читателей сходить за любимым напитком.


И пока все ушли на кухню, рекламная пауза. Приглашаю команду, занимающуюся разработкой проекта Qt рассмотреть вопрос приобретения лицензии на анализатор кода PVS-Studio. Запросить прайс можно здесь. С нашей стороны поддержка и помощь в настройке. Да, согласен, я сегодня более навязчив, чем обычно. Это эксперимент :).


Минутка отдыха с единорогом


Фрагмент N44-N72: нет проверки, что вернула функция malloc


void assignData(const QQmlProfilerEvent &other){  if (m_dataType & External) {    uint length = m_dataLength * (other.m_dataType / 8);    m_data.external = malloc(length);                          // <=    memcpy(m_data.external, other.m_data.external, length);    // <=  } else {    memcpy(&m_data, &other.m_data, sizeof(m_data));  }}

Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 277, 276. qqmlprofilerevent_p.h 277


Нельзя просто взять и использовать указатель, который вернула функция malloc. Нужно обязательно проверить, не является ли этот указатель нулевым, даже если очень лень всем этим заниматься. Для этого есть 4 причины, которые описаны в статье "Почему важно проверять, что вернула функция malloc".


Перед нами один из случаев, где отсутствует необходимая проверка. Есть и другие предупреждения, но из-за их количества включать весь список в статью не хочется. На всякий случай, я выписал 28 предупреждений в файл: qt6-malloc.txt. Но на самом деле разработчикам, конечно, лучше самим перепроверить проект и самостоятельно изучить предупреждения. У меня не было задачи выявить как можно больше ошибок.


Что интересно, на фоне важных забытых проверок есть совершенно ненужные. Речь идёт о вызове оператора new, который в случае ошибки выделения памяти сгенерирует исключение std::bad_alloc. Вот один из примеров такой избыточной проверки:


static QImageScaleInfo* QImageScale::qimageCalcScaleInfo(....){  ....  QImageScaleInfo *isi;  ....  isi = new QImageScaleInfo;  if (!isi)    return nullptr;  ....}

Предупреждение PVS-Studio: V668 [CWE-570] There is no sense in testing the 'isi' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qimagescale.cpp 245


P.S. Здесь читатели всегда задают вопрос, учитывает ли анализатор placement new или "new (std::nothrow) T"? Да, учитывает и не выдаёт для них ложные срабатывания.


Избыточный код ("код с запахом")


Встречаются ситуации, когда анализатор выдаёт предупреждения на код, который является корректным, но избыточным. Например, есть повторная проверка одной и той же переменной. При этом даже затруднительно сказать, ложное это срабатывание или нет. Формально анализатор прав, но и настоящую ошибку он не нашёл.


Пожалуй, можно сказать, что это "код с запахом". Раз этот код смущает анализатор, он может смущать и программистов, усложняя поддержку такого кода. Приходится тратить больше времени, чтобы понять, что происходит.


Часто я вообще не рассматриваю в статьях такие предупреждения. Это неинтересно. Однако в проекте Qt вдруг таких случаев оказалось удивительно много. Явно больше, чем обычно. Поэтому я решил уделить этому немного внимания и разобрать несколько таких случаев. Думаю, будет полезно провести рефакторинг этих и многих других аналогичных мест. Для этого потребуется использовать полный отчёт, не руководствоваться тем, что я выпишу в статью.


Итак, взглянем на несколько показательных случаев.


Фрагмент N73: "код с запахом" обратная проверка


void QQuick3DSceneManager::setWindow(QQuickWindow *window){  if (window == m_window)    return;  if (window != m_window) {    if (m_window)      disconnect(....);    m_window = window;    connect(....);    emit windowChanged();  }}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'window != m_window' is always true. qquick3dscenemanager.cpp 60


Если window==m_window, то функция завершает работу. Последующая обратная проверка не имеет никакого смысла и только загромождает код.


Фрагмент N74: "код с запахом" странная инициализация


QModelIndex QTreeView::moveCursor(....){  ....  int vi = -1;  if (vi < 0)    vi = qMax(0, d->viewIndex(current));  ....}

Предупреждение PVS-Stduio: V547 [CWE-571] Expression 'vi < 0' is always true. qtreeview.cpp 2219


Что это? Зачем так писать?


Что это? Зачем так писать? Код можно упростить до одной строки:


int vi = qMax(0, d->viewIndex(current));

Фрагмент N75: "код с запахом" недостижимый код


bool copyQtFiles(Options *options){  ....  if (unmetDependencies.isEmpty()) {    if (options->verbose) {      fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",              qPrintable(sourceFileName));    }  } else {    if (unmetDependencies.isEmpty()) {      if (options->verbose) {        fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",                  qPrintable(sourceFileName));      }    } else {      fprintf(stdout, "  -- Skipping %s. It has unmet dependencies: %s.\n",              qPrintable(sourceFileName),              qPrintable(unmetDependencies.join(QLatin1Char(','))));    }  }  ....}

Предупреждение PVS-Studio: V571 [CWE-571] Recurring check. The 'if (unmetDependencies.isEmpty())' condition was already verified in line 2203. main.cpp 2209


На первый взгляд перед нами респектабельный код, формирующий подсказку. Но давайте приглядимся. Если первый раз условие unmetDependencies.isEmpty() выполнилось, то второй раз этого уже не произойдёт. Это нестрашно, так как автор планировал вывести то же самое сообщение. Настоящей ошибки нет, но код переусложнён. Он может быть упрощен до следующего варианта:


bool copyQtFiles(Options *options){  ....  if (unmetDependencies.isEmpty()) {    if (options->verbose) {      fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",              qPrintable(sourceFileName));    }  } else {    fprintf(stdout, "  -- Skipping %s. It has unmet dependencies: %s.\n",            qPrintable(sourceFileName),            qPrintable(unmetDependencies.join(QLatin1Char(','))));  }  ....}

Фрагмент N76: "код с запахом" сложный тернарный оператор


bool QDockAreaLayoutInfo::insertGap(....){  ....  QDockAreaLayoutItem new_item    = widgetItem == nullptr      ? QDockAreaLayoutItem(subinfo)      : widgetItem ? QDockAreaLayoutItem(widgetItem)                    : QDockAreaLayoutItem(placeHolderItem);  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'widgetItem' is always true. qdockarealayout.cpp 1167


Возможно, здесь мы имеем дело с настоящей ошибкой. Но я больше склоняюсь к тому, что этот код несколько раз менялся и в результате получился бессмысленно сложным (избыточным). Его можно сократить до:


  QDockAreaLayoutItem new_item    = widgetItem == nullptr      ? QDockAreaLayoutItem(subinfo) : QDockAreaLayoutItem(widgetItem);

Фрагмент N77: "код с запахом" избыточная защита


typedef unsigned int uint;ReturnedValue TypedArrayCtor::virtualCallAsConstructor(....){  ....  qint64 l = argc ? argv[0].toIndex() : 0;  if (scope.engine->hasException)    return Encode::undefined();  // ### lift UINT_MAX restriction  if (l < 0 || l > UINT_MAX)    return scope.engine->throwRangeError(QLatin1String("Index out of range."));  uint len = (uint)l;  if (l != len)    scope.engine->throwRangeError(      QStringLiteral("Non integer length for typed array."));  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'l != len' is always false. qv4typedarray.cpp 306


Кто-то очень переживает, что значение 64-битной переменной не вмещается в 32-битную переменную unsigned. И использует сразу две проверки корректности. При этом вторая проверка избыточна.


Вот этого условия более чем достаточно:


if (l < 0 || l > UINT_MAX)

Приведенный ниже фрагмент можно смело удалить, и программа менее надёжной не станет:


uint len = (uint)l;if (l != len)  scope.engine->throwRangeError(    QStringLiteral("Non integer length for typed array."));

Дальше продолжать не буду. Думаю, идею вы поняли.


Здесь можно сделать маленький вывод: результатом использования анализатора PVS-Studio будет не только устранение ошибок, но и упрощение кода.


Другие ошибки


Я остановился после того, как описал 77 дефектов. Это красивое число и выписанного более чем достаточно, чтобы написать статью. Однако это не значит, что нет других ошибок, которые способен выявить PVS-Studio. При изучении лога я был весьма поверхностен и пропускал всё, где нужно было разбираться более пары минут, ошибка это или нет :).


Поэтому я всегда прошу не полагаться на ошибки, которые мы описываем в наших статьях, а выполнять анализ проектов самостоятельно.


Заключение


Статический анализ это круто! После внедрения PVS-Studio будет экономить время и нервы, выявляя множество ошибок сразу после написания кода. Намного лучше искать на code review с коллегами не опечатки, а высокоуровневые ошибки и обсуждать эффективность реализованного алгоритма. Тем более, как показывает практика, эти дурацкие опечатки всё равно отлично прячутся при просмотре кода глазами. Так что пусть их лучше ищет программа, а не человек.


Если у вас ещё остались вопросы или возражения, приглашаю познакомиться со статьёй "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio". С вероятность 90 % вы найдете в ней ответ на ваши вопросы :). В оставшихся 10 % случаев напишите нам, пообщаемся :).


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Date Processing Attracts Bugs or 77 Defects in Qt 6.

Подробнее..

Пример, как в PVS-Studio появляются новые диагностики

20.03.2021 18:12:42 | Автор: admin

Новая Си++ диагностика для PVS-Studio
Пользователи иногда спрашивают, как появляются новые диагностики в статическом анализаторе PVS-Studio. Мы отвечаем, что черпаем вдохновение из разнообразнейших источников: книг, стандартов кодирования, собственных ошибок, писем наших пользователей и так далее. Сегодня мы придумали новую интересную диагностику и решили рассказать историю, как это произошло.


Всё началось с проверки проекта COVID-19 CovidSim Model и статьи про неинициализированную переменную. Проект оказался маленьким и написанным с использованием современного стандарта языка C++. Это значит, что он отлично может пополнить базу тестовых проектов для регрессионного тестирования ядра анализатора PVS-Studio.


Однако, прежде чем пополнить базу новым проектом, полезно просмотреть выдаваемые предупреждения, чтобы выявить новые закономерности (паттерны) ложных срабатываний и выписать их для дальнейшей доработки анализатора. Также это дополнительная возможность заметить, что ещё что-то не так. Например, сообщение неудачно описывает ошибку для определённой конструкции кода.


Программист, которому поручили добавить проект в тестовую базу, подошёл к задаче основательно и решил заглянуть в раздел MISRA-диагностик. Вообще, это делать было необязательно, так как это весьма специфичная группа диагностик, которую нет смысла включать для таких проектов, как CovidSim.


MISRA С и MISRA C++ диагностики предназначены для разработчиков встраиваемых систем, и их суть сводится к ограничению использования небезопасных конструкций программирования. Например, не рекомендуется применять оператор goto (V2502), так как он провоцирует создание сложного кода, в котором легко допустить логическую ошибку. Подробнее с философией стандарта кодирования MISRA можно познакомиться в статье "Что такое MISRA и как её готовить".


Для прикладного программного обеспечения, которым как раз и является проект CovidSim, включать набор MISRA диагностик не имеет смысла. Пользователь просто утонет в огромном количестве малополезных для него сообщений. Например, экспериментируя с этим набором диагностик, мы получали более миллиона предупреждений для некоторых открытых проектов среднего размера. Грубо говоря, с точки зрения MISRA, в каждой третьей строчке кода может быть что-то не так :). Естественно, никто всё это смотреть и тем более править не будет. Проект или сразу разрабатывается с учётом MISRA рекомендаций, или для него этот стандарт кодирования неактуален.


Но мы отвлеклись от темы. Так вот, бегло просматривая MISRA предупреждения, коллега зацепился взглядом за предупреждение V2507, выданное для этого фрагмента кода.


if (radiusSquared > StateT[tn].maxRad2) StateT[tn].maxRad2 = radiusSquared;{  SusceptibleToLatent(a->pcell);  if (a->listpos < Cells[a->pcell].S)  {    UpdateCell(Cells[a->pcell].susceptible, a->listpos, Cells[a->pcell].S);    a->listpos = Cells[a->pcell].S;    Cells[a->pcell].latent[0] = ai;  }}StateT[tn].cumI_keyworker[a->keyworker]++;

Правило V2507 заставляет оборачивать тела условных операторов в фигурные скобки.


В первый момент наш дотошный коллега подумал, что анализатор дал сбой. Ведь здесь есть блок текста в фигурных скобках! Перед нами ложное срабатывание?


Давайте присмотримся. Код только кажется корректным, но таковым не является! Фигурные скобки не относятся к оператору if.


Отформатируем код для наглядности:


if (radiusSquared > StateT[tn].maxRad2)  StateT[tn].maxRad2 = radiusSquared;{  SusceptibleToLatent(a->pcell);  ....}

Согласитесь, это красивый баг. Он наверняка войдёт в Top10 C++ ошибок, найденных нами в 2021 году.


И что из этого следует? Подход, диктуемый стандартом MISRA, работает! Да, он заставляет писать везде фигурные скобки. Да, это утомительно. Но это оправданная плата за повышение надёжности встроенных приложений, используемых в медицинской технике, автомобилях и других системах высокой ответственности.


Хорошо, у разработчиков, использующих стандарт MISRA, всё нормально. Однако рекомендовать всем подряд использовать фигурные скобки это плохая идея. Используя такой подход, очень легко довести анализатор до состояния, когда им станет невозможно пользоваться. Будет так много сообщений, что их просто никто не будет смотреть.


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


Выдать предупреждение, если для оператора if выполняются следующие условия:


  • весь условный оператор if записан в одну строчку и имеет только then-ветку;
  • следующий statement после if это compound statement, и он находится не на той же строке, что и if.

Заранее можно прогнозировать, что получится хорошее правило с низким количеством ложных срабатываний.


Именно так эта идея сейчас оформлена в нашей системе учёта задач. Возможно, в процессе реализации что-то будет сделано по-другому, но это уже неважно. Главное, появится хорошее диагностическое правило, которое начнёт выявлять новый паттерн ошибки. Далее мы распространим его на C# и Java ядра анализатора PVS-Studio.


Только что мы рассмотрели интересный пример, как было сформулировано новое диагностическое правило, которое затем будет реализовано в PVS-Studio. Скажем спасибо проекту CovidSim, стандарту кодирования MISRA и наблюдательности нашего коллеги.


Спасибо за внимание и следуйте за мной в мир С++ и багов :). Twitter. Facebook.


Дополнительный ссылки:


  1. Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей.
  2. Под капотом PVS-Studio для Java: разработка диагностик.
  3. Использование машинного обучения в статическом анализе исходного кода программ.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Example of How New Diagnostics Appear in PVS-Studio.

Подробнее..

Перевод Как мы устранили редкую ошибку, из-за которой пришлось разлогинить всех пользователей Github

24.03.2021 10:15:26 | Автор: admin

8 марта мы рассказали о том, что из соображений безопасности разлогинили всех пользователей GitHub.com из-за редкой уязвимости защиты. Мы считаем, что прозрачность ключевой фактор сохранения доверия со стороны пользователей, поэтому решили рассказать об этом баге подробнее. В этом посте мы изложим технические детали данной уязвимости и объясним, как она возникла, как мы на неё отреагировали и какие мы предприняли шаги для того, чтобы подобное не повторилось.

Отчёты пользователей


2 марта 2021 через службу технической поддержки мы получили отчёт пользователя, который войдя на GitHub.com под собственными идентификационными данными, внезапно авторизировался как другой пользователь. Он немедленно вышел из аккаунта, но сообщил о проблеме нам, поскольку она обеспокоила его, и на то были все основания.

Мы привели в действие свои процедуры реагирования на инциденты с безопасностью и немедленно изучили этот отчёт, призвав на помощь специалистов компании из отделов безопасности, разработки и поддержки пользователей. Спустя несколько часов после первого отчёта мы получили второй отчёт от другого пользователя с очень похожей ситуацией. После анализа логов запросов и аудита мы смогли подтвердить истинность полученных внешних отчётов сессия пользователя действительно внезапно оказалась общей для двух IP-адресов примерно во время отправления отчётов.

Исследование недавних изменений в инфраструктуре


Учитывая то, что этот баг был новым поведением, мы сразу же заподозрили, что он связан с недавним изменением в нашей инфраструктуре, и начали поиск решения с анализа изменений. Недавно мы обновили компоненты на уровне системы, занимающемся балансировкой нагрузки и маршрутизацией. Мы выяснили, что устранили проблему с keepalive-сообщениями HTTP, которая, по нашему мнению, могла быть связана с указанной в отчётах проблемой.

После исследования этих изменений мы выяснили, что первопричиной были не они. Запросы пользователей, которые отправили нам отчёты, на уровне балансировки нагрузки следовали по совершенно другому пути, задействуя другие машины. Мы исключили возможность того, что ответы заменялись на этом уровне. Исключив из возможных первопричин недавние инфраструктурные изменения и будучи уверенными, что проблема присутствует не в слоях связи и протоколов, мы перешли к другим потенциальным причинам.

Исследование недавних изменений в коде


Благодаря тому, что мы начали расследование с недавних изменений в инфраструктуре, нам удалось выяснить, что запросы, приводившие к возврату неправильной сессии, обрабатывались на одной и той же машине и в одном процессе. Мы используем многопроцессную схему с веб-сервером Unicorn Rack, выполняющим наше основное Rails-приложение. обрабатывающее такие задачи, как отображение issues и пул-реквестов.

После изучения логов мы выяснили, что тело HTTP-объекта в передаваемом нами ответе клиенту было правильным, ошибочным были только куки в ответе пользователю. Отправившие отчёты об ошибке пользователи получили куки сессии пользователя, запрос которого совсем незадолго до них обрабатывался внутри того же процесса. В одном из случаев два запроса обрабатывались последовательно, один за другим. Во втором случае между ними было ещё два запроса.

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

Исследовав недавно внедрённые функции, мы выявили потенциальную проблему с безопасностью потоков, которая могла возникать из-за функциональности с недавно изменённой ради повышения производительности архитектурой. Одно из таких улучшений производительности заключалось в переносе логики включенных для пользователя функций в фоновый поток, который обновлялся с определённым интервалом вместо проверки их состояния во время обработки запроса. Нам показалось, что это изменение затронуло изучаемые нами области и теперь основной целью нашего исследования стало неопределённое поведение этой проблемы с безопасностью потоков.

Безопасность потоков и отчёты об ошибках


Для понимания проблемы с безопасностью потоков необходим контекст. Основное приложение, обрабатывающее большинство браузерных взаимодействий на GitHub.com это приложение Ruby on Rails, известное тем, что оно имело компоненты, написанные без учёта возможности в нескольких потоках (т.е. они были непотокобезопасны). Обычно в прошлом непотокобезопасное поведение могло приводить к неправильному значению во внутренних отчётах об исключениях системы, но при этом пользователи не сталкивались с изменением поведения системы.

Потоки уже использовались в других частях этого приложения, однако новый фоновый поток создал новое и непредусмотренное взаимодействие с нашими процедурами обработки исключений. Когда исключения (например, истечение срока запроса) передавал фоновый поток, лог ошибок содержал информацию и от фонового потока, и от текущего выполняемого запроса, и это доказывало, что данные передаются между потоками.

Изначально мы думали, что это только внутренняя проблема отчётности, и что мы будем наблюдать в логах данные, зафиксированные от не относящегося к делу запроса в фоновом потоке. Несмотря на противоречивость, мы посчитали такое поведение безопасным, поскольку каждый запрос имел собственные данные запроса, а Rails-приложение создавало новый экземпляр объекта-контроллера для каждого запроса. Нам по-прежнему было непонятно, как это могло вызывать проблемы, с которыми мы столкнулись.

Многократно используемый объект


Наша команда совершила прорыв, обнаружив, что HTTP-сервер Unicorn Rack, используемый в нашем Rails-приложении, не создаёт новый и отдельный объект env для каждого запроса. Вместо этого он выделяет единственный Ruby Hash, который очищается (с помощью Hash#clear) между запросами, которые он обрабатывает. Благодаря этому мы поняли, что проблема с потокобезопасностью в логгинге исключений может привести не только к неправильности фиксируемых в исключениях данных, но и к передаче данных запросов на GitHub.com.

Первоначальный анализ привёл к созданию гипотезы о том, что для возникновения условия гонки необходимо два запроса, происходящих за короткий промежуток времени. Получив эту информацию, мы попытались воспроизвести проблему в среде разработки. Когда мы попробовали выполнить последовательность определённых запросов, то обнаружили, что требуется ещё одно дополнительное условие, а именно анонимный запрос, с которого начинается вся последовательность. Вот полный список необходимых шагов:


  1. В потоке обработки запросов запускается анонимный запрос (назовём его Request #1). Он регистрирует обратный вызов в текущем контексте для внутренней библиотеки отчётности об исключениях. Обратные вызовы содержат ссылки на текущий объект контроллера Rails, имеющий доступ к единому объекту среды запроса Rack, предоставляемого сервером Unicorn.
  2. В фоновом потоке возникает исключение. Сообщение об исключении копирует текущий контекст, чтобы включить его в отчёт. Этот контекст содержит обратные вызовы, зарегистрированные запросом Request #1, в том числе и ссылку на единую среду Rack.
  3. В основном потоке запускается новый запрос залогиненного пользователя (Request #2).
  4. В фоновом потоке система отчётности об исключениях обрабатывает обратные вызовы контекста. Один из обратных вызовов считывает идентификатор сессии пользователя, но поскольку запрос на момент контекста не имеет авторизации, эти данные ещё не считываются, и, следовательно, запускают новый вызов к системе авторизации через контроллер Rails из запроса Request #1. Этот контроллер пытается выполнить авторизацию и получает куки сессии из общей среды Rack. Так как среда Rack это общий объект для всех запросов, контроллер находит куки сессии запроса Request #2.
  5. В основном потоке запрос Request #2 завершается.
  6. Запускается ещё один запрос залогиненного пользователя (Request #3). В этот момент Request #3 завершает свой этап авторизации.
  7. В фоновом потоке контроллер завершает этап авторизации, записывая куки сессии в cookie jar, находящийся в среде Rack. На данном этапе это cookie jar для Request #3!
  8. Пользователь получает ответ на запрос Request #3. Но cookie jar был обновлён данными куки сессии Request #2, то есть пользователь теперь авторизован как пользователь из Request #2.

Подведём итог: если исключение возникло именно в нужный момент и если параллельная обработка запросов происходит именно в нужном порядке среди серии запросов, то в результате сессия одного запроса заменяется сессией более раннего запроса. Возврат неправильного куки происходит только для заголовка куки сессии, и, как говорилось выше, остальная часть тела ответа, например HTML, по-прежнему была связана с ранее авторизованным пользователем. Это поведение соответствовало с тем, что мы видели в логах запросов и мы смогли чётко идентифицировать все элементы, составлявшие первопричину состояния гонки.

Для возникновения этого бага требовались очень конкретные условия: фоновый поток, общий контекст исключения между основным потоком и фоновым потоком, обратные вызовы в контексте исключения, многократное использование объекта env между запросами и наша система авторизации. Подобная сложность является напоминанием о многих из пунктов, представленных в статье How Complex Systems Fail, и демонстрирует, что для возникновения подобного бага требуется множество различных сбоев.

Предпринимаем действия


Выявив первопричину, мы немедленно сделали основным приоритетом устранение двух условий, критически важных для появления этого бага. Во-первых, мы избавились от нового фонового потока, появившегося после вышеупомянутой модернизации архитектуры, призванной повысить производительность. Благодаря тому, что мы точно знали, что было добавлено в процессе этой работы, изменения легко можно было откатить. Изменение, в котором был удалён этот поток, вывели в продакшен 5 марта. После этого изменения мы знали, что условия, требуемые для возникновения условия гонки, больше не будут удовлетворяться, и что наша непосредственный риск возврата неверных сессий снижен.

После выпуска изменения с удалением фонового потока, мы создали патч для Unicorn, удаляющий возможность создания общей среды. Этот патч был выпущен 8 марта и ещё больше усилил изоляцию между запросами, даже на случай возникновения проблем с потокобезопасностью.

Кроме устранения бага мы предприняли действия для определения пользовательских сессий, на которые он повлиял. Мы изучили данные логгинга на предмет паттернов, соответствующих неправильно возвращаемых сессий. Затем мы вручную просмотрели соответствующие паттерну логи, чтобы определить, на самом ли деле сессия была неверно возвращена от одного пользователя другому.

В конце мы решили сделать ещё одно превентивное действие, чтобы гарантировать безопасность данных наших пользователей, и аннулировали все активные сессии пользователей на GitHub.com. Учитывая редкость условий, необходимых для возникновения этого условия гонки, мы знали, что вероятность возникновения бага очень мала. Хотя наш анализ логов, проведённый с 5 по 8 марта, подтвердил, что это была редкая проблема, мы не могли исключить вероятность того, что сессия была неверно возвращена, но никогда не использовалась. Мы не хотели идти на такой риск, учитывая потенциальный ущерб использования даже одной из таких неверно возвращённых сессий.

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

Продолжаем работу


Несмотря на устранение непосредственной угрозы, мы объединили усилия с мейнтейнером Unicorn, чтобы поднять изменения вверх по потоку и чтобы новые запросы выделяли собственные хэши среды. Если Unicorn будет использовать новые хэши для каждой среды, то это устранить возможность того, что один запрос ошибочно завладеет объектом, который может повлиять на следующий запрос. Это дополнительная защита, позволяющая предотвратить превращение подобных багов потокобезопасности в уязвимости защиты для других пользователей Unicorn. Хотим искренне поблагодарить мейнтейнеров Unicorn за сотрудничество и обсуждение данной проблемы.

Ещё одним предпринятым шагом стало устранение обратных вызовов из контекста логгинга исключений. Эти обратные вызовы были полезны для откладывания времени выполнения и избегания снижения производительности без необходимости. Их недостаток заключается в том, что обратный вызов также усложняет отладку условий гонки и потенциально может сохранять ссылки на уже давно завершённые запросы. Обработка исключений при помощи более простого кода позволяет нам сейчас и в будущем использовать эти процедуры более безопасным способом. Кроме того, мы работаем над упрощением и ограничением путей выполнения кода в частях, где выполняется обработка куки сессий.

Также мы хотим повысить потокобезопасность всей кодовой базы в таких областях, как обработка исключений и оснащение. Мы не хотим просто устранять конкретные баги, а стремимся обеспечить невозможность возникновения подобного класса проблем в будущем.

Подведём итог


Многопоточный код бывает очень сложен в анализе и отладке, особенно в тех кодовых базах, где потоки в прошлом использовались редко. Пока мы решили перестать использовать долгоживущие потоки в наших процессах Rails. Мы воспользуемся этой возможностью, чтобы сделать наш код более надёжным для различных многопотоковых контекстов, а также для обеспечения дополнительной защиты и создания потокобезопасной архитектуры для использования в будущем.

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

Благодаря чёткому пониманию проблемы мы смогли сосредоточиться на принятии нужных мер для ограничения её воздействия на пользователей. Полученные нами в процессе этой работы знания позволили нам обсудить ситуацию с ответственными лицами из разных отделов компании и принять конфиденциальное решение о необходимости выхода из системы всех пользователей GitHub.com. Мы лучше разобрались в сложных системах компании и воспользуемся этой возможностью для создания мер предосторожности, предотвращающих возникновение подобных проблем в будущем.
Подробнее..

Как PVS-Studio защищает от поспешных правок кода

24.03.2021 12:04:07 | Автор: admin

Недостижимый код
Хотя только недавно была заметка про проект CovidSim, есть хороший повод вновь про него вспомнить и продемонстрировать пользу регулярного использования PVS-Studio. Бывает, что все мы спешим и вносим правки в код, потеряв сосредоточенность. Статический анализатор может оказаться здесь хорошим помощником.


Всё началось с написания вот этих двух небольших заметок про открытый проект COVID-19 CovidSim Model:



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


Только мы начали проверять этот проект, как результаты не заставили себя ждать :). Сейчас я покажу свежую ошибку, вызванную недавними неаккуратными правками кода. Конечно, мы не станем описывать в дальнейшем каждый баг и недочёт, обнаруженный анализатором. Будем писать только про что-то интересное.


Вот что получилось, после недавних модификаций файла CovidSim.cpp:


Правки


Кто-то решил создавать массивы не на стеке, а в куче. Но был невнимателен, внося изменения. Обратите внимание, что освобождение памяти находится после оператора return:


int GetXMLNode(....){  char* buf = new char[65536];  char* CloseNode = new char[2048];  char* CloseParent = new char[2048];  ....  if (ResetFilePos) fseek(dat, CurPos, 0);  return ret;  delete[] buf;  delete[] CloseNode;  delete[] CloseParent;}

В результате перед нами фрагмент недостижимого кода (unreachable code). И заодно утечка памяти.


Хорошо, что PVS-Studio тут же сообщает про эту ошибку: V779 Unreachable code detected. It is possible that an error is present. CovidSim.cpp 675


Используйте статический анализ регулярно и сможете устранить множество ошибок на самом раннем этапе! Согласитесь, лучше сразу исправить этот мелкий баг, чем гадать, почему вдруг программа начала потреблять слишком много оперативной памяти.


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


Правильный и надёжный вариант кода:


std::unique_ptr<char[]> buf(new char[65536]);std::unique_ptr<char[]> CloseNode(new char[2048]);std::unique_ptr<char[]> CloseParent(new char[2048]);

Спасибо за внимание. Следуйте за мной в мир С++ и багов :). Twitter. Facebook.

Подробнее..

Перевод Путаница зависимостей. Как я взломал Apple, Microsoft и десятки других компаний

02.04.2021 20:22:21 | Автор: admin

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

pip install package_name

В некоторых языках программирования, таких как Python, это простой, более или менее официальный способ установки зависимостей для ваших проектов. Такие установщики обычно привязаны к публичным репозиториям кода, куда каждый может свободно загружать пакеты кода для использования другими пользователями.

Вы, наверное, уже слышали о таких инструментах у Node есть менеджер npm и реестр npm, система управления пакетами pip языка Python использует PyPI (Python Package Index), а систему gems для языка Ruby можно найти на сайте RubyGems.

Загрузив и установив пакет из любого из этих источников, вы, по сути, доверяете его издателю, запуская соответствующий код на своём компьютере. Так может ли эта слепая вера быть использована злоумышленниками?


Конечно, может.

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

Другие хорошо известные пути атаки цепочек зависимостей включают использование различных методов компрометации существующих пакетов или загрузку вредоносного кода под именами зависимостей, которых больше не существует.

Идея

Пытаясь взломать PayPal вместе со мной летом 2020 года, Джастин Гарднер поделился интересным фрагментом исходного кода Node.js, найденного в GitHub.

Код предназначался для внутреннего использования в PayPal, и в его файле package.json, по-видимому, содержалась смесь публичных и частных зависимостей публичные пакеты от npm, а также имена непубличных пакетов, скорее всего, размещённых внутри PayPal. В то время этих имён не было в публичном реестре npm.

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

  • Что произойдёт при загрузке в npm вредоносного кода с этими именами? Возможно ли, что некоторые внутренние проекты PayPal начнут по умолчанию переходить на новые публичные пакеты вместо частных?

  • Начнут ли разработчики или даже автоматизированные системы выполнять код внутри библиотек?

  • Если это сработает, сможем ли мы получить за это вознаграждение?

  • Сработает ли такая атака и против других компаний?

Без лишних церемоний я стал разрабатывать план, чтобы ответить на эти вопросы.

Идея заключалась в загрузке собственных вредоносных пакетов Node в реестр npm под всеми невостребованными именами, которые будут звонить домой с каждого компьютера, на котором они были установлены. Если какой-либо из пакетов в итоге устанавливается на серверах, принадлежащих PayPal, или в любом другом месте, то содержащийся в них код немедленно уведомляет меня.

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

Это всегда DNS

К счастью, npm позволяет при установке пакета автоматически выполнять произвольный код, что облегчает создание пакета Node, который собирает некоторую базовую информацию о каждом компьютере, на котором он установлен, с помощью своего сценария preinstall.

Чтобы найти баланс между возможностью идентифицировать организацию на основе данных и необходимостью избегать сбора слишком большого количества конфиденциальной информации, я остановился только на регистрации имени пользователя, имени хоста и текущего пути каждой уникальной установки. Наряду с внешними IP-адресами этого было достаточно, чтобы помочь специалистам по безопасности выявить потенциально уязвимые системы на основе моих отчётов, избегая при этом того, чтобы моё тестирование было ошибочно принято за реальную атаку.

Остался только вопрос: как мне вернуть эти данные?

Зная, что большинство возможных целей находится глубоко внутри хорошо защищённых корпоративных сетей, я решил, что кража данных DNS это правильный путь.

Отправка информации моему серверу по протоколу DNS не была важна для работы самого теста, но она указывала на меньшую вероятность блокировки трафика и его обнаружения на выходе.Данные в шестнадцатеричной кодировке использовались как часть DNS-запроса, который достигал моего собственного полномочного сервера доменных имён напрямую или через промежуточные сопоставители. Сервер был настроен для регистрации каждого полученного запроса, по сути, сохраняя запись каждого компьютера, в который были загружены пакеты.

Чем больше, тем лучше

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

Первая стратегия заключалась в поиске альтернативных экосистем для атаки. Поэтому я перенёс код как на Python, так и на Ruby, чтобы можно было загружать аналогичные пакеты в PyPI (Python Package Index) и RubyGems соответственно.

Однако, возможно, самой важной частью этого теста был поиск максимально возможного количества значимых имён зависимостей.

Несколько полных дней поиска имён частных пакетов, принадлежащих некоторым целевым компаниям, показали, что многие другие имена можно найти в GitHub, а также в основных службах размещения пакетов во внутренних пакетах, которые были случайно опубликованы, и даже в сообщениях на различных интернет-форумах.

Однако, оказалось, что лучше всего искать имена частных пакетов... в файлах JavaScript.

Видимо, довольно часто внутренние файлы package.json с именами зависимостей проектов JavaScript, встраиваются в общедоступные файлы сценариев во время их сборки, раскрывая имена внутренних пакетов. Точно так же получаемые по каналам утечки внутренние пути или вызовы require() в таких файлах также могут содержать имена зависимостей. Apple, Yelp и Tesla это лишь несколько примеров компаний, у которых внутренние имена были раскрыты таким образом.

Во второй половине 2020 года, благодаря помощи пользователя @streaak и его замечательным навыкам реконструкции, мы смогли автоматически просканировать миллионы доменов, принадлежащих целевым компаниям, и извлечь сотни дополнительных имён пакетов JavaScript, которые ещё не были заявлены в реестре npm.Затем я загрузил свой код в службу размещения пакетов под всеми найденными именами и стал ждать обратных вызовов.

Результаты

Успех был просто поразительным.

От единичных ошибок, допущенных разработчиками на собственных компьютерах, до неправильно сконфигурированных внутренних или облачных серверов сборки, до системно уязвимых конвейеров разработки, было ясно одно: использование действительных имён внутренних пакетов было способом, который почти наверняка позволял проникнуть в сети некоторых крупнейших технологических компаний, получить доступ к удалённому выполнению кода и, возможно, позволить злоумышленникам добавлять бэкдоры во время сборки.

На тот момент уязвимость этого типа, которую я начал называть путаницей зависимостей, была обнаружен более чем в 35 организациях, во всех трёх тестируемых языках программирования. У подавляющего большинства затронутых компаний больше 1000 сотрудников, что, скорее всего, отражает более высокую распространённость внутреннего использования библиотек в более крупных организациях.

Поскольку имена зависимостей JavaScript легче найти, почти 75% всех зарегистрированных обратных вызовов исходили из пакетов npm, но это не обязательно означает, что Python и Ruby менее восприимчивы к такой атаке. На самом деле, несмотря на то что во время моих поисков мне удалось идентифицировать лишь внутренние имена gem для Ruby, принадлежащие восьми организациям, четыре из этих компаний оказались уязвимыми для путаницы зависимостей посредством RubyGems.

Одна такая компания канадский гигант электронной коммерции Giant Shopify, система сборки которой автоматически устанавливала пакет gem для Ruby с именем shopify-cloud всего лишь через несколько часов после того, как я его загрузил, а затем попытался выполнить код внутри него. Специалисты Shopify подготовили исправление в течение дня, а за обнаружение ошибки была присуждена награда в размере 30000 долларов.

Ещё одна награда в размере 30000 долларов была получена от Apple после того, как код в пакете Node, который я загрузил в npm в августе 2020 года, был выполнен на нескольких компьютерах внутри сети этой компании. Затронутые проекты оказались связаны с системой аутентификации Apple, известной за пределами компании как Apple ID.

Когда я подал идею о том, что эта ошибка, возможно, позволяла злоумышленникам внедрять бэкдоры в Apple ID, в компании Apple не посчитали, что этот уровень воздействия точно отражает проблему, и заявили:

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

Однако в компании Apple подтвердили возможность удалённого выполнения кода на серверах Apple с помощью этой технологии пакетов npm. Судя по потоку установок пакетов, проблема была исправлена в течение двух недель после моего отчёта, но премия за обнаружение ошибки была присуждена менее чем за день до этой публикации.

Такое же использование пакетов npm, устанавливаемых как на внутренние серверы, так и на ПК отдельных разработчиков, наблюдалось и в нескольких других успешных атаках на другие компании, причём некоторые установки часто выполнялись через часы или даже минуты после загрузки пакетов.

О, а имена PayPal, с которых всё началось? Они тоже сработали, что привело к ещё одной награде в размере 30 000 долларов. Фактически размер большинства присужденных премий за обнаружение ошибки был максимально разрешённым в рамках политики каждой программы, а иногда и превышал максимум, подтверждая в целом высокую серьезность уязвимости путаница зависимостей.

К затронутым компаниям также относятся Netflix, Yelp и Uber.

Это не баг, это фича

Несмотря на получение большого количества результатов по уязвимости путаница зависимости, одна деталь была и в определённой степени остаётся до сих пор неясной: почему это происходит? Каковы основные первопричины уязвимости такого рода?

Большинство затронутых организаций неохотно делились дальнейшими техническими подробностями о своих первопричинах и стратегиях смягчения последствий, но несколько интересных деталей всё же появилось во время моих исследований и общения со специалистами по безопасности.

Например, главный виновник путаницы зависимостей в Python, это, по-видимому, неправильное использование небезопасного по своей конструкции аргумента командной строки --extra-index-url. Если, используя этот аргумент с командой pip install library, указать собственный индекс пакета, можно обнаружить, что он работает ожидаемым образом, но то, что pip на самом деле делает за кулисами, выглядит примерно так:

  • проверяет существование library в указанном (внутреннем) индексе пакетов;

  • проверяет существование library в публичном индексе пакетов (PyPI);

  • устанавливает любую найденную версию. Если пакет существует в обоих индексах, то по умолчанию он устанавливается из источника с более высоким номером версии.

Поэтому загрузка пакета с именем library 9000.0.0 в PyPI приведёт к захвату зависимости в приведённом выше примере.

Хотя такое поведение уже было широко известно, простого поиска в GitHub выражения --extra-index-url было достаточно, чтобы найти несколько уязвимых сценариев, принадлежащих крупным организациям, включая ошибку, влияющую на компонент Microsoft .NET Core. Данная уязвимость, которая, возможно, позволила бы добавлять бэкдоры в .NET Core, к сожалению, была обнаружена вне рамок программы вознаграждения за нахождение ошибок в .NET.

В Ruby команда gem install --source работает аналогичным образом, но мне не удалось подтвердить, было ли её использование основной причиной каких-либо моих находок.

Конечно, замена --extra-index-url на --index-url быстро и прямо исправляет уязвимость, но снизить риск некоторых других вариантов уязвимости путаница зависимостей оказалось гораздо труднее.

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

В JFrog, как сообщается, знают об этой проблеме, но рассматривает её возможное исправление как запрос функции без расчётного времени его реализации, в то время как некоторые клиенты компании прибегли к изменению системной политики управления зависимостями, чтобы уменьшить риск путаницы зависимостей.

Компания Microsoft также предлагает аналогичную службу размещения пакетов под названием Azure Artifacts. По результатам одного из моих отчётов в эту службу были внесены некоторые незначительные улучшения, чтобы она могла гарантированно предоставить надёжный путь обхода уязвимостей путаница зависимостей. Как ни странно, эта проблема была обнаружена не в результате тестирования самой службы Azure Artifacts, а, скорее, путём успешной атаки на облачную систему Microsoft Office 365, в результате чего за этот отчёт была получена самая высокая награда Azure в размере 40 000 долларов.

Более подробную информацию о первопричинах и рекомендациях по профилактике можно найти в техническом документе Microsoft 3 Ways to Mitigate Risk When Using Private Package Feeds (Три способа снижения риска при использовании каналов частных пакетов).

Будущие исследования?

Хотя многие крупные технологические компании уже осведомлены об уязвимости этого типа и либо исправили её в своей инфраструктуре, либо работают над внедрением мер по смягчению последствий, у меня сохраняется ощущение, что есть ещё что-то, что нужно обнаружить.

В частности, я считаю, что поиск новых и умных способов утечки имён внутренних пакетов откроет ещё более уязвимые системы, а поиск целевых альтернативных языков программирования и репозиториев откроет некоторые дополнительные направления атак на основе ошибок, связанных с путаницей зависимостей.

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

Даже крупные компании совершают порой ошибки и допускают уязвимости. На нашем курсе Этичный хакер мы учим студентов искать эти уязвимости и зарабатывать на этом. Если устали чинить баги в коде и постоянно надстраивать что-то новое приходите к нам, будем учиться ломать, находя бреши в софте даже у крутых корпораций.

Узнайте, как прокачаться в других специальностях или освоить их с нуля:

Другие профессии и курсы
Подробнее..

Вы часть руководства? Отключите прием вызовов в телеграм! Баг-хантер? Уважайте других людей

13.04.2021 22:15:19 | Автор: admin

Всем привет. Сегодня речь пойдет не совсем о разработке. Я даже не знаю с чего начать. Это просто крик души.

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

Хто я?

Я - технический директор зарубежной компании. Прошел весь путь с момента идеи продукта и до текущего момента. По сути, мы - стартап, но вышли на самоокупаемость. Я часто отвечаю в поддержке для ускорения процесса если, конечно же, у меня есть свободное время. Итак, история началась 3 месяца назад. Наша команда в основном работает удаленно, но желающие могут посещать офис в Москве. Так вот, в конце января наш офис (а за одно и столицу) решил посетить один из сотрудников. Было принято решение встретить его с аэропорта. Должен сказать, я не отвечаю на телефонные звонки когда за рулем. И вот я еду в аэропорт, как мне начинает звонить раннее неизвестный мне человек в телеге. Я конечно же сбрасываю, но так просто я не отделался. Звонки начали повторяться постоянно. Сбросил - через 5 секунд тебе уже снова звонят. Ну что-ж. Выход очевиден: беззвучный режим и глаза на дорогу.

Как только я доехал до аэропорта и открыл чат - увидел что этот человек, который так сильно названивал, нашел несколько багов в нашем приложении и просит вознаграждение. После чего я перенаправляю его на других сотрудников, забираю коллегу и еду обратно. Дальше пришло несколько сообщений которые я снова не прочитал потому что был за рулем и снова очередь звонков, которые были сброшены.

Вернувшись в офис, я решил уделить время охотнику за багами. После небольшой переписки, багхантер понимает что вознаграждение не я плачу и что все-таки я бесполезен для него. Ситуация стихает на некоторое время. Но неделю назад все началось с бОльшей силой. Я как обычно сижу на работе, провожу митап со своей командой, как мне начинают звонить. Естественно все по старой схеме: я занят > сброс > звонок. Цикл замкнулся.

while (true) {  const rand = randomFloat();  if (rand > 0.9) {    writeMessage("Hi");    continue;  }    callTelegramm();}

После митапа я пишу "What happened?" ииии... Дальше ничего. Сообщение не прочитано, человек так сильно пытался до меня дозвониться и пропал. Через 3 дня он объявляется и сообщает что нашел аж 10(!) багов. Я думаю "как же так, 10 багов, я о них ничего не знаю". Логично что я сразу же занялся его вопросом и решил лично проверить все.

-_- ([%$#%^&$#] заголовок украден хакерами анонимусами)

После проверки "багов" оказалось что часть из них - возможность вытащить "личные данные пользователей". Звучит страшно, но на деле там оказалось что можно получить nickname пользователя, чей ник ты и так должен знать и (внимание) ID пользователя. Wow. Чтобы было немного понятнее: вы можете найти в телеграм пользователя по его нику. Чтобы его найти - вы и так должны знать ник. Это точно личные данные пользователей уязвимые для атаки? Вот такой же "баг" у нас нашли.

Другой "баг" - возможность доступа к открытым разделам приложения без оплаты. Мы сняли требование оплаты для того чтобы пользователь мог зайти в настройки и изменить емаил или пароль. Или же сменить тарифный план на бесплатный. Так вот как оказалось - это критическая уязвимость.

Еще могу рассказать как взломать абсолютно любой сервис. Как же? Все просто: открываете дев-тулзы на сайте и меняете текст в хтмл на свой личный. Для удобства представим что вы в сообществе вк добавляете модератора. Так вот вы находите в девтулзах слово "модератор" и пишете "владелец". А потом нажимаете кнопку, которая и выдает права. Проверять не стоит, ведь вы и так уверены что сообщество теперь принадлежит другому пользователю. Правда если все же проверить права - он окажется модератором, но это не важно. Звучит как ерунда? Я с вами согласен.

В конечном счете оказалось из 10 багов, за которые, кстати, тот самый нереальный охотник просил $2000, действительно багами считать можно только 2. И то они заключались в том, что мы не ограничиваем максимальное количество вводимых символов в инпут пароля при логине и регистрации. Казалось бы: можно попробовать забить сервер. Но при превышении максимального веса запроса - сервер сам отбросит его, да и к тому же у нас передается очень много данных между клиентом и сервером. Бывает возвращается ответ с 10к+ строк текста.

Пора поговорить о bug bounty

Решение было принято. За это платить $2000 не будем, оно того не стоит. Было предложено $400 за эти 2 момента. Но эта сумма не устроила охотника за багами. Он снисходительно снизил цену до $1500. 1500? Что? За максимальное ограничение символов? Его аргументом стало то, что это можно использовать для DDOS-атаки. Но DDOS-атака не проводится на поля для ввода. Она не так подбирается. Ищется место, в котором сервер медленнее всего обрабатывает запрос - очевидно там тяжелая работа с базой или какая-то дополнительная обработка, а может и в оперативу выгружает большое количество данных. И именно в этом место начинается атака чтобы положить сервер. В результате я предложил решение: проведешь успешную атаку с этим "багом" и мы поговорим об оплате за него.

Тут баг-хантер решил что пора понижать планку еще: до $1000. На самом деле $400 за эти "баги" и так сильно завышенная цена. После моего отказа - начались звонки - я отписал что в данный момент работаю и не могу ответить, но звонки продолжились. На следующий день все повторилось - я перенаправил его на других сотрудников, но с ними сей человек разговаривать не стал и продолжил долбиться ко мне в личку. В конце рабочего дня он отписал, что согласен на $600, после чего мы с ним еще раз поговорили и сошлись на 500, но без дальнейшего продолжения сотрудничества. После чего я связался с CEO компании и оказалось, что за день до этого ему уже перевели $500 (в тот момент, когда его это не устраивало). Я снова списался с охотником на баги и оказалось что он хочет еще $500. То есть в общей сложности $1000. И снова начались звонки... Я запретил ему в настройках звонить мне - он зарегал второй аккаунт, затем третий. После - начал звонить и писать в чат поддержки с требованием перевести ему еще со словами "иначе с вами никто не будет работать". Дальше - больше. Дело пошло к угрозам, жалобным и злобным эмоджи.

Потом пошел флуд в личку. И в конечном счете я не выдержал и отправил его в бан.

Может быть у кого-то были подобные проблемы? Как вы их решаете? Формально у нас не было компании на hackerOne. Этот человек сам нашел нас и сам же скинул баги и запросил цену. Мы готовы платить, но всему есть предел. Буду рад обсудить это в комментариях.

Мораль сей басни такова

  1. Не названивайте в телеграм постоянно. Если человек сбросил - он занят и обязательно ответит позже

  2. Не ломите цену. Ну ей богу. Ограничение по количеству символов в инпуте не стоит 2000 у.е. Оно и 500 не стоит если честно. Такими темпами можно будет оценивать отсутствие валидации полей на клиенте в тысячу долларов. А главным аргументом станет "Ну и что, что запрос не выполняется. Он же на сервер улетает. Запрос лишний - можно дудосить"

  3. Проверяйте баги прежде чем отписывать о них. Изменить в девтулзах текст - это не баг если он ни к чему не привел

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

Подробнее..

Перевод Лучшая практика обработки ошибок в современном JavaScript

07.10.2020 16:09:34 | Автор: admin

Когда вы пишете код, важно учитывать ситуации, приводящие к ошибкам. Обработка ошибок это неотъемлемая часть работы над веб-приложением. Мы посмотрим на некоторые рекомендации по обработке ошибок в JavaScript. Чтобы не тратить ваше время зря, сразу поясняем, что описанное в статье может быть не в новинку многопытным кодерам. Если вы себя таким считаете смело пропускайте этот материал, всех остальных приглашаем под кат.



Расширяем класс Error


Часто бывает полезно предоставить детальное описание ошибки внутри обработчика. И под этим я подразумеваю не только четкое сообщения об ошибке. Я имею в виду расширение класса Error. Расширив класс Error, вы можете настроить полезные при отладке свойства name и message, а также написать пользовательские геттеры, сеттеры и другие методы:

class BadParametersError extends Error {  name = 'BadParametersError'  constructor(message) {    super(message)  }  get recommendation() {    return this._recommendation  }  set recommendation(recommendation) {    this._recommendation = recommendation  }}

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

Посмотрим на ситуацию, когда расширение Error приносит пользу. Допустим, у вас есть функция, принимающая список функций решателя. Она принимает аргумент, проходит по списку решателей и передает аргумент в каждую функцию. Если функция возвращает какой-то результат, проход останавливается и этот результат возвращается функцией:

// Takes a list of resolvers, composes them and returns a func that calls// each resolvers on the provided args.function composeResolvers(...resolvers) {  return (args) => {    let result    for (let index = 0; index < resolvers.length; index++) {      const resolve = resolvers[index]      result = resolve(args)      if (result) {        break // Abort the loop since we now found a value      }    }    return result  }}

Представьте, что вы пишете страницу, где пользователю предлагается ввести год рождения, чтобы определить его в какую-то группу:

import composeResolvers from '../composeResolvers'const resolvers = []const someResolverFn = (userInput) => {  if (userInput > 2002) {    return 'NewKidsOnTheBlock'  }  return 'OldEnoughToVote'}// Pretending our code is only stable/supported by certain browsersif (/chrome/i.test(navigator.userAgent)) {  resolvers.push(someResolverFn)}const resolve = composeResolvers(...resolvers)window.addEventListener('load', () => {  const userInput = window.prompt('What year was your computer created?')  const result = resolve(userInput)  window.alert(`We recommend that you register for the group: ${result}`)})

Когда пользователь нажимает OK, его возраст присваивается userInput и передается в качестве аргумента функции из composeResolvers:

import composeResolvers from '../composeResolvers'const resolvers = []const someResolverFn = (userInput) => {  if (userInput > 2002) {    return 'NewKidsOnTheBlock'  }  return 'OldEnoughToVote'}// Pretending our code is only stable/supported by certain browsersif (/chrome/i.test(navigator.userAgent)) {  resolvers.push(someResolverFn)}const resolve = composeResolvers(...resolvers)window.addEventListener('load', () => {  const userInput = window.prompt('What year was your computer created?')  const result = resolve(userInput)  window.alert(`We recommend that you register for the group: ${result}`)})



По окончании работы запускается window.alert, чтобы показать пользователю его группу:



Код работает нормально. Но что, если пользователь смотрит страницу не в Chrome? Тогда строка resolvers.push(someResolverFn) не работает. Ниже мы видим неприятный результат:



Мы можем предупредить необработанные ошибки, бросив обычную Error, или можем использовать более подходящую BadParametersError:

// Takes a list of resolvers, composes them and returns a func that calls// each resolvers on the provided args.function composeResolvers(...resolvers) {  if (!resolvers.length) {    const err = new BadParametersError(      'Need at least one function to compose resolvers',    )    err.recommendation =      'Provide a function that takes one argument and returns a value'    throw err  }  return (args) => {    let result    for (let index = 0; index < resolvers.length; index++) {      const resolve = resolvers[index]      result = resolve(args)      if (result) {        break // Abort the loop since we now found a value      }    }    return result  }}

Так у ошибки гораздо меньше шансов попасть к пользователю. Сообщение заставляет разработчика исправить ситуацию:



Отладка сильно упрощается, когда по этой стратегии работают несколько функций и к ним прикреплено какое-то свойство. Теперь все это можно написать иначе и такой защитный механизм лучше:

const resolve = composeResolvers(...resolvers)window.addEventListener('load', () => {  const userInput = window.prompt('What year was your computer brought to you?')  let result  try {    result = resolve(userInput)  } catch (error) {    if (error instanceof BadParametersError) {      console.error(        `[Error] ${error.message}. Here's a recommendation: ${error.recommendation}`,      )      console.log(error.recommendation)    } else {      // Do some fallback logic      return window.alert(        'We are sorry, there was a technical problem. Please come back later',      )    }  }  window.alert(`We recommend that you register for the group: ${result}`)})

Применение TypeError


Мы часто работаем с Error, но когда есть более подходящая встроенная ошибка, полезно не пренебрегать ей:

async function fetchDogs(id) {  let result  if (typeof id === 'string') {    result = await api.fetchDogs(id)  } else if (typeof id === 'array') {    result = await Promise.all(id.map((str) => api.fetchDogs(id)))  } else {    throw new TypeError(      'callSomeApi only accepts a string or an array of strings',    )  }  return result}const params = { id: 'doggie123' }let dogsfetchDogs(params)  .then((dogs) => {    dogs = dogs  })  .catch((err) => {    if (err instanceof TypeError) {      dogs = Promise.resolve(fetchDogs(params.id))    } else {      throw err    }  })


Тестирование


Благодаря наследованию Error тестирование становится надежнее. Такие ошибки можно использовать при написании ассертов:

import { expect } from 'chai'import chaiAsPromised from 'chai-as-promised'import fetchCats from '../fetchCats'chai.use(chaiAsPromised)it('should only take in arrays', () => {  expect(fetchCats('abc123')).to.eventually.rejectWith(TypeError)})


Важно не перестараться


Возникает соблазн просто создать много пользовательских ошибок для каждой возможной ситуации. Особенно когда вы только начинаете изучать JavaScript. Если приложение маленькое или среднее по размеру приложение, можно попасть в такую ситуацию:

class AbortExecuteError extends Error {  name = 'AbortExecuteError'  constructor(message) {    super(message)  }}class BadParameters extends Error {  name = 'BadParameters'  constructor(message) {    super(message)  }}class TimedOutError extends Error {  name = 'TimedOutError'  constructor(message) {    super(message)  }}class ArrayTooLongError extends Error {  name = 'ArrayTooLongError'  constructor(message) {    super(message)  }}class UsernameError extends Error {  name = 'UsernameError'  constructor(message) {    super(message)  }}

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

class TimedOutError extends Error {  name = 'TimedOutError'  retried = 0  constructor(message) {    super(message)  }  set retry(callback) {    this._retry = callback  }  retry(...args) {    this.retried++    return this._retry(...args)  }}class ConnectToRoomTimedOutError extends TimedOutError {  name = 'ConnectToRoomTimedOutError'  constructor(message) {    super(message)  }  get token() {    return this._token  }  set token(token) {    this._token = token  }}let timeoutRefasync function connect(token) {  if (timeoutRef) clearTimeout(timeoutRef)    timeoutRef = setTimeout(() => {      const err = new ConnectToRoomTimedOutError(        'Did not receive a response from the server',      )      err.retry = connect      err.token = token      throw err    }, 10000)    const room = await api.join(token)    clearTimeout(timeoutRef)    return room  }}const joinRoom = () => getToken().then((token) => connect(token))async function start() {  try {    let room = await joinRoom()    return room  } catch (err) {    if (err instanceof ConnectToRoomTimedOutError) {      try {        // Lets retry one more time        room = await err.retry(err.token)        return room      } catch (innerErr) {        throw innerError      }    }    throw err  }}start()  .then((room) => {    console.log(`Received room, oh yea!`, room)  })  .catch(console.error)

Помните, что обработка ошибок экономит ваши деньги и время.

image

Получить востребованную профессию с нуля или Level Up по навыкам и зарплате, можно, пройдя онлайн-курсы SkillFactory:



Подробнее..

Исследование COVID-19 и неинициализированная переменная

05.02.2021 14:16:53 | Автор: admin

0796_covid_sim_ru/image1.png
Существует открытый проект COVID-19 CovidSim Model, написанный на языке C++. Существует статический анализатор кода PVS-Studio, который умеет хорошо находить ошибки. Однажды они встретились. Познайте хрупкость алгоритмов математического моделирования и почему нужно прикладывать максимум усилий к качеству программного кода.


На днях мне понадобилось кое-что найти на GitHub, что является началом этой маленькой истории. Изучая результаты поиска, я случайно набрёл на проект COVID-19 CovidSim Model. Недолго думая, я решил проверить его с помощью анализатора PVS-Studio.


Проект оказался совсем крошечным. В нём всего 13 000 строк кода, если не считать пустые строки и комментарии. И ошибок там тоже почти нет. Но одна ошибка настолько проста и красива, что я не могу пройти мимо!


void CalcLikelihood(int run, std::string const& DataFile,                    std::string const& OutFileBase){  ....  double m = Data[row][col]; // numerator  double N = Data[row][col + 1]; // denominator  double ModelValue;  // loop over all days of infection up to day of sample  for (int k = offset; k < day; k++)  {    // add P1 to P2 to prevent degeneracy    double prob_seroconvert = P.SeroConvMaxSens *      (1.0 - 0.5 * ((exp(-((double)(_I64(day) - k)) * P.SeroConvP1) + 1.0) *      exp(-((double)(_I64(day) - k)) * P.SeroConvP2)));    ModelValue += c * TimeSeries[k - offset].incI * prob_seroconvert;  }  ModelValue += c * TimeSeries[day - offset].S * (1.0 - P.SeroConvSpec);  ModelValue /= ((double)P.PopSize);  // subtract saturated likelihood  LL += m * log((ModelValue + 1e-20) / (m / N + 1e-20)) +        (N - m) * log((1.0 - ModelValue + 1e-20) / (1.0 - m / N + 1e-20));  ....}

Серьёзный научный код. Что-то считается. Формулы. Выглядит всё умно и обстоятельно.


Вот только все эти вычисления разбиваются о человеческую невнимательность. Хорошо, что на помощь может прийти анализатор кода PVS-Studio и указать на баг: V614 [CWE-457] Uninitialized variable 'ModelValue' used. CovidSim.cpp 5412


И действительно, посмотрим внимательнее на это:


double ModelValue;for (int k = offset; k < day; k++){  double prob_seroconvert = ....;  ModelValue += c * TimeSeries[k - offset].incI * prob_seroconvert;}

Перед нами простая и одновременно страшная ошибка: неинициализированная переменная. Этот алгоритм может насчитать всё что угодно.


Собственно, на этом всё. Пояснять тут нечего. Остаётся только вновь напомнить, что разработчики научных библиотек и научных приложений должны прикладывать дополнительные усилия для обеспечения качества кода. Падение обыкновенного приложения, скорее всего, будет стоить намного дешевле, чем использование некорректных результатов вычислений в научных, медицинских и прочих целях.


Это уже не первая наша статья на эту тему:



Используйте статический анализатор кода PVS-Studio! Польза от своевременно найденных ошибок может быть колоссальной. Спасибо за внимание.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. COVID-19 Research and Uninitialized Variable.

Подробнее..

ТОП самых неудачных мотиваций разработчиков

18.10.2020 16:23:45 | Автор: admin

У руководителя есть простые человеческие желания чтобы релизы выходили вовремя, багов было меньше, внутри коллектива не было болота с крокодилами, гадюками и жабама, взаимодействие между девами и опсами больше напоминали лозунг мир, дружба, жвачка, а не бей ...., спасай Россию. И много-много ещё других не менее важных целей, подцелей и пожеланий.


Одна отдельная и важная цель мотивация сотрудников, а именно разработчиков.


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



В первый очередь вспоминается знаменитая ошибка Гугла вознаграждение от количества строк кода. Как вы понимаете, в соревновании за премии победили индусские программисты с разгромным счётом. Да и было бы странно не заметить в этот системе мотивации огромаднейшую дыру в логике.


Yahoo решила зайти с обратной стороны. И стали осыпать разработчиков бонусами за код без багов. Тут они наткнулись на когнитивные грабли. Разработчики просто стали подсознательно бояться пропустить баг. На внутреннем уровне они получили от любимой работы повышенный уровень кортизола, тревожности и ускоренное выгорание. Скорость выкладки в прод уменьшилась раз в 8, релизы встали напрочь.


Ещё была одна эпическая ошибка Google, которую радостно повторил император в Яндексе. В выигрыше оказались HR потому что злые и обсценно разговаривающие продакты и проджекты просто повалили валом. Решение Яндекса было просто увольнение по результатам оценки. "Вы. Самое. Слабое. Звено", прямо звенит голос Марии Киселевой. Кто в этой системе получил самые низкие баллы? Естественно, те, кто шел на конфликты с окружением, пытаясь пробить для своих проектов максимальную скорость, ресурсы и прочее. Они и вылетали. Как говорится, инициатива наказуема в стиле Яндекса.


Неизменной уже много лет остаётся ошибка Google и Facebook. Каждый год в каждом подразделении увольняют 20 процентов самых слабых. Вдумайтесь. Из самого слабого подразделения увольняют 20%. И из самого сильного подразделения увольняют 20%. Хэдхантеры потирают ладошки и вылавливают обиженных.


Есть такая штуковина. Чисто русская. Называется Руджайл. Это Agile с русским колоритом берём методологию Agile, добавляем карго-культ, тиранию, медведей, ядерный реактор. В общем, то, что сделал Сбербанк. Берется Agile и внедряется везде даже если не лезет. Причем внедряют Agile так, как поняли сами. Не важно, что отцы основатели Agile говорят не работает везде. Не всеприменимая технология. А на русское привсеместное внедрение они давно на своем форуме говорят только одно: русские офигели, мы тут не при чем, простите братья-программисты.


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


Столь прекрасные истории собрала Алёна Владимирская. За что ей большое спасибо за хорошее настроение с самого утра.


Елена Арсеньева не осталась в стороне и поделилась тоже показательной историей. Была еще такая история, как KPI от количества закрытых багов, без учета сложности багов. Самые сложные и критичные баги стекались к самому умному, а он по показателям оказывался самым плохим.


Глеб Кудрявцев вспомнил историю, как в Ютинете в свое время вообще топчик сделали. Начали снимать с разработчиков половину зарплаты за незакрытый спринт. После первой же такой зарплаты уволилась половина разработчиков. Очень эффективно. Опасно, круто, как русская рулетка. С компанией тоже известно, что стало.


Все эти чудесные и в целом полезные истории собрал у себя Дмитрий Симонов ctorecords, CTO и создатель канала Техдирские заметки и партнёр учебного центра Слёрм.


P.S. А как мотивируют у вас? Как гладят по шёрстке? :)

Подробнее..

Navigation Component-дзюцу, vol. 3 Corner-кейсы

23.09.2020 10:08:04 | Автор: admin


В этой части трилогии про Navigation Component разберем как организовать навигацию в многомодульных приложениях, как работается с диплинками, а также рассмотрим кейсы со встраиваемыми фрагментами и диалогами.


Это третья и заключительная статья в цикле про различные кейсы навигации с Navigation Component-ом. Вы также можете ознакомиться с первой и второй частями



Если вы работаете с большим приложением, вероятно, вы уже разбили его на модули. Неважно, как именно. Может быть, вы создаёте отдельные модули для логики и UI, а может храните всю логику фичи (от взаимодействия с API до логики presentation-слоя) в одном модуле. Главное у вас могут быть кейсы, когда требуется осуществить навигацию между двумя независимыми модулями.


Где на схеме приложения кейсы с навигацией?


На картинке мы видим, что у нас есть как минимум два модуля: модуль :vacancy с одним экраном и модуль :company с двумя экранами вложенного flow. В рамках моего примера я построил навигацию из модуля :vacancy в модуль :company, которые не связаны друг с другом.


Существует три способа как это сделать, разберём их один за другим.


App-модуль + интерфейсы


Первый способ использовать ваш application-модуль в качестве хранилища всего графа навигации и определить в feature-модулях специальные интерфейсы для роутинга.


Структура вашего приложения в этом способе


Структура приложения будет стандартной: есть app-модуль, который знает обо всех feature-модулях, есть feature-модули, которые не знают друг о друге. В этом способе ваши feature-модули пребывают в священном неведении о Navigation Component, и для навигации они будут определять интерфейсы примерно вот такого вида:


// ::vacancy moduleinterface VacancyRouterSource {    fun openNextVacancy(vacancyId: String)    // For navigation to another module    fun openCompanyFlow()}

А ваш app-модуль будет реализовывать эти интерфейсы, потому что он знает обо всех action-ах и навигации:


fun initVacancyDI(navController: NavController) {  VacancyDI.vacancyRouterSource = object : VacancyRouterSource {      override fun openNextVacancy(vacancyId: String) {          navController.navigate(              VacancyFragmentDirections                .actionVacancyFragmentToVacancyFragment(vacancyId = vacancyId)          )      }      override fun openCompanyFlow() {          initCompanyDI(navController)          navController.navigate(R.id.action__VacancyFragment__to__CompanyFlow)      }  }}

Плюс этого способа отсутствие дополнительных модулей, которые могут повлиять на скорость сборки приложения. Однако в минусах вы получаете:


  • дополнительную работу в виде определения интерфейсов, реализаций, организации DI для проброса этих интерфейсов в ваши feature-модули;
  • отсутствие возможности использовать использовать Safe Args плагин, делегат navArgs, сгенерированные Directions, и другие фишки Navigation Component-а в feature-модулях, потому что эти модули ничего не знают про библиотеку.

Сомнительный, в общем, способ.


Графы навигации в feature-модулях + диплинки


Второй способ вынести отдельные графы навигации в feature-модули и использовать поддержку навигации по диплинкам (она же навигация по URI, которую добавили в Navigation Component 2.1).


Структура вашего приложения в этом способе


Структура приложения будет ровно такой же, как и в первом способе: есть app-модуль, который зависит от всех feature-модулей, и есть feature-модули, которые друг о друге ничего не знают.


Но теперь ваш app-модуль не обязан содержать весь граф навигации приложения, он может содержать только его часть. А остальные кусочки будут содержать именно feature-модули.


<navigation xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    android:id="@+id/company_flow__nav_graph"    app:startDestination="@id/CompanyFragment">    <fragment        android:id="@+id/CompanyFragment"        android:name="company.CompanyFragment">        <deepLink app:uri="companyflow://company" />        <!-- Or with arguments -->        <argument android:name="company_id" app:argType="long" />        <deepLink app:uri="companyflow://company" />        <action            android:id="@+id/action__CompanyFragment__to__CompanyDetailsFragment"            app:destination="@id/CompanyDetailsFragment" />    </fragment>    <fragment        android:id="@+id/CompanyDetailsFragment"        android:name="company.CompanyDetailsFragment" /></navigation>

Feature-модули будут определять свои собственные графы навигации для роутинга между экранами, о которых они знают. А ещё они будут объявлять диплинки для тех экранов, на которые можно попасть из других модулей. В примере выше мы добавили тэг deepLink, чтобы на экран CompanyFragment можно было попасть из другого модуля.


После этого мы можем использовать этот диплинк для открытия экрана CompanyFragment из модуля :vacancy :


// ::vacancy modulefragment_vacancy__button__open_company_flow.setOnClickListener {  // Navigation through deep link  val companyFlowUri = "companyflow://company".toUri()  findNavController().navigate(companyFlowUri)}

Плюс этого метода в том, что это самый простой способ навигации между двумя независимыми модулями. А минус что вы не сможете использовать Safe Args, или сложные типы аргументов (Enum, Serializable, Parcelable) при навигации между фичами.


P.S. Есть, конечно, вариант сериализовать ваши сложные структуры в JSON и передавать их в качестве String-аргументов в диплинк, но это как-то Странно.


Общий модуль со всем графом навигации


Третий способ ввести для хранения всего графа навигации специальный модуль, который будет подключаться к каждому feature-модулю.


Структура вашего приложения в этом способе


У нас по-прежнему есть app-модуль, но теперь его задача просто подсоединить к себе все feature-модули; он больше не хранит в себе граф навигации. Весь граф навигации теперь располагается в специальном модуле, который ничего не знает о feature-модулях. Зато каждый feature-модуль знает про common navigation.


В чём соль? Несмотря на то, что common-модуль не знает о реализациях ваших destination-ов (фрагментах, диалогах, activity), он всё равно способен объявить граф навигации в XML-файлах! Да, Android Studio начинает сходить с ума: все имена классов в XML-е горят красным, но, несмотря на это, все нужные классы генерируются, Safe Args плагин работает как нужно. И так как ваши feature-модули подключают к себе common-модуль, они могут свободно использовать все сгенерированные классы и пользоваться любыми action-ами вашего графа навигации.


Плюс этого способа наконец-то можно пользоваться всеми возможностями Navigation Component-а в любом feature-модуле. Из минусов:


  • добавился ещё один модуль в critical path каждого feature-модуля, которому потребовалась навигация;
  • отсутствует автоматический рефакторинг имён: если вы поменяете имя класса какого-нибудь destination-а, вам нужно будет не забыть, что надо поправить его в common-модуле.

Выводы по навигации в многомодульных приложениях


  • У вас на выбор целых три способа организации межмодульной навигации.
  • Выбирайте тот, который вам нравится, понятен и, главное, который вам проще реализовать.

Работа с диплинками


Практически каждое большое приложение должно уметь поддерживать диплинки. И практически каждый Android-разработчик мечтал о простом способе работы с этими глубокими ссылками. Окей, я мечтал. И казалось, что Navigation Component ровно то, что нужно.


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


Какую именно часть?


У меня было три кейса с диплинками, которые я хотел реализовать с помощью Navigation Component.


  • Открытие определённой вкладки нижней навигации допустим, я хочу через диплинк открыть вторую вкладку на главном экране после Splash-экрана

Посмотреть на картинке

Допустим, я хочу через диплинк открыть вкладку Favorites нижней навигации на главном экране после Splash-экрана:



  • Открытие определённого экрана ViewPager-а внутри конкретной вкладки нижней навигации

Посмотреть на картинке

Пусть я хочу открыть определённую вкладку ViewPager-а внутри вкладки Responses:



  • Открытие экрана, который требует авторизации пользователя пусть теперь для корректной работы вкладки нижней навигации требуется авторизация. И с помощью диплинка я хочу сначала показать Splash-экран, затем открыть флоу авторизации, а когда пользователь его пройдёт, открыть нужную вкладку

Посмотреть на картинке

Пусть вкладка Profile теперь требует авторизации. И с помощью диплинка я хочу сначала показать Splash-экран, затем открыть флоу авторизации, а когда пользователь его пройдёт открыть вкладку Profile.



Сразу скажу, что у меня не получилось нормально реализовать ни один из этих кейсов. И работа с диплинками в Navigation Component стала для меня самым большим разочарованием.



Прежде чем делать диплинк для второй вкладки, я решил настроить ссылку хотя бы для первой. Я решил, что для этого будет достаточно просто добавить диплинк на экран контейнера нижней навигации (не самой вкладки, а контейнера):


<navigation xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    android:id="@+id/app_nav_graph"    app:startDestination="@id/SplashFragment">    <fragment        android:id="@+id/SplashFragment"        android:name="ui.splash.SplashFragment" />    <fragment        android:id="@+id/MainFragment"        android:name="ui.main.MainFragment">        <deepLink app:uri="www.example.com/main" />    </fragment></navigation>

Затем я, следуя документации, добавил граф навигации с диплинком в Android Manifest:


<manifest xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    package="com.aaglobal.jnc_playground">    <application android:name=".App">        <activity android:name=".ui.root.RootActivity">            <nav-graph android:value="@navigation/app_nav_graph"/>            <intent-filter>                <action android:name="android.intent.action.MAIN" />                <category android:name="android.intent.category.LAUNCHER" />            </intent-filter>        </activity>    </application></manifest>

А потом решил проверить, работает ли то, что я настроил при помощи простой adb-команды:


adb shell am start \  -a android.intent.action.VIEW \  -d "https://www.example.com/main" com.aaglobal.jnc_playground

И-и-и нет. Ничего не завелось. Я получил краш приложения с уже знакомым исключением IllegalStateException: FragmentManager is already executing transactions. Дебаггер указывал на код, связанный с настройкой нижней навигации, поэтому я решил просто обернуть эту настройку в очередной Handler.post:


// MainFragment.kt  fragment with BottomNavigationViewoverride fun onViewCreated(view: View, savedInstanceState: Bundle?) {    super.onViewCreated(view, savedInstanceState)    if (savedInstanceState == null) {        safeSetupBottomNavigationBar()    }}private fun safeSetupBottomNavigationBar() {    Handler().post {        setupBottomNavigationBar()    }}

Это исправило краш, но приложение всё равно работало неправильно: запустив диплинк, мы пропустили Splash-экран, он просто не запускался. А это означает, что не отрабатывал код, который отвечал за инициализацию моего приложения.


Это произошло, потому что в нашем случае путь диплинка был таким: мы запустили приложение, запустилась его единственная Activity. В вёрстке этой activity мы инициализировали первый граф навигации. В этом графе оказался элемент, который удовлетворял URI, мы отправили его через adb-команду вуаля, он сразу и открылся, проигнорировав указанный в графе startDestination.


Тогда я решил перенести диплинк в другой граф внутрь вкладки нижней навигации.


<navigation xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    android:id="@+id/menu__search"    app:startDestination="@id/SearchContainerFragment">    <fragment        android:id="@+id/SearchContainerFragment"        android:name="tabs.search.SearchContainerFragment">        <deepLink app:uri="www.example.com/main" />        <action            android:id="@+id/action__SearchContainerFragment__to__CompanyFlow"            app:destination="@id/company_flow__nav_graph" />        <action            android:id="@+id/action__SearchContainerFragment__to__VacancyFragment"            app:destination="@id/vacancy_nav_graph" />    </fragment></navigation>

И, запустив приложение, я получил ЭТО:


Посмотреть на ЭТО


На гифке видно, как приложение запустилось, и мы увидели Splash-экран. После этого на мгновение показался экран с нижней навигацией, а затем приложение словно запустилось заново! Мы снова увидели Splash-экран, и только после его повторного прохождения появилась нужная вкладка нижней навигации.


И что самое неприятное во всей этой истории это не баг, а фича.


Если почитать внимательно документацию про работу с диплинками в Navigation Component, можно найти следующий кусочек:


When a user opens your app via an explicit deep link, the task back stack is cleared and replaced with the deep link destination.

То есть наш back stack специально очищается, чтобы Navigation Component-у было удобнее работать с диплинками. Говорят, что когда-то давно, в бета-версии библиотеки всё работало адекватнее.


Мы можем это исправить. Корень проблемы в методе handleDeepLink NavController-а:


Кусочек handleDeepLink
public void handleDeepLink(@Nullable Intent intent) {    // ...    if ((flags & Intent.FLAG_ACTIVITY_NEW_TASK) != 0) {        // Start with a cleared task starting at our root when we're on our own task        if (!mBackStack.isEmpty()) {            popBackStackInternal(mGraph.getId(), true);        }        int index = 0;        while (index < deepLink.length) {            int destinationId = deepLink[index++];            NavDestination node = findDestination(destinationId);            if (node == null) {                final String dest = NavDestination.getDisplayName(mContext, destinationId);                throw new IllegalStateException("Deep Linking failed:"                        + " destination " + dest                        + " cannot be found from the current destination "                        + getCurrentDestination());            }            navigate(node, bundle,                    new NavOptions.Builder().setEnterAnim(0).setExitAnim(0).build(), null);        }        return true;    }}

Чтобы переопределить это поведение, нам потребуется:


  • почти полностью скопировать к себе исходный код Navigation Component;
  • добавить свой собственный NavController с исправленной логикой (добавление исходного кода библиотеки необходимо, так как от NavController-а зависят практически все элементы библиотеки) назовём его FixedNavController;
  • заменить все использования исходного NavController-а на FixedNavController.

Несложно, правда? Но кажется, что это уже слишком. Подобные исправления заставляют нас в будущем тратить всё больше и больше времени на поддержку нашего кастомного решения, ведь нужно будет обновлять свой фикс при каждом обновлении библиотеки. Около года назад коллеги из Яндекса столкнулись с похожей проблемой, и им пришлось делать примерно то же самое. Сочувствую.


В этот невесёлый момент я заметил ещё один баг, который был добавлен при попытке исправить краш с диплинками: сломалась обратная навигация из auth-флоу.


Покажи гифку


На гифке видно, как мы переключаемся на вкладку Profile, затем переходим во флоу авторизации. Потом мы нажимаем на кнопку Back и получаем просто белый экран. Если переключиться на какую-нибудь другую вкладку и обратно, то мы снова увидим контент вкладки Profile.


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



Если у вас будет свой собственный NavController, корректно обрабатывающий диплинки, реализовать этот кейс будет просто.


В NavController-е есть специальный булевский флажок isDeepLinkHandled, который говорит нам, что текущий NavController успешно обработал диплинк. Вы могли бы добавить диплинк, ведущий на фрагмент, который содержит в себе ViewPager, затем написать примерно вот такой код, чтобы перейти на нужную вкладку:


if (findMyNavController().isDeepLinkHandled && requireActivity().intent.data != null) {    val uriString = requireActivity().intent.data?.toString()    val selectedPosition = when {        uriString == null -> 0        uriString.endsWith("favorites") -> 0        uriString.endsWith("subscribes") -> 1        else -> 2    }    fragment_favorites_container__view_pager.setCurrentItem(selectedPosition, true)}

Но, опять же, это будет доступно только в случае, если вы уже добавили к себе в кодовую базу свою реализацию NavController-а, ведь флаг isDeepLinkHandled является private-полем. Ок, можно достучаться до него через механизм reflection-а, но это уже другая история.



Navigation Component не поддерживает диплинки с условием из коробки. Если вы хотите поддержать такое поведение, Google предлагает действовать следующим образом:


  • через диплинк открыть экран, который требует авторизацию;
  • на этом экране проверить, авторизован ли пользователь, если нет открыть флоу авторизации поверх нужного экрана;
  • пройти auth flow, вернуть результат из вложенного графа и т.д., и т.п.

Возможности глобально решить мою задачу средствами Navigation Component-а я не нашёл.


Выводы по работе с диплинками в Navigation Component


  • Работать с ними больно, если требуется добавлять дополнительные действия или условия.
  • Объявлять диплинки ближе к месту их назначения классная идея, в разы удобнее AndroidManifest-а со списком поддерживаемых ссылок.

Бонус-секция кейсы БЕЗ проблем


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



Допустим, у вас есть экран вакансий, с которого вы можете перейти на другую вакансию.


Где на схеме приложения этот кейс?


По опыту предыдущих кейсов я ожидал проблем и здесь, но всё оказалось достаточно просто я определил навигацию с экрана на экран:


<fragment  android:id="@+id/VacancyFragment"  android:name="com.aaglobal.jnc_playground.ui.vacancy.VacancyFragment"  android:label="Fragment vacancy"  tools:layout="@layout/fragment_vacancy">  <argument      android:name="vacancyId"      app:argType="string"      app:nullable="false" />  <action      android:id="@+id/action__VacancyFragment__to__VacancyFragment"      app:destination="@id/VacancyFragment" /></fragment>

И этого оказалось достаточно новый экран открывался поверх старого, при нажатии на кнопку Back навигация была корректной. Если бы я захотел, чтобы каждый новый экран открывался вместо текущего, было бы достаточно добавить атрибут popUpTo к моему action-у.



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


Где на схеме приложения этот кейс?


Я добавил контейнер для будущего фрагмента со списком в вёрстку вкладки нижней навигации:


<LinearLayout xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    android:layout_width="match_parent"    android:layout_height="match_parent"    android:orientation="vertical">    <TextView        android:id="@+id/fragment_favorites_container__text__title"        style="@style/LargeTitle"        android:text="Favorites container" />    <androidx.fragment.app.FragmentContainerView        android:id="@+id/fragment_favorites_container__container__recommend_vacancies"        android:layout_width="match_parent"        android:layout_height="match_parent" /></LinearLayout>

А затем в runtime-е добавил нужный мне фрагмент в этот контейнер:


class FavoritesContainerFragment : Fragment(R.layout.fragment_favorites_container) {    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {        super.onViewCreated(view, savedInstanceState)        childFragmentManager.attachFragmentInto(          containerId = R.id.fragment_container_view,          fragment = createVacancyListFragment()        )    }}

Метод attachFragmentInfo на childFragmentManager это extension-метод, который просто оборачивает всю работу с транзакциями, не более того.


А вот как я создал фрагмент:


class FavoritesContainerFragment : Fragment(R.layout.fragment_favorites_container) {    // ...    private fun createVacancyListFragment(): Fragment {        return VacancyListFragment.newInstance(          vacancyType = "favorites_container",          vacancyListRouterSource = object : VacancyListRouterSource {              override fun navigateToVacancyScreen(item: VacancyItem) {                  findNavController().navigate(                      R.id.action__FavoritesContainerFragment__to__VacancyFragment,                      VacancyFragmentArgs(vacancyId = "${item.name}|${item.id}").toBundle()                  )              }        }     }}

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



Пусть у меня есть несколько BottomSheetDialog-ов, между которыми я хочу перемещаться с помощью Navigation Component.


Где на схеме приложения этот кейс?


Год назад с таким кейсом были какие-то проблемы, но сейчас всё работает как надо. Можно легко объявить какой-то dialog в качестве destination-а в вашем графе навигации, можно добавить action для открытия диалога из другого диалога.


<navigation xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    android:id="@+id/menu__favorites"    app:startDestination="@id/FavoritesContainerFragment">   <dialog        android:id="@+id/ABottomSheet"        android:name="ui.dialogs.dialog_a.ABottomSheetDialog">        <action            android:id="@+id/action__ABottomSheet__to__BBottomSheet"            app:destination="@id/BBottomSheet"            app:popUpTo="@id/ABottomSheet"            app:popUpToInclusive="true" />    </dialog>    <dialog        android:id="@+id/BBottomSheet"        android:name="ui.dialogs.dialog_b.BBottomSheetDialog">        <action            android:id="@+id/action__BBottomSheet__to__ABottomSheet"            app:destination="@id/ABottomSheet"            app:popUpTo="@id/BBottomSheet"            app:popUpToInclusive="true" />    </dialog></navigation>

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


Выводы по бонус-секции


Кейсы без проблем существуют.


Подведём итоги


На данный момент нет никакой причины переводить большое приложение на Navigation Component. Слишком много проблем, слишком много костылей, постоянно нужно выдумывать что-то для осуществления не самых сложных кейсов навигации. Сам факт, что я ухитрился написать так много текста про проблемы с Navigation Component-ом, что-то да говорит.


Если у вас маленькое приложение, надо внимательно посмотреть на то, какие кейсы навигации вы хотите поддержать. Если задумываетесь про нижнюю навигацию, про диплинки пожалуй, лучше реализовать всё по-старинке: либо руками, либо на Cicerone. В остальных случаях, если вас не пугает необходимость постоянно искать фиксы, можно воспользоваться Navigation Component-ом.


Пример приложения на Github-е лежит здесь.


Полезные ссылки по теме


Подробнее..

Как исправить баг с Drawable.setTint в API 21 Android SDK

09.11.2020 18:08:51 | Автор: admin

Привет, в этой заметке наш android-разработчик Влад Титов расскажет о том, как решить проблему с использованием инструмента изменения цвета для Drawable. Поехали.

В 21 версии API Android SDK появился универсальный инструмент изменения цвета для всех Drawable - Drawable.setTint(int color). Но как раз-таки в этой самой версии он не работает у некоторых наследников Drawable, а именно GradientDrawable, InsetDrawable, RippleDrawable и всех наследников DrawableContainer.

Если посмотреть в исходники API 21, скажем, GradientDrawable (прямого наследника Drawable), мы не найдем переопределенного метода setTint и его вариаций. А это значит, что в данной реализации разработчики попросту не поддержали эту функцию.

Проблему условно решили в библиотеке обратной совместимости. Сейчас ее можно найти по артефакту androidx.core:core. Чтобы поддержать tinting на версиях 14-22, были созданы обертки WrappedDrawableApi14 и WrappedDrawableApi21. Последняя является наследницей первой и, по сути, не несет логики по поддержке окрашивания.

Чтобы обернуть оригинальный Drawable, нужно всего лишь подать его в метод DrawableCompat.wrap(Drawable). Основная идея состоит в том, что сам ColorStateList тинта хранится в обертках, а у оригинального Drawable изменяется цветовой фильтр при изменении состояния Drawable.

final ColorStateList tintList = mState.mTint;final PorterDuff.Mode tintMode = mState.mTintMode;if (tintList != null && tintMode != null) {   final int color = tintList.getColorForState(state, tintList.getDefaultColor());   if (!mColorFilterSet || color != mCurrentColor || tintMode != mCurrentMode) {       setColorFilter(color, tintMode);       mCurrentColor = color;       mCurrentMode = tintMode;       mColorFilterSet = true;       return true;   }} else {   mColorFilterSet = false;   clearColorFilter();}

Данный кусок кода будет вызываться каждый раз при вызове Drawable.setState(int[] stateSet).

При использовании этих оберток вы теряете возможность вызывать специфические методы для конкретных Drawable. Так, например, при оборачивании GradientDrawable вы не сможете управлять градиентом, так как обертка в своем интерфейсе не имеет методов таких, как setShape, setGradientType и.т.п. Чтобы получить доступ к данным методам, обернутый Drawable придется развернуть (DrawableCompat.unwrap(Drawable)). Но в таком случае вы теряете тинт. Если он у вас состоял только из одного цвета, ничего страшного, ведь этот цвет сохранится как цветовой фильтр в оригинальном Drawable. Но если тинт был stateful, цвета для стейтов, отличных от текущего, будут потеряны.

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

Если ваш тинт состоит лишь из одного цвета, вы можете в любой момент выполнить следующие действия:

val wrapped = DrawableCompat.wrap(drawable)wrapped.setTint(...)drawable = DrawableCompat.unwrap(wrapped)

После чего смело делать дальше свои дела.

В ином случае есть смысл воспользоваться следующим решением:

class GradientDrawableWrapper(    val original: GradientDrawable,     var ColorStateList tint) {    fun get(): Drawable {        return wrap()    }    fun setShape(@Shape shape: Int) {        original.setShape(shape)    }    // other specific method proxies...    private fun wrap(): Drawable {        val wrapped = DrawableCompat.wrap(original)        wrapped.setTint(tint)        return wrapped    }}

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

Подробнее..

Navigation Component-дзюцу, vol. 1 BottomNavigationView

09.09.2020 12:15:30 | Автор: admin


Два года назад на Google I/O Android-разработчикам представили новое решение для навигации в приложениях библиотеку Jetpack Navigation Component. Про маленькие приложения уже было сказано достаточно, а вот о том, с какими проблемами можно столкнуться при переводе большого приложения на Navigation Component, информации практически нет.


В этой и следующих двух статьях я расскажу о кейсах, с которыми может встретиться разработчик, желающий опробовать Navigation Component в большом Android-приложении.


Это текстовая версия моего выступления в рамках серии митапов по Android 11 в Android Academy. Само выступление было на английском, статью пишу на русском. Кому удобнее смотреть велкам.


В первой статье я расскажу о кейсах, связанных с BottomNavigationView, во второй о кейсах с вложенными графами навигации, в третьей про навигацию в многомодульных приложениях, дип линки, встраиваемые фрагменты и диалоги. Все три статьи лонгриды, которые, однако, способны сэкономить много времени и вам, и вашей команде.


Disclaimer


Я сделал пример, по структуре навигации повторяющий основные моменты навигации соискательского приложения hh.ru, и выхватил ряд проблем, о которых и собираюсь рассказать. Я основательно поресёрчил практическую сторону вопроса, но, разумеется, рассмотрел далеко не все возможные кейсы.


Схема моего тестового приложения выглядит так:



В цикле статей мы разберём каждый переход, который описан на этой схеме, а также несколько кейсов, которые не поместились на картинку.


Кейсы с BottomNavigationView


Когда я только-только услышал про Navigation Component, мне стало интересно: как будет работать BottomNavigationView и как Google подружит несколько отдельных back stack-ов в разных вкладках. Два года назад с этим кейсом были некоторые проблемы, и я решил проверить, как там обстоят дела сегодня.


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


Где на схеме приложения кейсы с навигацией?


Первый опыт


Я установил Android Studio 4.1 Beta (последнюю более-менее стабильную версию на тот момент) и попробовал шаблон приложения с нижней навигацией. Начало было многообещающим.


  • Мне сгенерировали Activity в качестве контейнера для хоста навигации и нижней навигации

Вёрстка Activity из шаблона
<androidx.constraintlayout.widget.ConstraintLayout    android:id="@+id/container">    <com.google.android.material.bottomnavigation.BottomNavigationView        android:id="@+id/nav_view"        app:menu="@menu/bottom_nav_menu" />    <fragment        android:id="@+id/nav_host_fragment"        android:name="androidx.navigation.fragment.NavHostFragment"        app:defaultNavHost="true"        app:navGraph="@navigation/mobile_navigation" /></androidx.constraintlayout.widget.ConstraintLayout>

Я убрал шумовые атрибуты, чтобы было проще читать.


Стандартный ConstraintLayout, в который добавили BottomNavigationView и тэг <fragment> для инициализации NavHostFragment-а (Android Studio, кстати, подсвечивает, что вместо фрагмента лучше использовать FragmentContainerView).


  • Для каждой вкладки BottomNavigationView был создан отдельный фрагмент

Граф навигации из шаблона
<navigation    android:id="@+id/mobile_navigation"    app:startDestination="@+id/navigation_home">    <fragment        android:id="@+id/navigation_home"        android:name="com.aaglobal.graph_example.ui.home.HomeFragment"/>    <fragment        android:id="@+id/navigation_dashboard"        android:name="com.aaglobal.graph_example.ui.dashboard.DashboardFragment"/>    <fragment        android:id="@+id/navigation_notifications"        android:name="com.aaglobal.graph_example.ui.notifications.NotificationsFragment"/></navigation>

Все фрагменты были добавлены в качестве отдельных destination-ов в общий граф навигации.


  • А ещё в проект был добавлен файл-ресурс для описания меню BottomNavigationView

@menu-ресурс для описания табов
<menu xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android">    <item        android:id="@+id/navigation_home"        android:icon="@drawable/ic_home_black_24dp"        android:title="@string/title_home" />    <item        android:id="@+id/navigation_dashboard"        android:icon="@drawable/ic_dashboard_black_24dp"        android:title="@string/title_dashboard" />    <item        android:id="@+id/navigation_notifications"        android:icon="@drawable/ic_notifications_black_24dp"        android:title="@string/title_notifications" /></menu>

При этом я заметил, что идентификаторы элементов меню должны совпадать с идентификаторами destination-ов в графе навигации. Не самая очевидная связь между табами BottomNavigationView и фрагментами, но работаем с тем, что есть.


Пора запускать приложение


После создания приложения из шаблона я запустил его и сразу столкнулся с двумя проблемами.


Первая проблема: при переключении между вкладками их состояние не сохранялось.


А ну-ка покажи


Для проверки я добавил во вкладку Dashboard простенькую ViewModel со счётчиком. На гифке видно, как я переключаюсь со вкладки Home на вкладку Dashboard, увеличиваю счётчик до четырёх. После этого я переключился обратно на вкладку Home и вновь вернулся на Dashboard. Счётчик сбросился.


Баг с описанием этой проблемы уже два года висит в Issue Tracker-е. Чтобы решить её, Google-у потребовалось серьёзно переработать внутренности фреймворка Fragment-ов, чтобы поддержать возможность работать с несколькими back stack-ами одному FragmentManager-у. Недавно на Medium вышла статья Ian Lake, в которой он рассказывает, что Google серьёзно продвинулись в этом вопросе, так что, возможно, фикс проблемы с BottomNavigationView не за горами.


Вторая проблема следствие первой. По принятым принципам навигации, когда вы нажимаете на уже выбранную вкладку BottomNavigationView, вы должны вернуться на первый фрагмент в стеке текущей вкладки. Но когда это происходит, состояние этого первого фрагмента сбрасывается, так как сам фрагмент пересоздаётся.


А ну-ка покажи


Для демонстрации этой проблемы я добавил на вкладку Dashboard кнопку, которая ведёт на следующий экран. На гифке видно, как я переключаюсь на вкладку Dashboard, увеличиваю счётчик до трёх, а затем перехожу на экран Graphic. Если я нажимаю на кнопку Back то всё работает как надо, состояние вкладки не сбрасывается. Но если, находясь на экране Graphic, я ещё раз нажму на вкладку Dashboard, то после возврата на первый экран в стеке увижу, что его состояние сброшено.


Не самое лучшее первое впечатление, подумал я. И начал искать фикс.


У нас есть workaround


Решение этих проблем живёт в специальном репозитории Google-а с примерами работы с Architecture Components, в проекте NavigationAdvancedSample.


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


  • Во-первых, для каждой вкладки вводится отдельный, независимый граф навигации

Граф навигации для одной из вкладок
<?xml version="1.0" encoding="utf-8"?><navigation xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    xmlns:app="http://personeltest.ru/away/schemas.android.com/apk/res-auto"    xmlns:tools="http://personeltest.ru/away/schemas.android.com/tools"    android:id="@+id/navigation_home"    app:startDestination="@id/HomeFragment">    <fragment        android:id="@+id/HomeFragment"        android:name="com.aaglobal.jnc_playground.ui.home.HomeFragment"        android:label="@string/title_home"        tools:layout="@layout/fragment_home" /></navigation>

Соответственно, для примера BottomNavigationView с тремя вкладками у нас получится три отдельных файла навигации XML, в которых в качестве startDestination будут указаны первые фрагменты вкладок.


  • Во-вторых, для каждой вкладки под капотом создаётся отдельный NavHostFragment, который будет связан с графом навигации этой вкладки

Создание NavHostFragment-а для графа вкладки BottomNavigationView
private fun obtainNavHostFragment(    fragmentManager: FragmentManager,    fragmentTag: String,    navGraphId: Int,    containerId: Int): NavHostFragment {    // If the Nav Host fragment exists, return itval existingFragment = fragmentManager.findFragmentByTag(fragmentTag) as NavHostFragment?    existingFragment?.let { return it }    // Otherwise, create it and return it.    val navHostFragment = NavHostFragment.create(navGraphId)    fragmentManager.beginTransaction()        .add(containerId, navHostFragment, fragmentTag)        .commitNow()    return navHostFragment}

FragmentManager пока что не поддерживает работу с множеством back stack-ов одновременно, поэтому пришлось придумать альтернативное решение, которое позволило ассоциировать с каждым графом свой back stack. Им стало создание отдельного NavHostFragment-а для каждого графа. Из этого следует, что с каждой вкладкой BottomNavigationView у нас будет связан отдельный NavController.


  • В-третьих, мы устанавливаем в BottomNavigationView специальный listener, который будет заниматься переключением между back stack-ами фрагментов

Listener для переключения между вкладками BottomNavigationView
setOnNavigationItemSelectedListener { item ->  val newlySelectedItemTag = graphIdToTagMap[item.itemId]  if (selectedItemTag != newlySelectedItemTag) {    fragmentManager.popBackStack(firstFragmentTag, FragmentManager.POP_BACK_STACK_INCLUSIVE)    val selectedFragment = fragmentManager.findFragmentByTag(newlySelectedItemTag)        as NavHostFragment    if (firstFragmentTag != newlySelectedItemTag) {      fragmentManager.beginTransaction()        .attach(selectedFragment)        .setPrimaryNavigationFragment(selectedFragment).apply {          graphIdToTagMap.forEach { _, fragmentTagIter ->            if (fragmentTagIter != newlySelectedItemTag) {              detach(fragmentManager.findFragmentByTag(firstFragmentTag)!!)            }          }        }        .addToBackStack(firstFragmentTag)        .setReorderingAllowed(true)        .commit()    }    selectedNavController.value = selectedFragment.navController    true  } else {    false  }}

В прикреплённом кусочке кода мы видим, как при переключении между вкладками BottomNavigationView выполняется специальная транзакция в FragmentManager-е, которая прикрепляет фрагмент выбранной вкладки и отцепляет все остальные фрагменты. По сути, так мы и переключаемся между различными back stack-ами.


  • В итоге метод настройки BottomNavigationView возвращает разработчику специальную LiveData, которая содержит в себе NavController выбранной вкладки. Этот NavController можно использовать, например, для обновления надписи на ActionBar

Настраиваем BottomNavigationView в Activity
class RootActivity : AppCompatActivity(R.layout.activity_root) {  private var currentNavController: LiveData<NavController>? = null  private fun setupBottomNavigationBar() {      // Setup the bottom navigation view with a list of navigation graphs      val liveData = bottom_nav.setupWithNavController(          navGraphIds = listOf(            R.navigation.home_nav_graph,            R.navigation.dashboard_nav_graph,            R.navigation.notifications_nav_graph          ),          fragmentManager = supportFragmentManager,          containerId = R.id.nav_host_container,          intent = intent      )      // Whenever the selected controller changes, setup the action bar.      liveData.observe(this, Observer { ctrl -> setupActionBarWithNavController(ctrl) })      currentNavController = liveData  }}

Метод для настройки BottomNavigationView вызывают в onCreate-е, когда Activity создаётся в первый раз, затем в методе onRestoreInstanceState, когда Activity пересоздаётся с помощью сохранённого состояния.


Кроме того, для работы фикса нужно, чтобы идентификаторы элементов меню, которое используется для инициализации табов BottomNavigationView, совпадали с идентификаторами графов навигации.


Посмотреть, как это выглядит в коде


Опять же, не самая очевидная связь между этими элементами, зато работает.


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


А ну-ка покажи

Первая проблема решилась:



И вторая тоже:



Адаптация workaround-а для фрагментов


Очень здорово, что workaround помог решить основную проблему с сохранением состояний, но этих фиксов мне было недостаточно. Пример из проекта NavigationAdvancedSample использовал в качестве контейнера нижней навигации Activity, мне же нужен был фрагмент.


Почему тебе нужен фрагмент?

Посмотрите внимательно на эту схему:



На ней можно увидеть, что пользователь начинает свой путь в приложении со Splash-экрана:



Google говорит, что Splash-экраны зло, ухудшающее UX приложения. Тем не менее, Splash-экраны суровая реальность большинства крупных Android-приложений. И если мы хотим использовать в нашем приложении Single Activity-архитектуру, то в качестве контейнера нижней навигации придётся использовать Fragment, а не Activity:



Я добавил вёрстку для фрагмента с нижней навигацией и перенёс настройку BottomNavigationView во фрагмент:


Посмотреть код
class MainFragment : Fragment(R.layout.fragment_main) {    private var currentNavController: LiveData<NavController>? = null    override fun onViewStateRestored(savedInstanceState: Bundle?) {        super.onViewStateRestored(savedInstanceState)        setupBottomNavigationBar()    }    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {        super.onViewCreated(view, savedInstanceState)        if (savedInstanceState == null) {            setupBottomNavigationBar()        }    }}

Я добавил в свой пример Splash-экран и дополнительную вкладку для BottomNavigationView. А чтобы пример стал ещё более походить на приложение для соискателей hh.ru, я также убрал из него ActionBar.


Для этого я поменял тему приложения с Theme.MaterialComponents.DayNight.DarkActionBar на Theme.MaterialComponents.DayNight.NoActionBar и убрал код для связки NavController-а с ActionBar-ом:


Код настройки BottomNavigationView выглядел так
class MainFragment : Fragment(R.layout.fragment_main) {    private var currentNavController: LiveData<NavController>? = null    private fun setupBottomNavigationBar() {        val navGraphIds = listOf(            R.navigation.search__nav_graph,            R.navigation.favorites__nav_graph,            R.navigation.responses__nav_graph,            R.navigation.profile__nav_graph        )        val controller = bottom_navigation.setupWithNavController(            navGraphIds = navGraphIds,            fragmentManager = requireActivity().supportFragmentManager,            containerId = R.id.fragment_main__nav_host_container,            intent = requireActivity().intent        )        currentNavController = controller    }}

После всех манипуляций я включил режим Don't keep activities, запустил свой пример и получил краш при сворачивании приложения.


А ну-ка покажи


На гифке видно, как я запустил приложение, и после Splash-экрана показывается экран с нижней навигацией. После этого мы сворачиваем приложение и получаем краш.


В чём была причина? При вызове onDestroyView активный NavHostFragment пытается отвязаться от NavController-а. Так как мой фрагмент-контейнер с нижней навигацией никак не привязывал к себе NavController, который он получил из LiveData, метод Navigation.findNavController из onDestroyView крашил приложение.


Добавляем привязку NavController-а к фрагменту с нижней навигацией (для этого в Navigation Component-е есть утилитный метод Navigation.setViewNavController), и проблема исчезает.


Кусочек кода с фиксом
class MainFragment : Fragment(R.layout.fragment_main) {    private var currentNavController: LiveData<NavController>? = null    private fun setupBottomNavigationBar() {        ...        currentNavController?.observe(            viewLifecycleOwner,            Observer { liveDataController ->                Navigation.setViewNavController(requireView(), liveDataController)            }        )    }}

Но это ещё не всё. Не выключая режим Don't keep activities, я попробовал свернуть, а затем развернуть приложение. Оно снова упало, но с другим неприятным исключением IllegalStateException в FragmentManager FragmentManager already executing transactions.


А ну-ка покажи


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


Краш происходит в методах, которые прикрепляют NavHostFragment к FragmentManager-у после их создания. Это исключение можно исправить при помощи костыля: обернуть методы attach-detach в Handler.post {}.


Фиксим IllegalStateException
// NavigationExtensions.ktprivate fun attachNavHostFragment(    fragmentManager: FragmentManager,    navHostFragment: NavHostFragment,    isPrimaryNavFragment: Boolean) {  Handler().post {    fragmentManager.beginTransaction()    .attach(navHostFragment)    .apply {      if (isPrimaryNavFragment) {        setPrimaryNavigationFragment(navHostFragment)      }    }    .commitNow()  }}

После добавления Handler.post приложение заработало, как надо.


Выводы по работе с BottomNavigationView


  • Использовать BottomNavigationView в связке с Navigation Component можно, если знать, где искать workaround-ы.
  • Если вы захотите иметь фрагмент в качестве контейнера нижней навигации BottomNavigationView, будьте готовы искать дополнительные фиксы для ваших проблем, так как скорее всего я поймал не все возможные краши.

На этом с BottomNavigationView всё, на следующей неделе расскажу про кейсы с вложенными графами навигации.

Подробнее..

Navigation Component-дзюцу, vol. 2 вложенные графы навигации

16.09.2020 12:14:05 | Автор: admin


Каждое большое приложение содержит множество способов навигации между экранами. А хорошая библиотека навигации должна помогать разработчику их реализовывать. Именно с такой мыслью я подошёл к исследованию кейсов со вложенными графами навигации.


Это вторая из трёх статей про реализацию кейсов навигации при помощи Navigation Component-а.


Первая статья про BottomNavigationView.


Где на схеме приложения кейсы со вложенными графами?



В этом кейсе мы говорим про следующую часть общей схемы навигации:



Представим такую ситуацию: у нас есть 4 экрана A, B, C и D. Пусть с экранов A и B вы можете перейти на экран C, с экрана C в экран D, а после D вернуться на тот экран, который начал флоу C->D.


А можно нагляднее?

В тестовом приложении, которое я приготовил для разбора Navigation Component-а, есть две вкладки BottomNavigationView (на схеме это Search и Responses но пусть они будут экранами A и B):



С обеих этих вкладок мы можем перейти на некоторый вложенный флоу, который состоит из двух экранов (C и D):



Если мы перейдём на экран C с вкладки Search (экрана A), то после экрана D мы должны вернуться на вкладку Search:



А если мы стартуем экран C со вкладки Responses, то после завершения внутреннего флоу C->D мы должны вернуться на вкладку Responses:



Этот кейс описывает старт последовательности экранов из разных мест приложения, а после её завершения возврат на тот экран, который начал эту последовательность.


Как это реализовать? Для начала вы объявляете вашу вложенную последовательность экранов в отдельном XML-файле навигации, чтобы можно было вкладывать её в другие графы:


Объявление графа вложенной навигации
<!-- company_flow__nav_graph.xml --><navigation    android:id="@+id/company_flow__nav_graph"    app:startDestination="@id/CompanyFragment">    <fragment        android:id="@+id/CompanyFragment"        android:name="ui.company.CompanyFragment">        <action            android:id="@+id/action__CompanyFragment__to__CompanyDetailsFragment"            app:destination="@id/CompanyDetailsFragment" />    </fragment>    <fragment        android:id="@+id/CompanyDetailsFragment"        android:name="ui.company.CompanyDetailsFragment"/></navigation>

Затем следует вложить созданный граф навигации в уже существующий граф и использовать идентификатор вложенного графа для описания action-ов:


Добавление графа навигации в другой граф
<navigation    android:id="@+id/menu__search"    app:startDestination="@id/SearchContainerFragment">    <fragment        android:id="@+id/SearchContainerFragment"        android:name="ui.tabs.search.SearchContainerFragment">        <action            android:id="@+id/action__SearchContainerFragment__to__CompanyFlow"            app:destination="@id/company_flow__nav_graph" />    </fragment>    <include app:graph="@navigation/company_flow__nav_graph" /></navigation>

Итак, вы описали навигацию из двух разных мест приложения во вложенный флоу. Но что делать с возвратом?


Проблема в том, что Navigation Component не позволяет нормально описывать навигацию НАЗАД, только навигацию ВПЕРЁД. Но при этом даёт возможность описывать удаление экранов из back stack-а при помощи атрибутов popBackUp и popBackUpInclusive в XML, а также при помощи функции popBackStack в NavController-е.


Пока размышлял над этим, заметил интересную вещь: я подключился дебаггером перед переходом с экрана Splash на экран с нижней навигацией и обнаружил, что поле mBackStack внутри NavController-а Splash-экрана содержит два объекта NavBackStackEntry.


А можно на картинке?


Честно говоря, я не ожидал увидеть там два объекта, поскольку в back stack-е фрагментов точно был только один SplashFragment. Откуда взялась вторая сущность? Оказалось, что первый объект представляет собой NavGraph, который запустился в моей корневой Activity, а второй объект мой SplashFragment, который представлен классом FragmentNavigator.Destination.


И тут у меня появилась идея а что если вызвать на NavController-е функцию popBackStack и передать туда идентификатор графа? Коль скоро граф находится в back stack-е NavController-а, это должно удалить все экраны, которые были добавлены в рамках этого графа.


И эта идея сработала.


Возврат из flow при помощи popBackStack
class CompanyDetailsFragment : Fragment(R.layout.fragment_company_details) {    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {        super.onViewCreated(view, savedInstanceState)        finish_flow_button.setOnClickListener {            findNavController().popBackStack(R.id.company_flow__nav_graph, true)        }    }}

Минус такого подхода к определению обратной навигации очевиден: эта навигация не отобразится в визуальном редакторе. Конечно, можно определить action в XML-е вот таким образом:


Определение action-а для закрытия графа навигации
<fragment  android:id="@+id/CompanyDetailsFragment"  android:name="ui.company.CompanyDetailsFragment"  android:label="@string/fragment_company_details__title"  tools:layout="@layout/fragment_company_details">  <action      android:id="@+id/action__finishCompanyFlow"      app:popUpTo="@id/company_flow__nav_graph"      app:popUpToInclusive="true" /></fragment>

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


findNavController().navigate(R.id.action__finishCompanyFlow)

Но есть в этом что-то семантически неправильное: странно использовать слово navigate для закрытия экранов и обратной навигации.


Возврат результата из вложенного флоу


Что ж, мы получили некоторое подобие обратной навигации. Но возникает ещё один вопрос: есть ли способ вернуть из вложенного флоу какой-нибудь результат?


Да, есть. В Navigation Component 2.3 Google представил нам специальное key-value хранилище для проброса результатов с других экранов SavedStateHandle. К этому хранилищу можно получить доступ через свойства NavControllerpreviousBackStackEntry и currentBackStackEntry. Но в своих примерах Google почему-то считает, что ваш вложенный флоу всегда состоит только из одного экрана.


Типичный пример работы с SavedStateHandle
// Flow screenfindNavController().previousBackStackEntry    ?.savedStateHandle    ?.set("some_key", "value")// Screen that waits resultval result = findNavController().currentBackStackEntry    ?.savedStateHandle    ?.remove<String>("some_key")

Что делать, если вложенный флоу состоит из нескольких экранов? Вы не можете использовать previousBackStackEntry для доступа к SavedStateHandle, потому что в этом случае вы положите данные в один из экранов вашего вложенного флоу. Для решения этой проблемы можно воспользоваться следующим фиксом:


Посмотреть на фикс
fragment_company_details__button.setOnClickListener {    // Here we are inside nested navigation flow    findNavController().popBackStack(R.id.company_flow__nav_graph, true)    // At this line, "findNavController().currentBackStackEntry" means    // screen that STARTED current nested flow.    // So we can send the result!    findNavController().currentBackStackEntry      ?.savedStateHandle      ?.set(COMPANY_FLOW_RESULT_FLAG, true)}

Суть в следующем: до вызова findNavController().popBackStack вы находитесь ВНУТРИ вашего флоу экранов, а вот сразу после вызова popBackStack уже на экране, который НАЧАЛ ваш флоу! И это означает, что вы можете использовать для доступа к SavedStateHandle свойство currentBackStackEntry. Этот entry будет означать ваш стартовый экран, которому нужен результат из флоу.


В свою очередь, на на экране, который начал вложенный флоу, вы тоже используете currentBackStackEntry для доступа к SavedStateHandle. И, следовательно, читаете правильные данные:


Читаем данные из SavedStateHandle
// Read result from nested navigation flowval companyFlowResult = findNavController().currentBackStackEntry    ?.savedStateHandle    ?.remove<Boolean>(CompanyDetailsFragment.COMPANY_FLOW_RESULT_FLAG)text__company_flow_result.text = "${companyFlowResult}"

Выводы по работе с вложенным флоу


  • Для обратной навигации из вложенного флоу, состоящего из нескольких экранов, можно использовать функцию NavController.popBackStack, передав туда идентификатор графа навигации вашего флоу.
  • Для проброса какого-либо результата из вложенного флоу можно использовать SavedStateHandle.


Сейчас будет немного терминологии, чтобы синхронизировать наше понимание по поводу графов навигации.


Пусть у нас два графа навигации граф A и граф B. Я буду называть граф B вложенным в граф A, если мы вкладываем его через include. И, наоборот, я буду называть граф A внешним по отношению к графу B, если граф А включает в себя граф B.


Ещё немного картинок

Граф B вложенный в граф A:



Граф А внешний по отношению к графу B:



А теперь давайте разберём кейс навигации из вложенного графа во внешний граф.



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


Что? В смысле, это тот самый первый кейс, который ты уже разобрал? Разве вы не заметили, что у этой последовательности НЕТ нижней навигации?


Приблизить картинку

Смотрите, вот экран с нижней навигацией:



А вот последовательность экранов без неё:



Мы хотим открыть последовательность экранов поверх имеющегося. Если мы просто вложим Auth-граф в граф навигации, относящийся к вкладке нижней навигации, то мы получим не тот результат, который хотим: экраны Auth-графа будут иметь нижнюю навигацию.


Неправильный подход к такой навигации

Пусть мы вставили граф auth flow-навигации в наш граф вкладки нижней навигации и добавили action для перехода в него:


<navigation xmlns:android="http://personeltest.ru/away/schemas.android.com/apk/res/android"    android:id="@+id/menu__profile"    app:startDestination="@id/ProfileContainerFragment">    <fragment        android:id="@+id/ProfileContainerFragment"        android:name="ui.tabs.profile.ProfileContainerFragment">        <action            android:id="@+id/action__ProfileContainerFragment__to__AuthFlow"            app:destination="@id/auth__nav_graph" />    </fragment>    <include app:graph="@navigation/auth__nav_graph" /></navigation>

В этом случае первый экран auth-флоу появится в контейнере с нижней навигацией, а мы этого не хотели:



При этом не хочется переносить логику работы с нижней навигацией в Activity, писать какие-то методы по скрытию / демонстрации этого BottomNavigationView. Не зря же я адаптировал фикс из NavigationAdvancedSample для фрагмента, в конце концов.


В каком случае мы получим желаемый результат? Если каким-то образом осуществим навигацию из контейнера с BottomNavigationView (не из самой вкладки, а из контейнера, который является Host-ом для всех этих вкладок), то Auth-граф откроется без нижней навигации.


А на картинке можно?

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



Давайте введём action для навигации между MainFragment-ом и флоу авторизации:


Описание навигации
<! app_nav_graph.xml ><fragment  android:id="@+id/SplashFragment"  android:name="com.aaglobal.jnc_playground.ui.splash.SplashFragment"/><fragment  android:id="@+id/MainFragment"  android:name="com.aaglobal.jnc_playground.ui.main.MainFragment">  <action      android:id="@+id/action__MainFragment__to__AuthFlow"      app:destination="@id/auth__nav_graph" /></fragment><include app:graph="@navigation/auth__nav_graph" />

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


fragment_profile_container__button__open_auth_flow.setOnClickListener {    findNavController().navigate(R.id.action__MainFragment__to__AuthFlow)}

то приложение упадёт с IllegalArgumentException, потому что NavController текущей вкладки ничего не знает о навигации вне своего Host-а навигации.


Ищем правильный NavController


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


В Navigation Component есть специальная утилитная функция для поиска NavController-а, который привязан к нужному вам контейнеру, Navigation.findNavController:


Открываем флоу авторизации правильно
fragment_profile_container__button__open_auth_flow.setOnClickListener {  Navigation.findNavController(    requireActivity(),    R.id.activity_root__fragment__nav_host  ).navigate(R.id.action__MainFragment__to__AuthFlow)}


Проблемы с навигацией по кнопке Back


Итак, мы смогли открыть флоу авторизации поверх открытого фрагмента с нижней навигацией. Но появилась новая проблема: если пользователь нажмёт кнопку Back, находясь на первом экране графа авторизации, приложение упадёт. Снова с IllegalArgumentException на этот раз NavController не может найти контейнер, с которого мы только что пришли, как будто мы используем неправильный NavController для обратной навигации.


Покажи гифку


Исключение, которое мы получаем:


java.lang.IllegalArgumentException: No view found for id 0x7f08009a (com.aaglobal.jnc_playground:id/fragment_main__nav_host_container) for fragment NavHostFragment{5150965} (e58fc3a2-b046-4c80-9def-9ca40957502d) id=0x7f08009a bottomNavigation#0}

Эту проблему можно решить, переопределив поведение кнопки Back. В одной из новых версий AndroidX появился удобный OnBackPressedCallback. Раз мы используем неправильный NavController по умолчанию, значит, мы можем подменить его на правильный:


Переопределяем back-навигацию для первого экрана auth-графа
class StartAuthFragment : Fragment(R.layout.fragment_start_auth) {    private var callback: OnBackPressedCallback? = null    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {        super.onViewCreated(view, savedInstanceState)        callback = object : OnBackPressedCallback(true) {            override fun handleOnBackPressed() {                Navigation.findNavController(                    requireActivity(),                    R.id.activity_root__fragment__nav_host                ).popBackStack()            }        }.also {            requireActivity().onBackPressedDispatcher.addCallback(viewLifecycleOwner, it)        }    }}

В нашем callback-е мы делаем ровно то же, что и в прошлый раз, когда пытались построить навигацию от вкладки нижней навигации в auth-граф: находим правильный NavController, который привёл нас на первый экран внешнего графа навигации, чтобы именно с его помощью вернуться назад.


И это работает! Но есть одно но: чтобы это продолжало работать на протяжении всего auth-флоу, нам надо добавить точно такой же OnBackPressedCallback в каждый экран этого флоу =(


И, конечно же, придётся поправить закрытие всего auth-флоу там мы тоже должны добавить получение правильного NavController-а:


Как это выглядит?
class FinishAuthFragment : Fragment(R.layout.fragment_finish_auth) {  override fun onViewCreated(view: View, savedInstanceState: Bundle?) {      super.onViewCreated(view, savedInstanceState)      fragment_finish_auth__button.setOnClickListener {          Navigation.findNavController(              requireActivity(),              R.id.activity_root__fragment__nav_host          ).popBackStack(R.id.auth__nav_graph, true)          findNavController().currentBackStackEntry            ?.savedStateHandle            ?.set(AUTH_FLOW_RESULT_KEY, true)      }  }}

Подведём итоги


  • Если хотите осуществить навигацию вне текущего контейнера навигации, вы можете это сделать, получив правильный NavController.
  • Помните, что это вызовет проблемы с обратной навигацией.


Допустим, на старте приложения мы показываем пользователю экран Splash-а. На нём мы выполняем действия, связанные с инициализацией приложения. Потом, если пользователь не авторизован, мы хотим перевести его во флоу авторизации, в противном случае сразу покажем экран с нижней навигацией. При этом, когда пользователь завершит флоу авторизации (неважно, как именно), мы должны показать ему главный экран с нижней навигацией.


Покажи на картинке


У этого кейса есть пара особенностей, которые отличают его от первого рассмотренного случая.


  • Когда пользователь нажмёт на кнопку Back на первом экране флоу авторизации, мы хотим не вернуться назад (потому что зачем нам второй раз показывать Splash), а закрыть приложение.
  • После завершения флоу авторизации мы не просто закрываем открытый нами граф, но и двигаемся вперёд.

С первым пунктом проблем быть не должно, мы можем просто пробросить флажок в флоу авторизации, который будет говорить в OnBackPressedCallback, когда надо закрывать приложение, а когда просто двигаться назад:


Покажи код

Определяем флажок для StartAuthFragment:


<fragment  android:id="@+id/StartAuthFragment"  android:name="com.aaglobal.jnc_playground.ui.auth.StartAuthFragment"  android:label="Start auth"  tools:layout="@layout/fragment_start_auth">  <argument      android:name="isFromSplashScreen"      android:defaultValue="false"      app:argType="boolean"      app:nullable="false" />  <action      android:id="@+id/action__StartAuthFragment__to__FinishAuthFragment"      app:destination="@id/FinishAuthFragment" /></fragment>

А теперь используем этот флажок в OnBackPressedCallback:


class StartAuthFragment : Fragment(R.layout.fragment_start_auth) {    private val args: StartAuthFragmentArgs by navArgs()    private var callback: OnBackPressedCallback? = null    private fun getOnBackPressedCallback(): OnBackPressedCallback {      return object : OnBackPressedCallback(true) {          override fun handleOnBackPressed() {              if (args.isFromSplashScreen) {                  requireActivity().finish()              } else {                  Navigation.findNavController(                    requireActivity(),                    R.id.activity_root__fragment__nav_host                  ).popBackStack()              }          }      }    }}

Поскольку у нас Single Activity, requireActivity().finish() будет достаточно, чтобы закрыть наше приложение.


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


  • Первый способ: Navigation Component позволяет в runtime-е менять граф навигации, мы могли бы где-нибудь сохранить @id будущего destination-а и добавить немного логики при завершении авторизации.
  • Второй способ закрывать флоу авторизации как и раньше, а логику движения вперёд дописать в экран, который стартовал экраны авторизации, то есть в Splash.

Первый способ мне не нравится тем, что если появятся дополнительные destination-ы, которые надо открывать после экранов авторизации, появится и много лишней логики внутри флоу авторизации. Да и модифицировать граф навигации в runtime-е то ещё удовольствие.


Второй способ тоже не ахти потребуется сохранить предыдущий экран в back stack-е, чтобы, вернувшись на него и прочитав результат после авторизации, мы могли двигаться дальше. Но это всё равно приемлемый вариант: вложенный флоу будет отвечать только за свою собственную логику, а экран, который начинает подобную условную навигацию (выбор между main и auth на Splash-е, например), и так знает, как двигаться вперёд.


И реализовать это просто мы знаем, как закрыть auth-флоу, знаем, как прокинуть из него результат на экран, который стартовал экраны авторизации. Останется только поймать результат на SplashFragment-е.


Покажи код

Пробрасываем результат из auth-флоу:


// FinishAuthFragment.ktfragment_finish_auth__button.setOnClickListener {    // Save hasAuthData flag in prefs    GlobalDI.getAuthRepository().putHasAuthDataFlag(true)    // Navigate back from auth flow    Navigation.findNavController(        requireActivity(),        R.id.activity_root__fragment__nav_host    ).popBackStack(R.id.auth__nav_graph, true)    // Send signal about finishing flow    findNavController().currentBackStackEntry      ?.savedStateHandle      ?.set(AUTH_FLOW_RESULT_KEY, true)}

И ловим его на стороне SplashFragment-а:


// SplashFragment.ktoverride fun onViewCreated(view: View, savedInstanceState: Bundle?) {    super.onViewCreated(view, savedInstanceState)    val authResult = findNavController().currentBackStackEntry        ?.savedStateHandle        ?.remove<Boolean>(FinishAuthFragment.AUTH_FLOW_RESULT_KEY) == true    if (authResult) {        navigateToMainScreen()        return    }}

Выводы по кейсам вложенной навигации


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

Через неделю, в заключительной статье о Navigation Component, я расскажу, как можно организовать навигацию в многомодульных приложениях и как работается с дип линками, а также покажу кейсы со встраиваемыми фрагментами и диалогами.

Подробнее..

Категории

Последние комментарии

  • Имя: Макс
    24.08.2022 | 11:28
    Я разраб в IT компании, работаю на арбитражную команду. Мы работаем с приламы и сайтами, при работе замечаются постоянные баны и лаги. Пацаны посоветовали сервис по анализу исходного кода,https://app Подробнее..
  • Имя: 9055410337
    20.08.2022 | 17:41
    поможем пишите в телеграм Подробнее..
  • Имя: sabbat
    17.08.2022 | 20:42
    Охренеть.. это просто шикарная статья, феноменально круто. Большое спасибо за разбор! Надеюсь как-нибудь с тобой связаться для обсуждений чего-либо) Подробнее..
  • Имя: Мария
    09.08.2022 | 14:44
    Добрый день. Если обладаете такой информацией, то подскажите, пожалуйста, где можно найти много-много материала по Yggdrasil и его уязвимостях для написания диплома? Благодарю. Подробнее..
© 2006-2024, personeltest.ru