Топ 10 ошибок в открытых проектах С++ за 2016 год


    Пока во всём мире обсуждают 89-ю церемонию вручения наград премии «Оскар» и составляют различные рейтинги актёров и их костюмов, мы решили подготовить обзорную статью в IT-сфере. Речь пойдёт о самых интересных ошибках, допущенных в проектах с открытым исходным кодом в 2016 году. Этот год был примечателен тем, что наш анализатор PVS-Studio стал доступен и в операционных системах, основанных на Linux. Представленные ошибки наверняка уже исправлены, и каждый читатель может убедиться в серьёзности ошибок, которые допускают разработчики.

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

    Разделы отсортированы в соответствии с моими представлениями о красоте багов :).

    Десятое место


    Источник: Находим ошибки в коде компилятора GCC с помощью анализатора PVS-Studio

    V519 The 'bb_copy' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1076, 1078. cfg.c 1078

    void
    free_original_copy_tables (void)
    {
      gcc_assert (original_copy_bb_pool);
      delete bb_copy;
      bb_copy = NULL;       // <=
      delete bb_original;   // <=
      bb_copy = NULL;       // <=
      delete loop_copy;
      loop_copy = NULL;
      delete original_copy_bb_pool;
      original_copy_bb_pool = NULL;
    }

    Случайно дважды обнуляется указатель bb_copy, а указатель bb_original остался необнулённым.

    Девятое место


    Источник: Долгожданная проверка CryEngine V

    V519 The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266

    void CCryDXGLDeviceContext::
    OMGetBlendState(...., FLOAT BlendFactor[4], ....)
    {
      CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);
      if ((*ppBlendState) != NULL)
        (*ppBlendState)->AddRef();
      BlendFactor[0] = m_auBlendFactor[0];
      BlendFactor[1] = m_auBlendFactor[1];
      BlendFactor[2] = m_auBlendFactor[2]; // <=
      BlendFactor[2] = m_auBlendFactor[3]; // <=
      *pSampleMask = m_uSampleMask;
    }

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

    Восьмое место


    Источник: GDB оказался крепким орешком

    V579 The read_memory function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. jv-valprint.c 111

    extern void
    read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
    
    void
    java_value_print (....)
    {
      ....
      gdb_byte *buf;
      buf = ((gdb_byte *)
        alloca (gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT));
      ....
      read_memory (address, buf, sizeof (buf));
      ....
    }

    Оператор sizeof(buf) вычисляет не размер буфера, а размер указателя. Следовательно, из памяти извлекается недостаточное количество байт данных.

    Седьмое место


    Источник: Команда PVS-Studio готовит технический прорыв, ну а пока перепроверим Blender

    V522 Dereferencing of the null pointer 've' might take place. functions1d.cpp 107

    int QuantitativeInvisibilityF1D::operator()(....)
    {
      ViewEdge *ve = dynamic_cast<ViewEdge*>(&inter);
      if (ve) {
        result = ve->qi();
        return 0;
      }
      FEdge *fe = dynamic_cast<FEdge*>(&inter);
      if (fe) {
        result = ve->qi(); // <=
        return 0;
      }
      ....
    }

    Здесь опечатка в наименовании привела к более серьёзной ошибке. По всей видимости, второй фрагмент кода писался с помощью Copy-Paste. И случайно в одном месте забыли поменять имя переменной ve на fe. Как результат, возникнет неопределённое поведение программы, которое может, например, привести к аварийному завершению программы.

    Шестое место


    Источник: Плохой код пакета для создания 2D-анимаций Toonz

    V546 Member of a class is initialized by itself: 'm_subId(m_subId)'. tfarmcontroller.cpp 572

    class TaskId
    {
      int m_id;
      int m_subId;
    
    public:
      TaskId(int id, int subId = -1) : m_id(id), m_subId(m_subId){};

    Интересная ошибка в списке инициализации класса. Поле m_subld инициализируется само собой, хотя, скорее всего, хотели написать m_subId(subId).

    Пятое место


    Источник: PVS-Studio спешит на помощь CERN: проверка проекта Geant4

    V603 The object was created but it is not being used. If you wish to call constructor, 'this->G4PhysicsModelCatalog::G4PhysicsModelCatalog(....)' should be used. g4physicsmodelcatalog.cc 51

    class G4PhysicsModelCatalog
    {
      private:  
      ....
        G4PhysicsModelCatalog();
      ....
      static modelCatalog* catalog;
      ....
    };
    
    G4PhysicsModelCatalog::G4PhysicsModelCatalog()
    { if(!catalog) { 
        static modelCatalog catal;
        catalog = &catal; 
      } 
    }
    
    G4int G4PhysicsModelCatalog::Register(const G4String& name)
    {
      G4PhysicsModelCatalog();
      .... 
    }

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

    Четвертое место


    Источник: Casablanca: Единорог, который смог

    V554 Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. BlackJack_Server140 table.cpp 471

    void DealerTable::FillShoe(size_t decks)
    {
      std::shared_ptr<int> ss(new int[decks * 52]);
      ....
    }

    По умолчанию умный указатель типа shared_ptr для уничтожения объекта вызовет оператор delete без квадратных скобок []. В данном случае, это неправильно.

    Корректный вариант кода должен быть таким:

    std::shared_ptr<int> ss(new int[decks * 52],
                            std::default_delete<int[]>());

    Третье место


    Источник: Проверка исходного кода игрового движка Serious Engine v.1.10 к юбилею шутера Serious Sam

    V541 It is dangerous to print the string 'achrDefaultScript' into itself. dlgcreateanimatedtexture.cpp 359

    BOOL CDlgCreateAnimatedTexture::OnInitDialog() 
    {
      ....
      // allocate 16k for script
      char achrDefaultScript[ 16384];
      // default script into edit control
      sprintf( achrDefaultScript, ....); // <=
      ....
      // add finishing part of script
      sprintf( achrDefaultScript,        // <=
               "%sANIM_END\r\nEND\r\n",  // <=
               achrDefaultScript);       // <=
      ....
    }

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

    Для объяснения, почему здесь может получиться неожиданный результат, я процитирую простой пример из документации к диагностике V541:

    char s[100] = "test";
    sprintf(s, "N = %d, S = %s", 123, s);

    В результате работы этого кода хочется получить строку:

    N = 123, S = test

    Но на практике в буфере будет сформирована строка:

    N = 123, S = N = 123, S =

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

    Второе место


    Источник: PVS-Studio покопался в ядре FreeBSD

    V733 It is possible that macro expansion resulted in incorrect evaluation order. Check expression: chan — 1 * 20. isp.c 2301

    static void
    isp_fibre_init_2400(ispsoftc_t *isp)
    ....
      if (ISP_CAP_VP0(isp))
        off += ICB2400_VPINFO_PORT_OFF(chan);
      else
        off += ICB2400_VPINFO_PORT_OFF(chan - 1); // <=
      ....
    }

    На первый взгляд, в этом фрагменте кода нет ничего подозрительного. Иногда используется значение 'chan', иногда на единицу меньше: 'chan — 1', но посмотрим на определение макроса:

    #define ICB2400_VPOPT_WRITE_SIZE 20
    
    #define  ICB2400_VPINFO_PORT_OFF(chan) \
      (ICB2400_VPINFO_OFF +                \
       sizeof (isp_icb_2400_vpinfo_t) +    \
      (chan * ICB2400_VPOPT_WRITE_SIZE))          // <=

    При передаче в макрос бинарного выражения, там кардинально меняется логика вычислений. Предполагаемое выражение "(chan — 1) * 20" превращается в «chan — 1 *20», т.е. в «chan — 20», и далее в программе используется неверно вычисленный размер.

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

    Первое место


    Источник: Свежий взгляд на код Oracle VM VirtualBox

    V547 Expression is always false. Unsigned type value is never < 0. dt_subr.c 715

    #define vsnprintf RTStrPrintfV
    
    int
    dt_printf(dtrace_hdl_t *dtp, FILE *fp, const char *format, ...)
    {
      ....
      if (vsnprintf(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], // <=
            avail, format, ap) < 0) {
          rval = dt_set_errno(dtp, errno);
          va_end(ap);
          return (rval);
        }
      ....
    }

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

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

    size_t  RTStrPrintfV(char *, size_t, const char *, va_list args);
    int     vsnprintf   (char *, size_t, const char *, va_list arg );

    Заключение


    В заключение хочу привести самую популярную картинку к статье, которая получила множество восторженных комментариев. Картинка из статьи "Проверка проекта OpenJDK с помощью PVS-Studio ":



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

    Скачать и попробовать PVS-Studio можно по этой ссылке.

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

    Желаю всем безбажного кода!



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Top 10 bugs in C++ open source projects, checked in 2016

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    779.72
    Static Code Analysis for C, C++, C# and Java
    Share post

    Similar posts

    Comments 28

    • UFO just landed and posted this here
        –2
        Жаль, что не попало ничего из серии if (a != 1 && a != 2), помню, когда нашел такое анализатором, то было очень смешно :)
          0
          А что здесь не так?
            +5
            Скорее всего, имелось ввиду (a != 1 || a != 2) либо его производные выражения через отрицание.
            always true/always false, зачастую встречается даже в серьезном коде, те же resharper/студия( в c#) сильно не всегда отлавливают, особенно если предикат длинный.
              –1
              Кстати хороший вопрос. Если 1, или 2 — false, если что-то другое — true.
                +3
                Да, конечно же имелось в виду if (a != 1 || a != 2).
                  0
                  Ясно же, «А» заглавная!
                  +2
                  Была у меня статеечка на эту тему: Логические выражения в C/C++. Как ошибаются профессионалы. В один год я увидел слишком много таких ошибок и решил собрать все типы в один документ.
                  +2
                  В пятом случае ошибки нет, статический член catalog инициализируется корректно — проверка

                  В четвертом случае, при передаче POD типов, разницы между delete и delete[] тоже нет.

                  Но с копи-пастой борьба успешная =)
                    +6
                    В любом случае

                      0
                      А можно пояснить, как в случае №5 код соотносится с описанием «проблемы»? В конструкторе G4PhysicsModelCatalog создается статический объект типа modelCatalog, указатель на который сохраняется в статическом же члене класса. Такое впечатление, что разработчики хотели достичь того, чтобы экземпляр modelCatalog создавался бы только при создании первого экземпляра G4PhysicsModelCatalog. Если же G4PhysicsModelCatalog нет, то нет и экземпляра modelCatalog.
                        0
                        Ды фигня тут получилась, бывает. Перефразируя поговорку, не ошибается в статьях только то, кто их не пишет. :)
                        Мы столько кода и багов перелопачиваем, что сложно сосредоточиться.
                      +2

                      Про delete и delete[] в 4 примере поясните, пожалуйста, с какой стати нет разницы для POD-типов.
                      Товарищи вот тут http://stackoverflow.com/questions/4255598/delete-vs-delete цитируют стандарт, где ясно сказано про undefined behaviour.


                      P.s. в 3 примере вообще, скорее всего, было бы уместно
                      std::unique_ptr<int[]> ss(new int[decks * 52]);
                      (хотя без полного кода не скажешь, конечно).

                        0
                        Потому что delete[] дополнительно вызывает деструкторы каждого объекта массива. Других отличий от delete нет.

                        У POD типов нет деструкторов, так что вызывать нечего.

                        Но для классов я бы не пренебрегал [], т.к. классы могут и переделать когда нибудь потом другие люди. Но в примере простой int
                          +4
                          Всё равно так делать НЕЛЬЗЯ. Например, перед массивом может храниться его размер. И не важно, что для int[] это не обязательно. Да и вообще нет смысла рассуждать. Это UB. Точка.
                      +2

                      Давайте анализаторы для Go & TypeScript!

                        +1
                        Вот Cobol — это тема… На нем анализаторы нужны. А всякие Go — на них нет проектов.
                          +2
                          На Коболе все баги уже давно стали фичами.
                            0

                            PHP… Далеко не ушел, да и проектов не мало...

                          +2

                          А как вы вообще находите примеры ошибок, которые можно искать. Вам их присылают?

                            +1
                            Еще раз убедился в том, что ошибка при копипасте — это популярная ошибка.
                              0
                              которое может, например, привести к аварийному завершению программы.

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

                                +1
                                Такой программы давно нахватало, ведь программировать что-что сложно, а находить ошибки в коде ещё сложней (особенно если исправляешь программу человека который не хочет разбираться, а старается «пробиться» и самому не делать, так и хочется сказать потом «на, подавись!», но воспитанность не позволяет, а тут на бери не хочу и пользуйся в своё удовольствие).
                                  +4
                                  С макросами и FreeBSD порадовало. Помнится сам измучился с макросами когда студентом был, позвал начальника, и он сказал очень простое правило — каждый параметр макроса внутри макроса обязательно брать в круглые скобки, а так же использовать не более одного раза. Такая минимальная осторожность позволяет никогда не факапиться!
                                    +1
                                    Интересно, я давно работаю с C++, а об этом не знал и макросы из-за этого помешал в раздел «не работает, значит не пользуюсь». Надо будет пересмотреть некоторые коды.
                                    0
                                    Недавно проверил код эмулятора Nestopia. Нашлись несколько ошибок и несколько подозрений на ошибки, но нужно было сильно разбираться в коде, чтобы понять, действительно ли ошибки или ложные подозрения. Например, функция ширины спрайта возвращает координату левого края. Анализатор предупреждает, что тело функции соответствует функции получения левого края. Приятно, что о таком предупреждает.
                                    А вот когда предупреждает про одинаковое тело просто метода и const метода — огорчает. Может стоит дополнить правило, что если у методов одинаковые имена и списки аргументов, но различный const у возвращаемого значения и самого метода, то для них не предупреждать об одинаковом теле?
                                      0
                                      вот когда предупреждает про одинаковое тело просто метода и const метода — огорчает. Может стоит дополнить правило, что если у методов одинаковые имена и списки аргументов, но различный const у возвращаемого значения и самого метода, то для них не предупреждать об одинаковом теле?
                                      Если я всё правильно понял, то такого срабатывания быть не должно. Прошу привести синтетический пример, на котором воспроизводится ложное срабатывание. Можно в почту.
                                        0
                                        Пардон, не получается воспроизвести.

                                    Only users with full accounts can post comments. Log in, please.