Предновогоднее шоу: Топ 10 ошибок в C и С++ проектах в 2023 году

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

Вступление

Вечер, снег блестит под светом уличных фонарей, приятно щиплет лицо от мороза, вокруг тишина, а с неба, кружась, падают снежинки. Скоро новый год и всё замерло в предвкушении чуда. Думаю, каждому знакомо это ощущение надвигающегося праздника. Я с теплотой вспоминаю, как меня в детстве зимой перед праздниками водили в цирк. Это было великолепно! Везде горят разноцветные огни, и тебя наполняет праздничное настроение! Весёлая музыка, смешные клоуны, запах попкорна. Загадочный усатый ведущий в чёрном цилиндре, с тростью старинной, задающий атмосферу таинственности. И вот он объявляет номер, и на арене происходит маленькое приключение, или случаются чудеса, и весь зал замирает на время выступления. Затем аплодисменты, овации – всё смешивается в едином порыве. Сейчас мы уже выросли, но можем сами создавать свои чудеса для наших близких, друзей и знакомых, которых хотим порадовать.

А посему сегодня вас ждёт не просто статья о том, как где-то в коде допустили ошибку, а целое волшебство кодинга. 10 масштабных и зрелищных разборов ошибок – всё только для вас! Итак, *надевает маску Шпрехшталмейстера (Ш:)*, посмотрим, что у нас сегодня в программе:

Ш: "Наше предновогоднее шоу готово искренне удивить и восхитить вас своей новой программой! Незабываемое зрелище, множество жанров: от утечек памяти до разыменовывания указателей без страховки! Смешные опечатки!

Потрясающие выступления профессиональных программистов на открытых проектах – это шоу мирового уровня!"

Начинаем Шоу!

Ш: "Шоу для программистов! Так, так, так... Раз уж на то пошло, наше волшебство начнём... пожалуй, с цифры 9, а закончим 0! Воистину магические цифры, и зачем нам больше?"

Девятый "номер". Сомнительный цикл

Ш: "Дамы и Господа! Леди и Джентльмены! Программисты всех мастей! Время для магии и волшебства! Этот код заставит вас усомниться в самой концепции существования эффективного код-ревью."

Статья: "Проверка компилятора GCC 13 с помощью PVS-Studio".

static bool
combine_reaching_defs (ext_cand *cand,
                       const_rtx set_pat,
                       ext_state *state)
{
  ....
  while (   REG_P (SET_SRC (*dest_sub_rtx))
         && (REGNO (SET_SRC (*dest_sub_rtx)) == REGNO (SET_DEST (set))))
  {
    ....
    if (....)
      break;

    if (....)
      break;
    
    ....
    break;
  }
}

Предупреждение анализатора PVS-Studio: V612 An unconditional 'break' within a loop. ree.cc 985

Анализатор PVS-Studio обнаружил, что этот цикл безусловно прерывается на первой итерации. При дальнейшем рассмотрении мы также обнаруживаем множество других break под условиями. Возможно, это способ избежать написания оператора goto. Однако выглядит он странно, учитывая его постоянное открытое использование в целом по проекту.

Восьмой "номер". Самая опасная функция в мире C и C++

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

Статья: "Microsoft PowerToys: Король GitHub среди C# проектов с C++ ошибками".

void SetNumLockToPreviousState(....)
{
  int key_count = 2;
  LPINPUT keyEventList = new INPUT[size_t(key_count)]();
  memset(keyEventList, 0, sizeof(keyEventList));
  ....
}

На этот код PVS-Studio выдаёт сразу три предупреждения:

• V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. KeyboardEventHandlers.cpp 16
• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'keyEventList' class object. KeyboardEventHandlers.cpp 16
• V1086 A call of the 'memset' function will lead to underflow of the buffer 'keyEventList'. KeyboardEventHandlers.cpp 16

На тему подобных ошибок у нас даже есть отдельная статья: "Самая опасная функция в мире С/С++".

В данном случае разработчики хотели занулить массив keyEventList. Обратим внимание на 3 параметр – количество байт, которые хотели заполнить нулями. Вот только sizeof(keyEventList) вычисляет не размер массива, а размер указателя. Он зависит от целевой платформы, но чаще всего это 4 или 8 байт. При этом размер структуры явно больше 4 или 8 байт:

typedef struct tagINPUT 
{
  DWORD   type;

  union
  {
    MOUSEINPUT      mi;
    KEYBDINPUT      ki;
    HARDWAREINPUT   hi;
  } DUMMYUNIONNAME;
} INPUT, *PINPUT, FAR* LPINPUT;

Седьмой "номер". Дважды добавленный символ

Ш: "Загадки, загадки, загадки... Сейчас мы с вами поучаствуем в битве! Посмотрите на код! Сможете ли вы за короткое время найти здесь самый маленький, но очень важный символ? Часто этот символ означает конец всего! Но иногда он является началом чего-то нового и прекрасного. Ну, или обращения напрямую к полям объекта..."

Статья: "PVS-Studio vs CodeLite: битва за идеальный код".

std::unordered_set<wxChar> delimiters =
  { ':', '@', '.', '!', ' ', '\t', '.', '\\', 
    '+', '*', '-', '<', '>', '[', ']', '(', 
    ')', '{', '}',  '=', '%', '#', '^', '&', 
    '\'', '"', '/', '|',  ',', '~', ';', '`' };

Предупреждение анализатора: V766 An item with the same key ''.'' has already been added. wxCodeCompletionBoxManager.cpp:19

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

Шестой "номер". Ошибочное переопределение

Ш: "Вы хотите выиграть? Или не проиграть? Или это иллюзия выбора? Этот номер порадует вас как ответами на эти вопросы, так и иллюзией переопределения виртуальной функции!"

Статья: "PVS-Studio vs CodeLite: битва за идеальный код".

В базовом классе функция выглядит так:

class WXDLLIMPEXP_CORE wxGenericProgressDialog : public wxDialog
{
public:
  ....
  virtual bool Update(int value,
                      const wxString& newmsg = wxEmptyString,
                      bool *skip = NULL);
  ....
};

А вот как в наследнике:

class clProgressDlg : public wxProgressDialog
{
public:
  ....
  bool Update(int value, const wxString& msg);
  ....
};

Предупреждение анализатора: V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'Update' in derived class 'clProgressDlg' and base class 'wxGenericProgressDialog'. progress_dialog.h:47, progdlgg.h:44

Исходя из совпадения первых двух параметров функций, можно сделать вывод, что действительно хотели переопределить виртуальную функцию. Однако параметры по умолчанию также являются частью сигнатуры. Поэтому функция clProgressDlg::Update на самом деле не переопределяет, а скрывает виртуальную функцию wxGenericProgressDialog::Update.

Корректное объявление виртуальной функции должно быть такое:

class clProgressDlg : public wxProgressDialog
{
public:
  ....
  bool Update(int value, const wxString& msg, bool *skip);
  ....
};

Чтобы избежать таких ошибок, начиная с C++11, можно и даже нужно использовать спецификатор override:

class clProgressDlg : public wxProgressDialog
{
public:
  ....
  bool Update(int value, const wxString& msg, bool *skip) override;
  ....
};

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

Пятый "номер". Мультивселенная безумия

Ш: "Короли, иллюзии, магия и загадки. И правда — вселенная сегодня безумна, но сейчас вы увидите что-то по-настоящему такое, что повергает в шок! Слабонервным просьба отвернуться!"

Статья: "Ква! Как писали код во времена Quake".

// PVS-Studio: void Mod_LoadTexinfo (lump_t *l)
// PVS-Studio: client/model.c

mtexinfo_t *out;
// ... 
for (j=0 ; j<8 ; j++)
{
  ut->vecs[0][j] = LittleFloat (in->vecs[0][j]);
}

V557 Array overrun is possible. The value of 'j' index could reach 7.

Для понимания добавлено объявление для mtexinfo_t *out:

// PVS-Studio: client/model.c

typedef struct
{
  float       vecs[2][4]; // PVS-Studio: see what is wrong here?
  float       mipadjust;
  texture_t   *texture;
  int         flags;
} mtexinfo_t;
// PVS-Studio: client/common.h

float (*LittleFloat) (float l);

У нас есть двумерный массив, который используют как одномерный. Мало кто увидит проблему. Программист может подумать, что в памяти он лежит последовательно как минимум C99, 6.2.5 Types p20 — и будет прав.

"An array type describes a contiguously allocated nonempty set of objects with a particular member object type, called the element type. The element type shall be complete whenever the array type is specified."

Однако в языке С существует специальный тип — указатель на массив T. Во время обращения к массиву внутри цикла, j обязательно превысит значение 3 и оператор [] вернёт переменную типа float[4]. Как следствие произойдёт чтение элемента массива вне его пределов, что является классическим примером неопределённого поведения (UB).

Четвёртый "номер". Только истина

Ш: "Подходите ближе, дамы и господа! Вас ждёт удивительный номер! Представьте, что условия — это всего лишь условности, а истина может обмануть ваши ожидания..."

Статья: "Герои Кода и Магии: анализ игрового движка VCMI".

void deallocate_segment(....) 
{
  ....
  if (seg_index >= first_block) 
  {
    segment_element_allocator_traits::deallocate(....);
  }
  else 
  if (seg_index == 0) 
  {
    elements_to_deallocate = first_block > 0
                               ? this->segment_size(first_block) 
                               : this->segment_size(0);
    ....
  }
}

Предупреждение анализатора: V547 Expression 'first_block > 0' is always true. concurrent_vector.h 669

В данном коде, если первая проверка не выполняется, то seg_index < first_block, а если seg_index == 0, то это означает, что first_block > 0. Затем идёт ещё одна проверка first_block > 0, которую анализатор справедливо помечает как всегда истинную. Следовательно, выражение this->segment_size(0) никогда не выполнится.

Хорошо, что у нас есть технологии для статического анализа, выявляющие подобные ошибки! Всё это возможно благодаря использованию в анализаторе технологии символьного выполнения (symbolic execution). Магия, не правда ли? И наверняка так и воспринимают это обычные люди. А вот представьте:

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

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

Третий "номер". Очепятка неопределённого поведения

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

Статья: "FreeCAD и C++ код с неопределённым поведением для медитации".

QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (!vpp) {
    return vpp->getQGVPage();
  }
  return nullptr;
}

Предупреждение анализатора: V522 [CWE-476, CERT-EXP34-C] Dereferencing of the null pointer 'vpp' might take place. QGIView.cpp 592

Автор кода опечатался при написании условия:

• Если указатель ненулевой, то функция ничего не делает и возвращает nullptr.
• Если указатель нулевой, то произойдёт его разыменование. Возникнет неопределённое поведение.

Что интересно, по соседству присутствует функция-близнец, и в ней условие правильное:

QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (!vpp) {
    return vpp->getQGVPage();
  }
  return nullptr;
}

QGSPage* QGIView::getQGSPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (vpp) {
    return vpp->getQGSPage();
  }
  return nullptr;
}

Непонятно, как так получилось. С большой вероятностью вторая функция была написана с помощью copy-paste. Странно то, что во второй функции ошибку поправили, а в первой она осталась, и, возможно, ошибка присутствовала в обеих функциях. Затем кто-то заметил, что функция getQGSPage работает неправильно, и исправил её.

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

Второй "номер"! Доверяй, но проверяй

Ш: "И снова загадка! И снова код, покрытый тайнами и иллюзиями правильности своего выполнения! Неотвратимость судьбы или чья-то нехорошая шутка? Попробуйте же отгадать, что здесь не так?"

Статья: "Игоры! Как пишут код для SDL (+ интервью с создателем)".

Файл: SDL/src/stdlib/SDL_iconv.c (GitHub permalink)

char *SDL_iconv_string(const char *tocode, const char *fromcode,
                       const char *inbuf, size_t inbytesleft)
{
  SDL_iconv_t cd;
  ....

  cd = SDL_iconv_open(tocode, fromcode);
  if (cd == (SDL_iconv_t)-1) {
    /* See if we can recover here (fixes iconv on Solaris 11) */
    if (tocode == NULL || !*tocode) {
      tocode = "UTF-8";
    }
    if (fromcode == NULL || !*fromcode) {
      fromcode = "UTF-8";
    }
    cd = SDL_iconv_open(tocode, fromcode);
  }
  ....
}

V595 The 'tocode' pointer was utilized before it was verified against nullptr. Check lines: 37, 789, 792. SDL/src/stdlib/SDL_iconv.c:792:1

Для понимания проблемы надо окунуться в реализацию функции SDL_iconv_open:

SDL_iconv_t SDL_iconv_open(const char *tocode, const char *fromcode)
{
  return (SDL_iconv_t)((uintptr_t)iconv_open(tocode, fromcode));
}

На вход функции SDL_iconv_string вполне могут быть переданы нулевые указатели в аргументах tocode и fromcode. В случае этого они перед проверкой с ветерком полетят и в саму iconv_open.

Если мы посмотрим в man страницу данной функции проекта GNU, то не увидим никакого упоминания о том, что NULL толкать в неё нельзя. Но нас не проведёшь! Поэтому надо взглянуть в исходники библиотеки С:

iconv_t
iconv_open (const char *tocode, const char *fromcode)
{
  /* Normalize the name.  We remove all characters beside alpha-numeric,
     '_', '-', '/', '.', and ':'.  */
  size_t tocode_len = strlen (tocode) + 3;
  ....
}

Ну... обращения по указателю, конечно, не происходит, но вполне себе присутствует вызов strlen без проверки на NULL. Все мы знаем, что strlen делает в этом случае, и мало кому это по душе!

Однако автор не остановился на одной имплементации — он проверил также BSD и Musl. И все они ведут себя равным образом.

Первый "номер"! Много потоков, много проблем

Ш: "А теперь ловкач-процессор покажет вам, как жонглировать потоками! И в этом ему будет помогать малыш-компилятор! Посмотрите на его точные акробатические движения с потоками, сможет ли кто-то из зала так же?"

Статья: "Проверяем YTsaurus. Доступность, надёжность, open source".

TAsyncSignalsHandler *SIGNALS_HANDLER = nullptr;

void SetAsyncSignalHandler(int signum, TAutoPtr<TEventHandler> handler)
{
  static TAdaptiveLock lock;

  if (Y_UNLIKELY(SIGNALS_HANDLER == nullptr))       // N1
  {
    TGuard dnd(lock);

    if (SIGNALS_HANDLER == nullptr)                 // N2
    {
      // NEVERS GETS DESTROYED
      SIGNALS_HANDLER = new TAsyncSignalsHandler(); // N3
    }
  }

  SIGNALS_HANDLER->Install(signum, handler);        // N4
}

Предупреждение анализатора: V547 Expression 'SIGNALS_HANDLER == nullptr' is always true. async_signals_handler.cpp:200

Несмотря на то, что здесь сработало диагностическое правило V547, был найден интересный экземпляр паттерна Double-checked locking.

Что же может пойти не так? Для начала имеем небольшую вероятность того, что компилятор может закэшировать значение переменной SIGNALS_HANDLER в регистре и не будет заново читать из неё новое значение. По крайней мере, если будет использоваться нетипичный способ блокировки. Лучшим решением будет объявить переменную как volatile или использовать std::atomic.

Дальше больше. Давайте попробуем разобраться, что здесь не очень хорошо. Будем считать, что с этим кодом оперируют поток A и поток B.

Поток A первым добирается до точки N1. Указатель нулевой, так что поток управления заходит в первый if и захватывает объект блокировки.

Поток A доходит до точки N2. Указатель также нулевой, поэтому поток управления заходит во второй if.

Дальше в точке N3 может произойти небольшая магия. Инициализацию указателя можно представить примерно в следующем виде:

auto tmp = (TAsyncSignalsHandler *) malloc(sizeof(TAsyncSignalsHandler));
new (tmp) TAsyncSignalsHandler();
SIGNALS_HANDLER = tmp;

Хитрый процессор может переупорядочить инструкции, и в итоге SIGNALS_HANDLER будет проинициализирован до того, как будет вызван placement new.

И тут в бой вступает поток B — как раз в тот момент, когда поток A ещё не успел вызвать placement new. Поток управления приходит в точку N1, видит инициализированный указатель и перемещается в точку N4.

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

Как поправить этот паттерн? Надо сделать так, чтобы чтение и запись в SIGNALS_HANDLER происходили атомарно:

std::atomic<TAsyncSignalsHandler *> SIGNALS_HANDLER { nullptr };

void SetAsyncSignalHandler(int signum, TAutoPtr<TEventHandler> handler)
{
  static TAdaptiveLock lock;

  auto tmp = SIGNALS_HANDLER.load(std::memory_order_acquire); // <=
    
  if (Y_UNLIKELY(tmp == nullptr))
  {
    TGuard dnd(lock);

    tmp = SIGNALS_HANDLER.load(std::memory_order_relaxed);    // <=
    if (tmp == nullptr)
    {
      // NEVERS GETS DESTROYED
      tmp = new TAsyncSignalsHandler();
      SIGNALS_HANDLER.store(tmp, std::memory_order_release);  // <=
    }
  }

  tmp->Install(signum, handler);
}

Нулевой "номер"!!! Капитан Блад и его сокровища!

Ш: "А теперь то, чего вы все так долго ждали! Безжалостные Пираты! Сундук с сокровищами, оставленный в назидание нам, а также тем, кто придёт после... Бережно хранимый в нашем набитом другими редкими сокровищами трюме. Сегодня мы откроем его специально для вас! Йо-хо-хо и бутылка рома!"

Статья: "Приключения капитана Блада: потонет ли Арабелла?".

Приведу проблемную функцию полностью:

void appDebuger::CopyDump(const char * src, DWORD from, const char * dst)
{
  FILE * file = fopen(src, "rb");
  if (file)
  {
    fseek(file, 0, SEEK_END);
    DWORD size = ftell(file);
    fseek(file, from, SEEK_SET);
    if(size > from)
    {
      FILE * to = fopen(dst, "a+b");
      if(to)
      {
        char buf[128];
        while (from < size)
        {          
          DWORD s = size - from;
          if (s > sizeof(buf)) s = sizeof(buf);
          memset(buf, ' ', sizeof(buf));
          fread(buf, s, 1, file);
          fwrite(buf, s, 1, file);            // <=
          from += s;
        }
        fclose(to);
      }
    }
    fclose(file);
  }
}

V1075 The function expects the file to be opened for writing, but it was opened for reading. Debugger.cpp 172

Мы не думали, что диагностика когда-нибудь сработает, так как ошибка довольно редкая, однако первое трофейное срабатывание произошло сразу после выхода в релизе PVS-Studio 7.15. Данное диагностическое правило ругается на запись в файлы, открытые для чтения, и наоборот.

Итак, функция должна прочитать данные из одного файла по пути src, начиная с позиции from, и записать их в другой файл по пути dst. За исходный файл отвечает переменная file, за результирующий файл — переменная to.

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

fwrite(buf, s, 1, to);

Конец представления!

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

А самое главное, не забывайте, мы с вами все можем творить чудеса хоть каждый день. Главное упорство и вера в своё дело, чем бы мы с вами не занимались.

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

Традиционно предлагаю попробовать наш анализатор PVS-Studio. Для Open Source проектов у нас предоставляется бесплатная лицензия.

Берегите себя и всего доброго! Приятных новогодних праздников!