PVS-Studio хотел, но не смог найти баги в robots.txt

    Picture 1

    На днях Google опубликовал исходники парсера robots.txt. Почему бы не прогнать уже проверенный всеми вдоль и поперек проект через PVS-Studio и, возможно, найти ошибку. Сказано — сделано. Жаль, что ничего значимого найти не удалось. Ну что ж, тогда пусть это будет просто повод похвалить разработчиков Google.

    robots.txt – индексный файл, который содержит правила для поисковых роботов. Он действует для протоколов https, http и FTP. Google сделала доступным для всех свой парсер файла robots.txt. Подробнее об этой новости можно почитать здесь: Google открывает исходный код парсера robots.txt

    Думаю, большинству читающих наши статьи известно, что делает PVS-Studio. Но на случай, если вы впервые в нашем блоге, дадим краткую справку. PVS-Studio – статический анализатор кода, который позволяет находить разнообразные ошибки, уязвимости и недочеты в проектах, написанных на С, С++, С# и Java. Другими словами, PVS-Studio является SAST решением и может работать как на пользовательских машинах или сборочных серверах, так и в облаке. А ещё команда PVS-Studio очень любит писать статьи о проверке различных проектов. Так что перейдем к делу и попробуем найти ошибки в исходном коде парсера от Google.

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

    В общем эта статья получилась в духе другой нашей публикации "Самая короткая статья о проверке nginx".

    Нашлась возможность небольшой оптимизации:

    V805 Decreased performance. It is inefficient to identify an empty string by using 'strlen(str) > 0' construct. A more efficient way is to check: str[0] != '\0'. robots.cc 354

    bool RobotsTxtParser::GetKeyAndValueFrom(char **key, ....)
    {
      ....  
      *key = line;
      ....
      if (strlen(*key) > 0) {
        ....
        return true;
      }
      return false;
    }

    Вызов функции strlen для того, чтобы узнать, является ли строка непустой — это неэффективный способ. Такую проверку можно произвести гораздо проще: if (*key[0] != '\0'), и не нужно будет проходить по всем элементам строки, если она непустая.

    V808 'path' object of 'basic_string' type was created but was not utilized. robots.cc 123

    std::string GetPathParamsQuery(....)
    {
      std::string path;
      ....
    }

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

    В следующем случае анализатор, по сути, дает рекомендацию добавить дефолтный return после того, как весь main будет отработан. Возможно, стоило бы добавить return в самом конце, чтобы можно было понять, что все действительно отработало. Однако если данное поведение и было задумано, и изменять ничего не нужно, а сообщение анализатора видеть бы не хотелось, то в случае PVS-Studio можно подавить это предупреждение и никогда его больше не видеть :).

    V591 The 'main' function does not return a value, which is equivalent to 'return 0'. It is possible that this is an unintended behavior. robots_main.cc 99

    int main(int argc, char** argv)
    {
      ....
      if (filename == "-h" || filename == "-help" || filename == "--help")
      {
        ShowHelp(argc, argv);
        return 0;
      }
      if (argc != 4)
      {
        ....
        return 1;
      }
      if (....)
      {
        ....
        return 1;
      }
      ....
      if (....)
      {
        std::cout << "...." << std::endl;
      }
    }

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

    V524 It is odd that the body of 'MatchDisallow' function is fully equivalent to the body of 'MatchAllow' function. robots.cc 645

    int MatchAllow(absl::string_view path, absl::string_view pattern)
    {
      return Matches(path, pattern) ? pattern.length() : -1;
    }
    
    int MatchDisallow(absl::string_view path, absl::string_view pattern)
    {
      return Matches(path, pattern) ? pattern.length() : -1;
    } 

    Это единственное место, которое вызывает у меня какое-то подозрение. Его стоит проверить авторам проекта.

    Таким образом, проверка парсера robots.txt от Google показала, что столь активно используемый и, скорее всего, многократно проверенный на ошибки проект, имеет высокое качество кода. А найденные недочеты совсем не могут испортить впечатление от того, какие крутые кодеры из Google занимались этим проектом :).

    Предлагаем и вам скачать и попробовать PVS-Studio на интересующем вас проекте.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Victoria Khanieva. PVS-Studio wanted but couldn't find bugs in robots.txt
    PVS-Studio
    695,06
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Похожие публикации

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

      +39

      Оказывается, из ничего тоже можно сделать статью на Хабр!

        +3
        Подскажите, вы планируете сделать плагин для JetBrains Rider?
          0
          if (strlen(*key) > 0)
          Такую проверку можно произвести гораздо проще: if (*key[0] != '\0')
          Это конечно так, но не надо забывать, что приоритет операторов другой — *(key[0]) вместо более близкого по смыслу (*key)[0]
            +2

            Интересно, разве компилятор не делает эту оптимизацию для strlen(*key) > 0 самостоятельно? Вроде не бы не рокет сайнс.

              +2
              Делает. И clang и gcc и msvc делают.
                +6
                Выходит, что предлагаемая замена не даёт обещанного прироста в перфомансе, зато читается хуже и вызывает больше вопросов. Такая подсказка анализатора скорее вредна, чем безобидна?
                  +2
                  Сложный вопрос. А вдруг вы этот код будете славным Borland C++ 3.1 компилировать?

                  Но так-то да, я бы, скорее, это оставил компилятору…
                    0

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

              +1

              А тут это ничего не изменит т.к. [0] справа и звёздочка слева это одно и то же.
              Можно было ещё написать **key или key[0][0].
              Но это тут, где индекс нулевой, а вообще да, я в таких конструкциях (когда одновременно и префиксный и постфиксный операторы есть) всегда скобки ставлю чтобы сразу видеть желаемый и фактический порядок действий.

                +4
                Это изменит восприятие кода человеком.
                Если после кода if (*key[0] != '\0') вдруг понадобится проверить следующий символ в строке, то другой человек запросто может скопипастить и заменить на if (*key[1] != '\0').
                  0
                  может для правильного восприятия и лучшей читаемости макрос препроцессора использовать?
              +8
              А может гугл уже проверил через PVS-Studio и исправил важные ошибки?)
                0
                Ни гугла, ни альфабет до сих пор нет в списке клиентов этой маленькой студии…
                  0
                  Мне кажется, они должны какие-то другие анализаторы использовать

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

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