История о том, как PVS-Studio нашёл ошибку в библиотеке, используемой в… PVS-Studio

    Picture 1

    Это небольшая история о том, как с помощью PVS-Studio удалось найти ошибку в исходном коде библиотеки, используемой в PVS-Studio. Причём не теоретическую, а фактическую — ошибка проявлялась на практике при использовании библиотеки в анализаторе.

    В PVS-Studio_Cmd (а также некоторых других утилитах) мы используем специальную библиотеку для разбора аргументов командной строки — CommandLine.

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

    Итак, код написан, компилируется, запускаю на исполнение, иии…

    Picture 3


    Исполнение кода переходит внутрь библиотеки, где возникает исключение типа NullReferenceException. Со стороны не очень понятно — явных никаких нулевых ссылок в метод я не передаю.

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

    Picture 2


    В комментариях к методу информации ни о каком NullReferenceException нет (что, впрочем, ожидаемо).

    Чтобы посмотреть, из-за чего конкретно возникает исключение (и где), решил загрузить исходный код проекта, собрать и подключить к анализатору отладочную версию библиотеки. Исходный код проекта доступен на GitHub. Необходима версия 1.9.71, так как именно такая сейчас используется в анализаторе.

    Загружаю соответствующую версию исходного кода, собираю, подключаю отладочную библиотеку к анализатору, запускаю код на исполнение и вижу:

    Picture 4


    Итак, место возникновения исключения понятно — helpInfo имеет значение null, из-за чего возникает исключение типа NullReferenceException при обращении к экземплярному свойству Left.

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

    Я проверил исходный код, и среди прочих предупреждений увидел именно то, на которое надеялся.

    Предупреждение PVS-Studio: V3080 Possible null dereference inside method at 'helpInfo.Left'. Consider inspecting the 2nd argument: helpInfo. Parser.cs 405

    Да, вот оно! Именно то, что нужно. Посмотрим на исходный код чуть детальнее.

    private bool DoParseArgumentsVerbs(
      string[] args, object options, ref object verbInstance)
    {
      var verbs 
        = ReflectionHelper.RetrievePropertyList<VerbOptionAttribute>(options);
      var helpInfo 
        = ReflectionHelper.RetrieveMethod<HelpVerbOptionAttribute>(options);
      if (args.Length == 0)
      {
        if (helpInfo != null || _settings.HelpWriter != null)
        {
          DisplayHelpVerbText(options, helpInfo, null); // <=
        }
    
        return false;
      }
      ....
    }

    Анализатор выдаёт предупреждение на вызов метода DisplayHelpVerbText и предупреждает о втором аргументе — helpInfo. Обратите внимание, что этот метод находится в then-ветви оператора if. Условное выражение составлено таким образом, что then-ветвь может быть исполнена при следующих значениях переменных:

    • helpInfo == null;
    • _settings.HelpWriter != null;

    Посмотрим тело метода DisplayHelpVerbText:

    private void DisplayHelpVerbText(
      object options, Pair<MethodInfo, 
      HelpVerbOptionAttribute> helpInfo, string verb)
    {
      string helpText;
      if (verb == null)
      {
        HelpVerbOptionAttribute.InvokeMethod(options, helpInfo, null, out helpText);
      }
      else
      {
        HelpVerbOptionAttribute.InvokeMethod(options, helpInfo, verb, out helpText);
      }
    
      if (_settings.HelpWriter != null)
      {
        _settings.HelpWriter.Write(helpText);
      }
    }

    Так как verb == null (см. вызов метода) нас интересует then-ветвь оператора if. Хотя с else ветвью ситуация будет аналогична, рассматривать будем then-ветвь, так как в нашем частном случае именно через неё шло исполнение. Помним, что helpInfo может иметь значение null.

    Теперь посмотрим на тело метода HelpVerbOptionAttribute.InvokeMethod. Собственно, его вы уже видели на скриншоте выше:

    internal static void InvokeMethod(
        object target,
        Pair<MethodInfo, HelpVerbOptionAttribute> helpInfo,
        string verb,
        out string text)
    {
      text = null;
      var method = helpInfo.Left;
      if (!CheckMethodSignature(method))
      {
        throw new MemberAccessException(
          SR.MemberAccessException_BadSignatureForHelpVerbOptionAttribute
            .FormatInvariant(method.Name));
      }
    
      text = (string)method.Invoke(target, new object[] { verb });
    }

    helpInfo.Left вызывается безусловно, при том, что helpInfo может иметь значение null. Об этом предупреждал анализатор, это и произошло.

    Заключение

    Забавно получилось, что с помощью PVS-Studio удалось найти ошибку в коде библиотеки, которая используется в PVS-Studio. Я думаю, это своего рода продолжение ответа на вопрос «Находит ли PVS-Studio ошибки в коде PVS-Studio?». :) Может находить ошибки не только в коде PVS-Studio, но и в коде используемых библиотек.

    Напоследок предложу скачать анализатор и попробовать проверить свой проект — вдруг и там удастся найти что-нибудь интересное?



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. The story of how PVS-Studio found an error in the library used in… PVS-Studio
    PVS-Studio
    1,019.66
    Static Code Analysis for C, C++, C# and Java
    Share post

    Comments 17

      +2
      А зачем там вообще: if (verb == null)…?
      В чём сакральный смысл вызывать HelpVerbOptionAttribute.InvokeMethod() с разными значениями параметра verb в зависимости от его же самого?
        +3
        На этот вопрос ответа у меня нет. :)

        P.S. Про сакральные смыслы — это ещё пустяки. В библиотеках .NET Core встречались куда более интересные места. Вот там действительно сидишь и думаешь, а в чём здесь скрытый смысл… Впрочем, это тема другой статьи, не будем забегать вперёд.
          0
          Смысла нет, но могу предположить что в какой-то версии вызывались разные методы или с разными аргументами. После рефакторинга остался один метод с такой странной логикой.
          Ну, это как идея, чтобы оправдать автора библиотеки :)
          0
          Если уж первый С++ компилятор был написан на С++, то, почему-бы, PVS не анализировать исходный код PVS :)
            +5
            Занимаемся этим на регулярной основе :)
              +1
              Я не знаток истории, но объясните мне, как ПЕРВЫЙ C++ компилятор мог быть написан на C++?
                +2
                Короткий ответ: макросы препроцессора языка С (и не сравниваем современный С++ с его первой версией, разумеется)
                Длинный овтет: есть «биографическая» книжка Страуструпа «Дизайн и эволюция языка С++». Очень интересная.
                0

                .

                +1
                В PVS-Studio_Cmd (а также некоторых других утилитах) мы используем специальную библиотеку для разбора аргументов командной строки — CommandLine.

                Тут вспомился анекдот:
                Экскурсовод: А сейчас мы проезжаем мимо публичного дома…
                Экскурсант: А почему???
                  +4
                  А интересно, возможна ли такая ошибка, из-за наличия которой PVS-Studio пропустит (не найдёт) эту же самую ошибку в своем коде?
                    +1
                    Теоретически, думаю, такая ситуация возможна.

                    Сходу могу придумать такой синтетический пример.

                    Возьмём, например, диагностику V3004 (then и else ветви оператора if имеют идентичные тела). Естественно, что диагностика содержит различные проверки. В частности, нам нет смысла анализировать код, если оператор if не имеет else-ветви. Допустим, есть проверка подобного вида.
                    if (ifStatement.Else == null)
                      return;
                    .... // Дальнейшая логика по определению ошибки

                    Соответственно, ошибка, выявляемая V3004, в данном случае будет выглядеть как:
                    if (ifStatement.Else == null)
                      return; // <=
                    else
                      return; // <=
                    .... // Дальнейшая логика по определению ошибки

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

                    Пример, конечно, синтетический, но на вопрос ответить должен. :)
                      +1
                      Но на практике есть юнит-тесты, которые сразу выявят, что диагностика не работает должным образом :). Мы всегда за комплексное использование разных методологий. При разработке PVS-Studio используется статические анализаторы кода, обзоры кода, юнит-тесты, динамические анализаторы, регрессионные тесты, UI тесты.
                    0
                    Библиотека то неплохая, вы завели там issue?
                      –4
                      Issue не заводил. Уже с десяток релизов прошло, актуальна ли проблема на последней доступной версии — вопрос. Не проверял.
                      0
                      Как у вас с ложными срабатываниями? Недавно клиент анализировал наш код в Checkmarx, вывалило под 1000 уязвимостей, из них реальных багов 3-4 и потенциальных с десяток.
                      Мы используем годами вылизанную libjpeg, только в ней он нашел полторы сотни ошибок.

                      0

                      Так что, вы подключаемые либы не прогоняете перед использованием?

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