Ищем и анализируем ошибки в коде GitExtensions


    Как изестно Ядро Git представляет собой набор утилит командной строки с параметрами. Для комфортной работы как правило используются утилиты, дающие нам привычный графический интерфейс. Вот и мне довелось в свое время поработать с такой утилитой для Git, как GitExtensions. Не скажу, что это самый удобный инструмент, которым мне доводилось пользоваться (к примеру, тот же TortoiseGit мне нравится больше), но он явно и не безосновательно занимает свою нишу в списке любимых и проверенных утилит для работы с Git.

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


    GitExtensions


    GitExtensions — кроссплатформенный визуальный клиент для работы с системой управления версиями Git с открытым исходным кодом.

    Проект GitExtensions небольшой. В сумме это 10 основных проектов, 20 плагинов и 2 дополнительных проектов, компилируемых в вспомогательные библиотеки. В общей сложности 56 091 строк кода в 441 файле.

    Давайте посмотрим, сможет ли что-то интересное найти в этом проекте статический анализатор PVS-Studio.

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


    По итогам проверки было получено 121 предупреждение. Если рассматривать более подробно, то 16 предупреждений было получено на первом (высоком) уровне. 11 из них явно указывали на проблемные места или ошибки. На втором уровне (среднем) было получено 80 предупреждений. По моему субъективному мнению, 69 предупреждений были верными и указывали на места с ошибками или опечатками. Третий (низкий) уровень предупреждений мы рассматривать не будем, так как он в основном указывает на места, где возникновение ошибок маловероятно. И так, приступим к рассмотрению найденных ошибок.



    Выражение всегда истинно


    Начинает наш хит-парад довольно странный участок кода.

    string rev1 = "";
    string rev2 = "";
    
    var revisions = RevisionGrid.GetSelectedRevisions();
    if (revisions.Count > 0)
    {
      rev1 = ....;
      rev2 = ....;
      ....
    }
    else
    
    if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <=
    {
        MessageBox.Show(....);
        return;
    }

    V3022 Expression 'string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)' is always true. GitUI FormFormatPatch.cs 155

    Анализатор выдал предупреждение, что выражение с проверкой переменных rev1 и rev2 всегда будет истинным. Сначала я подумал, что это обычная опечатка, оплошность в логике алгоритма, которая никак не повлияет на корректность работы программы. Но рассмотрев код более детально заметил, по всей видимости лишний оператор else. Он находится перед второй проверкой, которая будет выполнена только в случае неравенства первой.

    Одно из сравнений избыточно


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

    public void EditNotes(string revision)
    {
      string editor = GetEffectivePathSetting("core.editor").ToLower();
      if (editor.Contains("gitextensions") || 
          editor.Contains("notepad") || // <=
          editor.Contains("notepad++")) // <=
      {
        RunGitCmd("notes edit " + revision);
      }
      ....
    }

    V3053 An excessive expression. Examine the substrings 'notepad' and 'notepad++'. GitCommands GitModule.cs 691

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

    Переменная используется перед проверкой на null


    Третье место занимает довольно распространённая ошибка, но сказать о том, что она в данном случае 100% вызовет некорректную работу программы я сказать не могу, так как не вникал слишком глубоко в логику работы данного кода. Лишь тот факт, что переменная проверяется на null может говорить о предполагаемом нулевом значении.

    void DataReceived(string data)
    {
      if (data.StartsWith(CommitBegin)) // <=
      {
          ....
      }
      
      if (!string.IsNullOrEmpty(data))
      {
          ....
      }
    }

    V3095 The 'data' object was used before it was verified against null. Check lines: 319, 376. GitCommands RevisionGraph.cs 319

    Переменная data используется перед проверкой на null, что потенциально может привести к возникновению исключения NullReferenceException. Если же переменная data никогда не является нулевой (null), то расположенная ниже проверка бесполезна и её следует удалить, чтобы она не вводила в заблуждение.

    Возможно возникновение исключения NullReferenceException в переопределенном методе Equals.


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

    public override bool Equals(object obj)
    {
      return GetHashCode() == obj.GetHashCode(); // <=
    }

    V3115 Passing 'null' to 'Equals(object obj)' method should not result in 'NullReferenceException'. Git.hub User.cs 16

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

    public override bool Equals(object obj)
    {
      return GetHashCode() == obj?.GetHashCode(); // <=
    }

    Идентичные выражения в условии if


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

    private void ConfigureRemotes()
    {
      ....
      if (!remoteHead.IsRemote ||
        localHead.IsRemote ||
        !string.IsNullOrEmpty(localHead.GetTrackingRemote(localConfig)) ||
        !string.IsNullOrEmpty(localHead.GetTrackingRemote(localConfig)) ||
        remoteHead.IsTag ||
        localHead.IsTag ||
        !remoteHead.Name.ToLower().Contains(localHead.Name.ToLower()) ||
        !remoteHead.Name.ToLower().Contains(_remote.ToLower()))
        continue;
      ....
    }

    V3001 There are identical sub-expressions to the left and to the right of the '||' operator. GitUI FormRemotes.cs 192

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

    Так же была найдена еще одна подобная ошибка.

    if (!curItem.IsSourceEqual(item.NeutralValue) && // <=
      (!String.IsNullOrEmpty(curItem.TranslatedValue) && 
      !curItem.IsSourceEqual(item.NeutralValue))) // <=
    {
      curItem.TranslatedValue = "";
    }

    V3001 There are identical sub-expressions to the left and to the right of the '&&' operator. TranslationApp TranslationHelpers.cs 112

    Несоответствие количества параметров при форматировании строки


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

    Debug.WriteLine("Loading plugin...", pluginFile.Name); // <=

    V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: pluginFile.Name. GitUI LoadPlugins.cs 35

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

    Debug.WriteLine("Loading '{0}' plugin...", pluginFile.Name); // <=

    Часть выражения всегда истинна


    И в завершение еще одна опечатка, которой можно было бы избежать.

    private void toolStripButton(...)
    {
      var button = (ToolStripButton)sender;
      if (!button.Enabled)
      {
        StashMessage.ReadOnly = true;
      }
      else if (button.Enabled && button.Checked) // <=
      {
        StashMessage.ReadOnly = false;
      }
    }

    V3063 A part of conditional expression is always true if it is evaluated: button.Enabled. GitUI FormStash.cs 301

    Поскольку мы проверяем, что button.Enabled равен false, то в else данной проверки мы можем смело утверждать, что button.Enabled будет равен true и таким образом исключить данную проверку повторно.

    Заключение


    В данном проекте были найдены и другие ошибки, опечатки, недочёты. Но мне они не показались интересными, чтобы описывать их в статье. Разработчики GitExtensions легко смогут найти все недочёты, воспользовавшись инструментом PVS-Studio. Вы так же можете поискать ошибки в своих проектах воспользовавшись предложенным выше статическим анализатором.

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


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ivan Kishchenko. GitExtensions bugs found and analyzed.

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

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

      +2

      Кстати, а сам Git вы проверяли?

      +2
      Переменная data используется перед проверкой на null, что потенциально может привести к возникновению исключения NullReferenceException. Если же переменная data никогда не является нулевой (null), то расположенная ниже проверка бесполезна и её следует удалить, чтобы она не вводила в заблуждение.

      Там идет не проверка на null, а на IsNullOrEmpty. Удалять ее нельзя, но можно заменить на != ""

      • НЛО прилетело и опубликовало эту надпись здесь
          0

          Самый первый баг точно приводил к падениям при просмотре истории пустого репозитория в какой-то версии. Потом, вроде бы, перестал — но, по всей видимости, вместо исправления заткнули костылем.

            +1
            ну, вот именно «как-то» )))
            либо пользователи просто с этим мирятся, либо этот код мало когда выполняется, т.к. никто не использует продукт в таких условиях

            например, в самом Git'е есть «кросс-конвертирование» кодировки сообщения коммита: когда написанное в UTF-8 сообщение нормально отображается на не-UTF-8 терминалах, и наоборот; настройки i18n.commitencoding и i18n.logoutputencoding)
            но в какой-то момент у меня это не сработало, а всё потому, что я использовал не просто git log ..., а git log --format в таких репозиториях…

            и таких примеров — масса

            З.Ы. пришлось самому исправлять и отправлять патчи
              0
              Для статического анализа свойственно обнаруживать ошибки в редко- или даже никогда не выполняющемся коде. Именно поэтому такой код «как-то работает». Однако, в любой момент в исходник могут быть внесены изменения, после которых такой код станет часто используемым. При этом выяснение причин возможных неполадок будет затруднено. Именно поэтому полезно использовать именно регулярный статический анализ. Чтобы потом не искать ошибки в коде, написанном 3 года назад, да еще почти не использовавшемся.
                +1
                Статический анализ следует использовать не только из-за этого. Да, многие ошибки в новом коде быстро исправляются при первичном тестировании. Но можно найти часть багов не в отладчике, зря тратя время, а вообще сразу ещё на этапе написания кода с помощью SCA.

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

                Помоему это признание когда о твоем продукте пишут авторы PVS-Studio :-)
                Спасибо за проверку, будем исправлять :-)

                  0
                  Так и есть!
                    +6

                    Кстати с Debug.WriteLine("Loading plugin...", pluginFile.Name); это к сожалению не баг, а дурной дизайн API Debug.WriteLine.
                    Есть перегрузка метода принимающая на вход две строки https://msdn.microsoft.com/ru-ru/library/1w33ay0x(v=vs.110).aspx

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


                  Не согласен с тем что это ошибка или опечатка. Если исправить как предлагается и вдруг захотят убрать проверку на «notepad» то с большой вероятностью перестанут работать оба варианта. Всякое бывает, и не всё задокументировано.
                  Небольшая избыточность кода которая лучше описывают логику работы и помогает поддерживать код — не баг и не опечатка. К тому же в данном случае проверки записаны в правильном порядке и не влияют на производительность. Какие-нибудь умные компиляторы/анализаторы, в теории, даже выкинуть смогут этот лишний код при компиляции.
                  • НЛО прилетело и опубликовало эту надпись здесь
                      0
                      Автор предлагает убрать проверку на notepad++ как не имеющую смысла с точки зрения выполнения программы. Но она, скорее всего, имеет смысл для разработчиков.

                      Если поменять местами то как раз таки появится лишний вызов поиска подстроки и тогда можно было бы писать про ошибку. А так всё хорошо в этом фрагменте кода.
                        –1
                        Просто представьте, что для «notepad» и «notepad++» потребуются разные обработчики. И как только вы их туда добавите, так сразу и обнаружится, что почему-то обработчик для «notepad++» почему-то вообще не вызывается, а вместо него вызывается обработчик для «notepad»… Так что правильный порядок — от длинной строки к короткой…
                    +2
                    Кстати, вот этот pull-request целиком состоит из исправлений, найденных триальной версией PVS-Studio. Тогда ещё было найдено несколько багов в PVS-Studio, а также было выяснено на своём опыте, что человек (особенно в пятницу вечером) может не видеть ошибки там, где видит и указывает анализатор, которому не сложно работать не уставая!

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

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