Проверка открытого проекта WinSCP, разрабатываемого в Embarcadero C++ Builder

    PVS-Studio and WinSP
    Мы постоянно проверяем открытые проекты на языке Си/Си++. Но почти всегда, это проекты, разрабатываемые в Visual Studio. А вот Embarcadero C++ Builder мы как-то обделили вниманием. Нужно исправляться и сегодня мы проверили проект WinSCP.

    WinSCP


    WinSCP – свободный графический клиент протоколов SFTP и SCP, предназначенный для Windows. Распространяется по лицензии GNU GPL. Обеспечивает защищённое копирование файлов между компьютером и серверами, поддерживающими эти протоколы.

    Официальный сайт: http://winscp.net

    Для сборки проекта необходим Embarcadero C++ Builder XE2.

    Проверка


    Проверка осуществлялась с помощью анализатора PVS-Studio. На данный момент PVS-Studio поддерживает:
    • Visual Studio 2013 C, C++, C++11, C++/CX (WinRT)
    • Visual Studio 2012 C, C++, C++11, C++/CX (WinRT)
    • Visual Studio 2010 C, C++, C++0x
    • Visual Studio 2008 C, C++
    • Visual Studio 2005 C, C++
    • Embarcadero RAD Studio XE5 C, C++, C++11, 64-bit compiler included
    • Embarcadero RAD Studio XE4 C, C++, C++11, 64-bit compiler included
    • Embarcadero RAD Studio XE3 Update 1 C, C++, C++11, 64-bit compiler included
    • Embarcadero RAD Studio XE2 C, C++, C++0x
    • Embarcadero RAD Studio XE C, C++
    • Embarcadero RAD Studio 2010 C, C++
    • Embarcadero RAD Studio 2009 C, C++
    • MinGW C, C++, C++11
    Плюс вы можете запустить PVS-Studio Standalone. Он позволяет проверять заранее подготовленные *.i файлы или отслеживать процесс сборки проекта и собирать всю необходимую для проверки информацию. Подробности можно узнать здесь: "PVS-Studio теперь поддерживает любую сборочную систему".

    Результаты проверки


    Ошибок нашлось не много, но достаточно, чтобы написать эту статью и привлечь внимание пользователей Embarcadero RAD Studio.

    Перепутаны аргументы функции memset()


    TForm * __fastcall TMessageForm::Create(....)
    {
      ....
      LOGFONT AFont;
      ....   
      memset(&AFont, sizeof(AFont), 0);
      ....
    }   

    Предупреждение PVS-Studio: V575 The 'memset' function processes '0' elements. Inspect the third argument. messagedlg.cpp 786

    Функция memset() принимает размер массива в третьем аргументе. Простая, но неприятная опечатка. Структура остаётся неинициализированной.

    Ниже в коде имеется ещё одна идентичная опечатка: messagedlg.cpp 796

    Использование несуществующего объекта


    void __fastcall TCustomScpExplorerForm::EditorAutoConfig()
    {
      ....
      else
      {
        ....
        TEditorList EditorList;
        EditorList = *WinConfiguration->EditorList;
        EditorList.Insert(0, new TEditorPreferences(EditorData));
        WinConfiguration->EditorList = &EditorList;
      }
      ....
    }

    Предупреждение PVS-Studio: V506 Pointer to local variable 'EditorList' is stored outside the scope of this variable. Such a pointer will become invalid. customscpexplorer.cpp 2633

    Объект 'EditorList' будет уничтожен сразу после выхода из области видимости. Однако, в программе сохраняют указатель на этот объект и затем используют. Это приводит к неопределённому поведению.

    Мусор в диалоге


    bool __fastcall RecursiveDeleteFile(....)
    {
      SHFILEOPSTRUCT Data;
      memset(&Data, 0, sizeof(Data));
      ....
      Data.pTo = L"";
      ....
    }

    Предупреждение PVS-Studio: V540 Member 'pTo' should point to string terminated by two 0 characters. common.cpp 1659

    Обратите внимание на следующую строку в описании параметра pTo в MSDN: «Note This string must be double-null terminated».

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

    Продублированная строка


    int CFileZillaApi::Init(....)
    {
      ....
      m_pMainThread->m_hOwnerWnd=m_hOwnerWnd;
      m_pMainThread->m_hOwnerWnd=m_hOwnerWnd;
      ....
    }

    Предупреждение PVS-Studio: V519 The 'm_pMainThread->m_hOwnerWnd' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 88, 89. filezillaapi.cpp 89

    Возможно, здесь нет никакой ошибки. Просто одна строка лишняя.

    Неработающая проверка


    STDMETHODIMP CShellExtClassFactory::CreateInstance(....)
    {
      ....
      CShellExt* ShellExt = new CShellExt();
      if (NULL == ShellExt)
      {
        return E_OUTOFMEMORY;
      }
      ....
    }

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

    Проверка «if (NULL == ShellExt)» не имеет смысла, так как если не удастся выделить память, то оператор 'new' сгенерирует исключение std::bad_alloc.

    Опасный способ использования функции fprintf()


    bool CAsyncSslSocketLayer::CreateSslCertificate(....)
    {
      ....
      char buffer[1001];
      int len;
      while ((len = pBIO_read(bio, buffer, 1000)) > 0)
      {
        buffer[len] = 0;
        fprintf(file, buffer);
      }
      ....
    }

    V618 It's dangerous to call the 'fprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); asyncsslsocketlayer.cpp 2247

    Если при записи в файл, в буфере будут содержаться управляющие спецификаторы, то результат окажется непредсказуемым. Безопасный способ:
    fprintf(file, "%s", buffer);

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

    Что-то не так с переменной 'err'


    static error_t
    client_send_propfind_request(....)
    {
      ....
      error_t err = 0;
      int code = 0;
    
      apr_hash_t * props = NULL;
      const char * target = path_uri_encode(remote_path, pool);
      char * url_path = apr_pstrdup(pool, target);
    
      WEBDAV_ERR(neon_get_props(&props, ras, url_path,
        NEON_DEPTH_ZERO, starting_props,
        false, pool));
    
      if (err && (err == WEBDAV_ERR_DAV_REQUEST_FAILED))
      ....
    }

    Предупреждение: V560 A part of conditional expression is always false: (err == 1003). webdavfilesystem.cpp 10990

    Заключение


    Где вы, программисты, использующие Embarcadero RAD Studio? Ау! Как показывает наша статистика, их почти нет. Приходите и попробуйте анализатор кода PVS-Studio!

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. A Check of the Open-Source Project WinSCP Developed in Embarcadero C++ Builder .

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

    Комментарии 25

    • НЛО прилетело и опубликовало эту надпись здесь
        +1
        Data.pTo = L"";
        

        Предупреждение PVS-Studio: V540 Member 'pTo' should point to string terminated by two 0 characters. common.cpp 1659
        В C++ Builder sizeof(*L"") == sizeof(wchar_t) == 2 * sizeof(char) == 2, так что ошибки тут нет. Но с точки зрения теоретического С++ верно.
          +2
          А если подумать? :)

          Даю подсказку:
          Вы хотите сказать, что (wchar *) неявно приводится к (char *)?
          А если нет, то почему и что из этого следует?
            +2
            В MSDN даже пример для «знатоков» есть:

            
            // WRONG
            LPTSTR pszSource = L"C:\\Windows\\*";
            
            // RIGHT
            LPTSTR pszSource = L"C:\\Windows\\*\0";
            
            
              0
              Лучше и здесь писать PCZZTSTR тогда.
                0
                А это по стандарту? Или фишка VisualC? MinGW генерирует 2 нуля
                  0
                  Эх…
                  Предлагаю внимательно прочитать: msdn.microsoft.com/en-us/library/windows/desktop/bb759795%28v=vs.85%29.aspx
                  И осознать.
                  Фишек тут никаких нет. Есть непонимание, что должно быть 2 нулевых байта (если char *) и 4 нулевых байта (если wchar_t *).
                    +3
                    Спасибо, почитал еще более внимательно и нашел, что double null-terminating — это просто требование для конкретной функции, где на вход может быть подано несколько файловых путей за раз, каждый из которых является простой null-teminated строкой (два 0 для wchar), а вся последовательность терминируется дополнительным нулем (еще 2 нуля).
                    Получается, что инструмент очень умный и проверил, что параметр не простой, а для особой функции. Уф, а то я уже стал сомневаться в собственном рассудке.
                +11
                О, погуглил, там PCZZTSTR раскрывающийся в wchar_t*, и в конце должно стоять два (wchar_t)0. Тогда действительно предупреждение верное. Спасибо, не знал про существование такой штуки (ох уж этот WinAPI).
              0
              А вы статический анализатор только для С будете? Или ваша платформа позволяет абстрагироваться, и подставив нужный парсер для нужной грамматики, реализовать сие чудо для других ЯВУ?
                +4
                Если абстрагироваться от существующего программного продукта, то можно подставив другой нужный программный продукт сделать анализатор для любого языка :-). Но мы про C/C++ пока.
                  0
                  Вряд ли в рамках такой задачи можно абстрагироваться, ведь, как я понимаю, анализатор ориентирован в том числе на неправильное использование API (будь то STL, POSIX или WinAPI). Так что дело одной грамматикой не обойдется.
                    0
                    Вот, кстати, АПИ-то как раз одно. Плюшки языка — специфичны.
                      0
                      Как джавист, не могу полностью согласиться ;) Не везде АПИ одно.
                        0
                        В контексте разных ОСей, да — разное. В контексте разных языков, но одной ОСи — одно. Разве нет?
                    0
                    Большинство ошибок, которые описываются в статьях Andrey2008, на Яве просто невозможны.
                    Для Явы своих инструментов хватает, едва ли уступающих в дотошности PVS-Studio. FindBugs, например.
                  +3
                  Где вы, программисты, использующие Embarcadero RAD Studio? Ау!

                  Ну я пишу на билдере и 2е знакомых специалиста тоже пишут на билдере, только мы программисты встраиваемых систем
                    +1
                    Вот такой вот подход:
                    LOGFONT AFont = LOGFONT();
                    

                    Позволяет забыть как все эти ручные memset'ы как дурной сон. Абсолютно стандартный, наглядный, невозможно перепутать типы или обнулить что-то не правильно. И даже если тип потом станет non-POD, код останется корректным.
                      0
                      или LOGFONT AFont = {0}; для структур
                        0
                        Не для всех.
                      0
                      SHFILEOPSTRUCT
                      

                      Меня всегда умиляли такие названия типов. Прям черное наречие!
                        0
                        Друзья, спасибо Вам за вашу работу! :) Но есть пожелание — протестируйте ядро Linux апстрим версии? Уверен, там можно внести огромный вклад в фикс неоднозначностей.
                          0
                          Если еще не сделали, то это классная идея :)

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

                        Самое читаемое