Chromium: опечатки

    Опечатки
    Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium. Это четвёртая часть, которая будет посвящена проблеме опечаток и написанию кода с помощью «Copy-Paste метода».

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

    Появление дополнительных ошибок провоцирует методика написания кода с помощью Copy-Paste. К сожалению, копировать код — это эффективный метод, и ничего с этим не сделать. Намного быстрее скопировать несколько строк и что-то в них поправить, чем написать фрагмент кода заново. Я даже не буду стараться отговорить от этого метода, так как сам грешен и использую копирование кода. Неприятным побочным эффектом копирования является то, что в скопированном фрагменте кода забывают что-то изменить. Эти ошибки в каком-то смысле тоже являются опечатками: по невнимательности забыли что-то изменить в тексте или изменили неправильно.

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

    Разыменование нулевого указателя


    Согласно Common Weakness Enumeration ошибки разыменования нулевого указателя можно классифицировать как CWE-476.

    Приведённые далее ошибки относятся к коду проекта Chromium.

    class Display : ....
    {
      ....
      std::unique_ptr<FocusController> focus_controller_;
      ....  
    }
    
    Display::~Display() {
      ....
      if (!focus_controller_) {
        focus_controller_->RemoveObserver(this);
        focus_controller_.reset();
      }
      ....
    }

    Неправильно написано условие. Указатель разыменовывается, если он нулевой. Символ '!' здесь явно лишний.

    Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'focus_controller_' might take place. display.cc 52

    Следующая ошибка достойна носить звание «Классической классики».

    void AppViewGuest::CreateWebContents(....) {
      ....
      if (!guest_extension ||
          !guest_extension->is_platform_app() ||
          !embedder_extension |
          !embedder_extension->is_platform_app()) {
        callback.Run(nullptr);
        return;
      }
      ....
    }

    Опечатка. Вместо оператора '||' случайно написали оператор '|'. В результате, указатель embedder_extension разыменовывается независимо от того, нулевой он или нет.

    Примечание. Если статью читает кто-то, слабо знакомый с языком C++, то предлагаю ознакомиться со статьёй "Short-circuit evaluation", чтобы понять, в чём тут дело.

    Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'embedder_extension' might take place. Check the bitwise operation. app_view_guest.cc 186

    Следующая ошибка связана с тем, что код не дописан. Я думаю, это тоже можно рассматривать как опечатку. Должны были присвоить какое-то значение умному указателю, но забыли.

    std::unique_ptr<base::ListValue>
    NetworkingPrivateServiceClient::GetEnabledNetworkTypes() {
      std::unique_ptr<base::ListValue> network_list;
      network_list->AppendString(::onc::network_type::kWiFi);
      return network_list;
    }

    Умный указатель по умолчанию является нулевым. Раз этот указатель после создания не изменяется, то произойдёт разыменование нулевого указателя.

    Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'network_list' might take place. networking_private_service_client.cc 351

    Давайте теперь изучим какой-нибудь более сложный случай.

    Response CSSAgent::getMatchedStylesForNode(int node_id,
      Maybe<CSS::CSSStyle>* inline_style)
    {
      UIElement* ui_element = dom_agent_->GetElementFromNodeId(node_id);
      *inline_style = GetStylesForUIElement(ui_element);
      if (!inline_style)
        return NodeNotFoundError(node_id);
      return Response::OK();
    }

    Предупреждение PVS-Studio: V595 CWE-476 The 'inline_style' pointer was utilized before it was verified against nullptr. Check lines: 142, 143. css_agent.cc 142

    Указатель inline_style разыменовывается до проверки на равенство nullptr. Мне кажется, это следствие опечатки: здесь просто пропустили символ звёздочка '*'. В этом случае корректный код должен выглядеть так:

    *inline_style = GetStylesForUIElement(ui_element);
    if (!*inline_style)
      return NodeNotFoundError(node_id);

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

    Response CSSAgent::getMatchedStylesForNode(int node_id,
      Maybe<CSS::CSSStyle>* inline_style)
    {
      UIElement* ui_element = dom_agent_->GetElementFromNodeId(node_id);
      if (!inline_style)
        return NodeNotFoundError(node_id);
      *inline_style = GetStylesForUIElement(ui_element);
      return Response::OK();
    }

    У меня закончились ошибки разыменования нулевого указателя, относящиеся к коду Chromium, но есть ещё одна, замеченная мною в движке V8.

    bool Object::IsSmi() const { return HAS_SMI_TAG(this); }
    
    bool IC::IsHandler(Object* object) {
      return (object->IsSmi() && (object != nullptr)) ||
             object->IsDataHandler() ||
             object->IsWeakCell() || 
             object->IsCode();
    }

    Указатель object вначале разыменовывается, а уже потом проверяется на равенство nullptr. Да и вообще выражение выглядит как-то подозрительно. Очень похоже на опечатку при поспешном программировании: сначала написали object->IsSmi(), потом вспомнили, что надо проверить указатель object на равенство nullptr и добавили проверку. А подумать забыли :).

    Анализатор PVS-Studio выдаёт здесь сразу два предупреждения:

    • V522 CWE-628 Dereferencing of the null pointer 'object' might take place. The null pointer is passed into 'IsHandler' function. Inspect the first argument. Check lines: 'ic-inl.h:44', 'stub-cache.cc:19'. ic-inl.h 44
    • V713 CWE-476 The pointer object was utilized in the logical expression before it was verified against nullptr in the same logical expression. ic-inl.h 44

    Copy-Paste


    Невозможно классифицировать ошибки, допущенные из-за Copy-Paste, согласно Common Weakness Enumeration. Нет такого дефекта, как «Copy-Paste» :). Разные опечатки будут приводить к дефектам разного типа. Далее я покажу ошибки, которые можно классифицировать как:

    • CWE-563: Assignment to Variable without Use
    • CWE-570: Expression is Always False
    • CWE-571: Expression is Always True
    • CWE-682: Incorrect Calculation
    • CWE-691: Insufficient Control Flow Management

    Начнём вновь с кода, относящегося непосредственно к проекту Chromium.

    void ProfileSyncService::OnEngineInitialized(....)
    {
      ....
      std::string signin_scoped_device_id;
      if (IsLocalSyncEnabled()) {
        signin_scoped_device_id = "local_device";
      } else {
        SigninClient* signin_client = ....;
        DCHECK(signin_client);
        std::string signin_scoped_device_id =                 // <=
            signin_client->GetSigninScopedDeviceId();
      }
      ....
    }

    Я прямо чувствую, как программисту было лень набирать вновь имя переменной signin_scoped_device_id. И он решил скопировать это имя. Однако случайно, вместе с именем, он скопировал и тип std::string:

    std::string signin_scoped_device_id

    Следствием лени стало то, что возвращенное функцией GetSigninScopedDeviceId значение будет помещено во временную переменную, которая тут же будет уничтожена.

    Предупреждение PVS_Studio: V561 CWE-563 It's probably better to assign value to 'signin_scoped_device_id' variable than to declare it anew. Previous declaration: profile_sync_service.cc, line 900. profile_sync_service.cc 906

    Следующую ошибку я встретил в движке V8, который используется в Chromium.

    static LinkageLocation ForSavedCallerReturnAddress() {
      return ForCalleeFrameSlot(
        (StandardFrameConstants::kCallerPCOffset -
         StandardFrameConstants::kCallerPCOffset) /
           kPointerSize,
        MachineType::Pointer());
    }

    Скорее всего программист скопировал StandardFrameConstants::kCallerPCOffset и хотел изменить имя константы, но забыл это сделать. Поэтому в коде константа вычитается сама из себя, в результате чего получается 0. После чего этот 0 делится на kPointerSize, но это уже не имеет никакого значения. Все равно получится 0.

    Предупреждение PVS-Studio: V501 There are identical sub-expressions 'StandardFrameConstants::kCallerPCOffset' to the left and to the right of the '-' operator. linkage.h 66

    Ещё один подозрительный фрагмент кода, замеченный в V8:

    void JSObject::JSObjectShortPrint(StringStream* accumulator) {
      ....
      accumulator->Add(global_object ? "<RemoteObject>" :
                                       "<RemoteObject>");
      ....
    }

    Предупреждение PVS-Studio: V583 CWE-783 The '?:' operator, regardless of its conditional expression, always returns one and the same value: "<RemoteObject>". objects.cc 2993

    Теперь заглянем в проект PDFium.

    inline bool FXSYS_iswalpha(wchar_t wch) {
      return FXSYS_isupper(wch) || FXSYS_islower(wch);
    }
    
    bool CPDF_TextPage::IsHyphen(wchar_t curChar) const {
      WideStringView curText = m_TempTextBuf.AsStringView();
      ....
      auto iter = curText.rbegin();
      ....
      if ((iter + 1) != curText.rend()) {
        iter++;
        if (FXSYS_iswalpha(*iter) && FXSYS_iswalpha(*iter))    // <=
          return true;
      }
      ....
    }

    Скопировали FXSYS_iswalpha(*iter). И… И что-то забыли изменить во второй части условия.

    Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions 'FXSYS_iswalpha(* iter)' to the left and to the right of the '&&' operator. cpdf_textpage.cpp 1218

    Схожую ошибку при написании выражения можно встретить в библиотеке Protocol buffers.

    bool IsMap(const google::protobuf::Field& field,
               const google::protobuf::Type& type) {
     return
       field.cardinality() == 
         google::protobuf::Field_Cardinality_CARDINALITY_REPEATED
       &&
       (GetBoolOptionOrDefault(type.options(), "map_entry", false) ||
        GetBoolOptionOrDefault(type.options(),
          "google.protobuf.MessageOptions.map_entry", false) || // <=
        GetBoolOptionOrDefault(type.options(),
          "google.protobuf.MessageOptions.map_entry", false));  // <=
    }

    Код явно написан с помощью Copy-Paste. Никто не будет повторно набирать такую длинную строчку :).

    Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. utility.cc 351

    Кстати, рядышком есть ещё одна такая же ошибка: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. utility.cc 360

    В следующем фрагменте кода из библиотеки SwiftShader нас ждёт мой любимый "эффект последней строки". Красивый баг! Люблю такие.

    void TextureCubeMap::updateBorders(int level)
    {
      egl::Image *posX = image[CubeFaceIndex(..._POSITIVE_X)][level];
      egl::Image *negX = image[CubeFaceIndex(..._NEGATIVE_X)][level];
      egl::Image *posY = image[CubeFaceIndex(..._POSITIVE_Y)][level];
      egl::Image *negY = image[CubeFaceIndex(..._NEGATIVE_Y)][level];
      egl::Image *posZ = image[CubeFaceIndex(..._POSITIVE_Z)][level];
      egl::Image *negZ = image[CubeFaceIndex(..._NEGATIVE_Z)][level];
      ....
      if(!posX->hasDirtyContents() ||
         !posY->hasDirtyContents() ||
         !posZ->hasDirtyContents() ||
         !negX->hasDirtyContents() ||
         !negY->hasDirtyContents() ||          // <=
         !negY->hasDirtyContents())            // <=
      {
        return;
      }
      ....
    }

    В самом конце условия следовало использовать не указатель negY, а указатель negZ. Явно имеем дело с Copy-Paste строки кода и потерей внимания в самом конце.

    Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions '!negY->hasDirtyContents()' to the left and to the right of the '||' operator. texture.cpp 1268

    Движок WebKit тоже порадовал красивым багом:

    bool IsValid(....) const final {
      OptionalRotation inherited_rotation =
        GetRotation(*state.ParentStyle());
      if (inherited_rotation_.IsNone() ||
          inherited_rotation.IsNone())
        return inherited_rotation.IsNone() ==
               inherited_rotation.IsNone();
      ....
    }

    Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions 'inherited_rotation.IsNone()' to the left and to the right of the '==' operator. cssrotateinterpolationtype.cpp 166

    Скопировали inherited_rotation.IsNone() и забыли добавить символ подчёркивания '_'. Правильный вариант:

    return inherited_rotation_.IsNone() ==
           inherited_rotation.IsNone();

    Вновь заглянем в библиотеку Protocol buffers.
    void SetPrimitiveVariables(....,
                               std::map<string, string>* variables) {
      ....
      (*variables)["set_has_field_bit_message"] = "";
      (*variables)["set_has_field_bit_message"] = "";
      (*variables)["clear_has_field_bit_message"] = "";
      ....
    }

    Пояснения излишни. Copy-Paste в чистом виде. Предупреждение PVS-Studio: V519 CWE-563 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 149, 150. java_primitive_field_lite.cc 150

    Продолжим. Программисты должны знать, сколь коварен Copy-Paste. Читайте и ужасайтесь. Перед нами функция, взятая из WebRTC.

    size_t WebRtcSpl_FilterAR(....)
    {
      ....
      for (i = 0; i < state_length - x_length; i++)
      {
        state[i] = state[i + x_length];
        state_low[i] = state_low[i + x_length];
      }
      for (i = 0; i < x_length; i++)
      {
        state[state_length - x_length + i] = filtered[i];
        state[state_length - x_length + i] = filtered_low[i];  // <=
      }
      ....
    }

    Да, вновь последствия Copy-Paste. Скопировали строку:

    state[state_length - x_length + i] = filtered[i];

    Заменили в ней filtered на filtered_low. А вот заменить state на state_low забыли. В результате часть элементов массива state_low остаются неинициализированными.

    Вы устали читать? Представьте, как я устал это писать! Давайте передохнём и выпьем кофе.

    Picture 3



    ПАУЗА.

    Надеюсь вы восстановили силы и готовы продолжить наслаждаться 50-тью оттенками Copy-Paste. Вот что можно увидеть в библиотеке PDFium.

    bool CPWL_EditImpl::Backspace(bool bAddUndo, bool bPaint) {
      ....
        if (m_wpCaret.nSecIndex != m_wpOldCaret.nSecIndex) {
          AddEditUndoItem(pdfium::MakeUnique<CFXEU_Backspace>(
              this, m_wpOldCaret, m_wpCaret, word.Word, word.nCharset));
        } else {
          AddEditUndoItem(pdfium::MakeUnique<CFXEU_Backspace>(
              this, m_wpOldCaret, m_wpCaret, word.Word, word.nCharset));
        }
      ....
    }

    Независимо от условия выполняется одно и тоже действие. Скорее всего, здесь неудачный Copy-Paste. Программист скопировал фрагмент, потом отвлёкся и забыл поправить else-ветку.

    Предупреждения PVS-Studio: V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpwl_edit_impl.cpp 1580

    Есть ещё три таких же места. Я пожалею читателя и приведу только сообщения анализатора:

    • V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpwl_edit_impl.cpp 1616
    • V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpdf_formfield.cpp 172
    • V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cjs_field.cpp 2323

    Библиотека Skia.

    bool SkPathRef::isValid() const {
      ....
      if (nullptr == fPoints && 0 != fFreeSpace) {
        return false;
      }
      if (nullptr == fPoints && 0 != fFreeSpace) {
        return false;
      }
      ....
    }

    Два раза выполняется одинаковая проверка. Вторая проверка лишняя. Или забыли проверить что-то другое. Анализатор сигнализирует о подозрительности этого кода сразу двумя предупреждениями:

    • V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 758, 761. skpathref.cpp 761
    • V649 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 758, 761. skpathref.cpp 761

    И ещё одна ошибка в библиотеке Skia.

    static inline bool can_blit_framebuffer_for_copy_surface(
      const GrSurface* dst, GrSurfaceOrigin dstOrigin,
      const GrSurface* src, GrSurfaceOrigin srcOrigin, ....)
    {
      ....
      const GrGLTexture* dstTex =
        static_cast<const GrGLTexture*>(dst->asTexture());
      const GrGLTexture* srcTex =
        static_cast<const GrGLTexture*>(dst->asTexture());     // <=
    
      const GrRenderTarget* dstRT = dst->asRenderTarget();
      const GrRenderTarget* srcRT = src->asRenderTarget();
    
      if (dstTex && dstTex->target() != GR_GL_TEXTURE_2D) {
        return false;
      }
      if (srcTex && srcTex->target() != GR_GL_TEXTURE_2D) {
        return false;
      }
      ....
    }

    Предупреждение PVS-Studio: V656 Variables 'dstTex', 'srcTex' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 3312, 3313. grglgpu.cpp 3313

    После Copy-Paste забыли заменить dst на src. Должно быть написано:

    const GrGLTexture* srcTex =
      static_cast<const GrGLTexture*>(src->asTexture());

    Библиотека HarfBuzz.

    inline int get_kerning (hb_codepoint_t left,
                            hb_codepoint_t right,
                            const char *end) const
    {
      unsigned int l = (this+leftClassTable).get_class (left);
      unsigned int r = (this+leftClassTable).get_class (left);  // <=
      unsigned int offset = l * rowWidth + r * sizeof (FWORD);
      ....
    }

    Предупреждение PVS-Studio: V751 Parameter 'right' is not used inside function body. hb-ot-kern-table.hh 115

    Красивая ошибка. Скопировали строчку, но забыли два раза поменять left на right. Должно быть:

    unsigned int l = (this+leftClassTable).get_class (left);
    unsigned int r = (this+rightClassTable).get_class (right);

    Библиотека SwiftShader. Уверен, эти однотипные фрагменты кода писались с использование Copy-Paste:

    class ELFObjectWriter {
      ....
      ELFStringTableSection *ShStrTab;
      ELFSymbolTableSection *SymTab;
      ELFStringTableSection *StrTab;
      ....
    };
    
    void ELFObjectWriter::assignSectionNumbersInfo(
      SectionList &AllSections)
    {
      ....
      ShStrTab->setNumber(CurSectionNumber++);
      ShStrTab->setNameStrIndex(ShStrTab->getIndex(ShStrTab->getName()));
      AllSections.push_back(ShStrTab);
    
      SymTab->setNumber(CurSectionNumber++);
      SymTab->setNameStrIndex(ShStrTab->getIndex(SymTab->getName()));
      AllSections.push_back(SymTab);
    
      StrTab->setNumber(CurSectionNumber++);
      StrTab->setNameStrIndex(ShStrTab->getIndex(StrTab->getName()));
      AllSections.push_back(StrTab);
      ....
    }

    Программист был невнимателен. Во втором блоке текста он забыл заменить hStrTab->getIndex на на SymTab->getIndex, а в третьем блоке не заменил hStrTab->getIndex на StrTab->getIndex.

    Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'SymTab' variable should be used instead of 'ShStrTab'. iceelfobjectwriter.cpp 194

    Следующая ошибка связана с неправильным вычислением размера прямоугольника в библиотеке WebKit. Код «вырви глаз». Слабо заметить ошибку?

    void NGFragmentBuilder::ComputeInlineContainerFragments(....)
    {
      ....
      value.start_fragment_union_rect.size.width =
        std::max(descendant.offset_to_container_box.left +
             descendant.fragment->Size().width -
             value.start_fragment_union_rect.offset.left,
           value.start_fragment_union_rect.size.width);
      value.start_fragment_union_rect.size.height =
        std::max(descendant.offset_to_container_box.top +
             descendant.fragment->Size().height -
             value.start_fragment_union_rect.offset.top,
           value.start_fragment_union_rect.size.width);
      ....
    }

    В самом конце забыли заменить в скопированном блоке width на height.

    Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'height' variable should be used instead of 'width'. ng_fragment_builder.cc 326

    Уфф… Заканчиваем. Последний фрагмент кода в этом разделе взят из библиотеки PDFium.

    void sycc420_to_rgb(opj_image_t* img) {
      ....
      opj_image_data_free(img->comps[0].data);
      opj_image_data_free(img->comps[1].data);
      opj_image_data_free(img->comps[2].data);
      img->comps[0].data = d0;
      img->comps[1].data = d1;
      img->comps[2].data = d2;
      img->comps[1].w = yw;                 // 1
      img->comps[1].h = yh;                 // 1
      img->comps[2].w = yw;                 // 1
      img->comps[2].h = yh;                 // 1
      img->comps[1].w = yw;                 // 2
      img->comps[1].h = yh;                 // 2
      img->comps[2].w = yw;                 // 2
      img->comps[2].h = yh;                 // 2
      img->comps[1].dx = img->comps[0].dx;
      img->comps[2].dx = img->comps[0].dx;
      img->comps[1].dy = img->comps[0].dy;
      img->comps[2].dy = img->comps[0].dy;
    }

    Повторяющийся блок присваиваний. Предупреждение PVS-Studio: V760 Two identical blocks of text were found. The second block begins from line 420. fx_codec_jpx_opj.cpp 416

    Нет, я вас обманул. Встретился ещё один Copy-Paste из PDFium. Пришлось добавить в статью.

    void Transform(int x, int y, int* x1,
                   int* y1, int* res_x, int* res_y) const
    {
      ....
      if (*res_x < 0 && *res_x > -kBase)
        *res_x = kBase + *res_x;
      if (*res_y < 0 && *res_x > -kBase)
        *res_y = kBase + *res_y;
      }
    }

    Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'res_y' variable should be used instead of 'res_x'. cfx_imagetransformer.cpp 201

    Была скопирована строчка:

    if (*res_x < 0 && *res_x > -kBase)

    Одно имя переменной res_x заменили на res_y, а второе забыли. В результате в функции отсутствует проверка условия *res_y > -kBase.

    Другие опечатки


    Если опечатки типа «нулевые указатели» и «Copy-Paste» выделились как-то сами собой, то оставшиеся ошибки достаточно разнородны. Я не стал пытаться их как-то классифицировать и просто собрал в этом разделе статьи.

    Начнем с фрагмента кода, относящегося к проекту Chromium. Я не уверен, что это ошибка. Однако это место явно стоит проверить разработчикам.

    namespace cellular_apn {
      const char kAccessPointName[] = "AccessPointName";
      const char kName[] = "Name";
      const char kUsername[] = "Username";
      const char kPassword[] = "Password";
      const char kLocalizedName[] = "LocalizedName";
      const char kLanguage[] = "LocalizedName";
    }

    Подозрительно то, что константы kLocalizedName и kLanguage содержат в себе одну и ту же строку. Рискну предположить, что должно быть написано вот так:
    const char kLanguage[] = "Language";

    Впрочем, я не уверен.

    Анализатор PVS-Studio выдаёт здесь предупреждение: V691 Empirical analysis. It is possible that a typo is present inside the string literal: «LocalizedName». The 'Localized' word is suspicious. onc_constants.cc 162

    Следующая ошибка в библиотеке Skia просто прекрасна и отсылает нас к статье "Зло живёт в функциях сравнения".

    inline bool operator==(const SkPDFCanon::BitmapGlyphKey& u,
                           const SkPDFCanon::BitmapGlyphKey& v) {
      return memcmp(&u, &u, sizeof(SkPDFCanon::BitmapGlyphKey)) == 0;
    }

    Из-за опечатки объект u сравнивается сам с собой. Получается, что operator == считает любые два объекта одинаковыми.

    Предупреждение PVS-Studio: V549 CWE-688 The first argument of 'memcmp' function is equal to the second argument. skpdfcanon.h 67

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

    SkString dumpInfo() const override {
      SkString str;
      str.appendf("# combined: %d\n", fRects.count());
      for (int i = 0; i < fRects.count(); ++i) {
        const RectInfo& geo = fRects[0];
        str.appendf("%d: Color: 0x%08x, "
                    "Rect [L: %.2f, T: %.2f, R: %.2f, B: %.2f]\n", i,
                    geo.fColor, geo.fRect.fLeft, geo.fRect.fTop,
                    geo.fRect.fRight, geo.fRect.fBottom);
      }
      str += fHelper.dumpInfo();
      str += INHERITED::dumpInfo();
      return str;
    }

    Вместо fRects[0] должно быть написано fRects[i]. Предупреждение PVS-Studio: V767 Suspicious access to element of 'fRects' array by a constant index inside a loop. grnonaafillrectop.cpp 276

    Опечатка в проекте SwiftShader приводит к тому, что макрос assert проверяет не всё, что требуется.

    static Value *createArithmetic(Ice::InstArithmetic::OpKind op,
                                   Value *lhs, Value *rhs)
    {
      assert(lhs->getType() == rhs->getType() ||
             (llvm::isa<Ice::Constant>(rhs) &&
              (op == Ice::InstArithmetic::Shl ||
               Ice::InstArithmetic::Lshr ||
               Ice::InstArithmetic::Ashr)));
      ....
    }

    Два раза забыли написать «op ==». В результате, частью условия являются константы Ice::InstArithmetic::Lshr и Ice::InstArithmetic::Ashr, которые ни с чем не сравниваются. Это явная ошибка, из-за которой часть условия всегда истинно.

    Здесь на самом деле должно быть написано:

    assert(lhs->getType() == rhs->getType() ||
           (llvm::isa<Ice::Constant>(rhs) &&
            (op == Ice::InstArithmetic::Shl ||
             op == Ice::InstArithmetic::Lshr ||
             op == Ice::InstArithmetic::Ashr)));

    Анализатор PVS-Studio выдаёт два предупреждения:
    • V768 CWE-571 The enumeration constant 'Lshr' is used as a variable of a Boolean-type. subzeroreactor.cpp 712
    • V768 CWE-571 The enumeration constant 'Ashr' is used as a variable of a Boolean-type. subzeroreactor.cpp 712

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

    Одну из таких переменных мы можем наблюдать в библиотеке Yasm:

    static int i;  /* The t_type of tokval */

    Предупреждение PVS-Studio: V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'i' variable. nasm-eval.c 29

    Да, это ещё не ошибка. Но очень легко где-то забыть объявить локальную переменную i и воспользоваться глобальной. Причем код скомпилируется, но как после этого будет работать программа — большая загадка. В общем, лучше давать глобальным переменным какие-то более уникальные имена.

    Надеюсь, я смог донести всю серьезность ошибок, которые возникают из-за опечаток!

    Рисунок 1



    Напоследок, предлагаю познакомиться с красивой опечаткой в библиотеке Protocol buffers, которую я описал в отдельной заметке — "31 февраля".

    Рекомендации


    Прошу прощения, в этот раз я вас подведу с разделом «рекомендации». Очень сложно дать какие-то чёткие универсальные советы, чтобы избегать ошибок, рассмотренных в статье. Человеку свойственно делать такие ошибки, и всё тут.

    Тем не менее, я всё-таки попробую хоть как-то помочь, чтобы уменьшить количество опечаток в программах.

    1. Используйте «табличное оформление» сложных условий. Я подробно описал эту технологию в мини-книге "Главный вопрос программирования, рефакторинга и всего такого". Смотрите совет N13 — Выравнивайте однотипный код «таблицей». Кстати, в этом году я планирую написать расширенный вариант этой книги. В неё будет входить уже не 42, а 50 рекомендаций. Плюс некоторые старые главы нуждаются в обновлении и доработке.
    2. Занимаясь Copy-Paste, сосредоточьтесь в конце работы. Это связано с "эффектом последней строки", когда в конце написания однотипных блоков текста человек расслабляется и допускает ошибки. Если хочется почитать что-то более научное на эту тему, то предлагаю статью "Объяснение эффекта последней строки".
    3. Будьте аккуратны при написании функций, сравнивающих объекты. Эти функции обманчиво просты и провоцируют появление большого количества опечаток. Подробности: "Зло живёт в функциях сравнения".
    4. Если код тяжело читать, то и заметить в нём ошибку сложно. Поэтому старайтесь писать код как можно более понятным и легко читаемым. К сожалению, это сложно формально сформулировать, тем более кратко. Этой теме посвящены многие замечательные книги, например, такая как «Совершенный код», написанная Стивом Макконнелом. С практической точки зрения рекомендую уделять внимание стандарту кодирования в вашей компании и не лениться пополнять его хорошими практиками, о которых где-то узнали. И вообще, C++ быстро развивается, поэтому есть смысл регулярно проводить аудит стандарта и обновлять его.
    5. Регулярно используйте статический анализатор кода PVS-Studio. В конце концов, все описанные в этой статье опечатки были найдены именно с помощью PVS-Studio. Скачать и попробовать анализатор можно здесь.

    Спасибо всем за внимание, но я ещё не прощаюсь. Скоро будет очередная статья.


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Chromium: Typos.
    • +34
    • 6,9k
    • 7
    PVS-Studio 281,88
    Ищем ошибки в C, C++ и C# на Windows, Linux, macOS
    Поделиться публикацией
    Комментарии 7
    • +1
      Фрагмент
      ShStrTab->getIndex

      может и не является ошибкой. Этот метод у ShStrTab вызывается не только во втором, но и в третьем блоке текста.
      • +1
        Возможно. Но проверить надо.
        • +1
          Судя по всему — не ошибка. Но вырвиглаз аж жуть.
        • 0
          А что произойдёт, если хром разыменует нулевой указатель? Вкладка крашнется, или там отлавливается как-то это?
            • 0

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

            • +1
              Если статью читает кто-то, слабо знакомый с языком C++

              Спасибо, смиялсо.

              Табличное оформление (рекомендация 1) сам очень люблю, но жить с этой любовью становится всё труднее — современные IDE норовят всё испохабить своим автоформатированием, ничего не спросив.

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

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