悪は比較機能に生きる

比較関数の悪

読者は、「The Last Lineの効果」というタイトルの私の記事を覚えているかもしれません。 これは、私が気付いたパターンを指します。ほとんどの場合、同じタイプのテキストブロックの最後の行に間違いがあります。 ここで、新しい興味深い観察についてお話したいと思います。 プログラマーは、2つのオブジェクトを比較する機能に間違いを犯しがちです。 そのような声明は信じがたいように思えますが、読者に衝撃を与えるエラーの膨大な数の例を示します。 新しい研究を読んでください、それは面白くて怖いでしょう。

発行


プログラマは、2つのオブジェクトを比較するように設計された非常に単純な関数でミスを犯すことが非常に多いと主張します。 このステートメントは、C、C ++、およびC#で開かれた多数のプロジェクトをチェックするチームの経験に基づいています。

問題の関数は、 IsEqualEqualsCompareAreEqualなどと呼ばれます。 または== ,! =などのオーバーロード演算子。

記事を書いているときに、比較関数に関連するエラーに遭遇することがよくあります。 この問題をより詳細に調査することにし、発見したエラーのデータベースを調査しました。 これを行うために、データベースでCmpEqualCompareなどの単語を含む名前の関数が検索されました。 結果は非常に印象的で衝撃的でした。

一般に、このストーリーは「The Last Lineの効果」という記事を書くことに似ています。 同様に、私は異常に気づき、それをより詳細に研究することにしました。 残念ながら、言及された記事とは異なり、私はどのような数字や統計を持ってくるべきかわかりません。 おそらく、どのように、何を計算できるか-後で考えます。 今、私は直観に導かれ、私の気持ちを共有することができるだけです。 彼らは、比較関数に多くのエラーがあると言います。そして、私が多数の印象的な例を示すとき、あなたは同じ感覚を持っていると確信しています。

心理学


最後のラインエフェクトに戻りましょう。 ちなみに、もしあなたがそれを読んでいないのなら、一時停止して彼女と知り合うことをお勧めします。 このトピックのより詳細な分析があります:「 最後の行の効果の説明 」。

一般に、最後の行のエラーの原因は、開発者が現在のフラグメントの完了に集中するのではなく、次の行/タスクにすでに心を動かしているという事実によると結論付けることができます。 その結果、同じタイプのテキストブロックを最後に書いた最後に、彼は以前のものよりもタイプミスをする可能性が高くなります。

比較関数を書くとき、開発者はそれが些細であると考えて、それに焦点を合わせないことが多いと思います。 言い換えれば、彼はそれを考えずに自動的にコードを書きます。 それ以外の場合、同様のエラーを作成する方法は明確ではありません。

bool IsLuidsEqual(LUID luid1, LUID luid2) { return (luid1.LowPart == luid2.LowPart) && (luid2.HighPart == luid2.HighPart); } 

PVS-Studioアナライザーは、 RunAsAdmin Explorer Shim (C ++)プロジェクトコードでこのエラーを検出しました: V501 '=='演算子の左右に同じ副次式があります:luid2.HighPart == luid2.HighPart RAACommon raacommonfuncs。 cpp 1511

タイプミス。 2行目は、 luid1.HighPart == luid2.HighPartになっているはずです。

同意して、コードは簡単です。 どうやら、コードの単純さはすべてを台無しにします。 プログラマーは、このような関数標準を作成して、面白くないタスクをすぐに認識します。 彼は即座に頭の中で関数の書き方を決定し、コードを実装するだけです。 これはルーチンですが、残念ながら、より重要で複雑で興味深いコードの作成に進むために必要なプロセスです。 開発者の考えはすでにそこにあり、その結果、彼は間違いを犯します。

さらに、プログラマーがこのような機能の単体テストを作成することはほとんどありません。 繰り返しますが、これらの関数の見かけ上の単純さは失敗します。 これらの関数は非常に単純で同じタイプであるため、それらをテストするのは冗長なようです。 人は自分の生活の中で何百ものそのような機能を書いたが、彼は本当に次の機能で間違いを犯すことができるのか?! たぶんそうです。

同時に、この記事はプログラミングを学ぶ学生のコードに関するものではないという事実に注意を喚起したいと思います。 GCC、Qt、GDB、LibreOffice、Unreal Engine 4、CryEngine V、Chromium、MongoDB、Oracle VM Virtual Box、FreeBSD、WinMerge、CoreCLR、MySQL、Mono、CoreFX、Roslyn、MSBuildなどのプロジェクトのコードのエラーについて話しているなど すべてが非常に深刻です。

私はあなたが寝るのが怖いほど多くの多様な例を示します。

比較関数の誤ったパターン


比較関数のすべてのエラーは、いくつかのパターンに分割されます。 この記事では、C、C ++、C#言語のプロジェクトのエラーを扱いますが、多くのパターンは異なる言語で同じであるため、これらの言語を何らかの形で分ける意味はありません。

パターン:A <B、B> A


多くの場合、比較関数では次のチェックを実行する必要があります。


プログラマーは、同じ<operatorを使用して変数名を交換する方がより美しいと感じる場合があります。


ただし、不注意のため、次のチェックが取得されます。


実際、同じ比較がここで2回実行されます。 おそらくあなたは議論されていることを理解していなかったかもしれませんが、実際の例に移り、すべてが明らかになります。

 string _server; .... bool operator<( const ServerAndQuery& other ) const { if ( ! _orderObject.isEmpty() ) return _orderObject.woCompare( other._orderObject ) < 0; if ( _server < other._server ) return true; if ( other._server > _server ) return false; return _extra.woCompare( other._extra ) < 0; } 

PVS-Studioアナライザーは、 MongoDB (C ++)プロジェクトコードでこのエラーを検出しました: V581互いに並んでいる「if」演算子の条件式は同一です。 チェック行:44、46。parallel.h 46

この状態は次のとおりです。

 if ( other._server > _server ) 

上記の2行とまったく同じチェックが実行されたため、常にfalseになります。 コードの正しいバージョン:

 if ( _server < other._server ) return true; if ( other._server < _server ) return false; 

このエラーはChromiumプロジェクトコード(C ++)にもあります。

 enum ContentSettingsType; struct EntryMapKey { ContentSettingsType content_type; ... }; bool OriginIdentifierValueMap::EntryMapKey::operator<( const OriginIdentifierValueMap::EntryMapKey& other) const { if (content_type < other.content_type) return true; else if (other.content_type > content_type) return false; return (resource_identifier < other.resource_identifier); } 

PVS-Studio 警告V517 「if(A){...} else if(A){...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 61、63行目を確認します。browser content_settings_origin_identifier_value_map.cc 61

C ++で例を示しましたが、今度はC#の番です IronPythonおよびIronRuby (C#)コードで次のエラーが見つかりました。

 public static int Compare(SourceLocation left, SourceLocation right) { if (left < right) return -1; if (right > left) return 1; return 0; } 

PVS-Studio警告(C#): V3021同一の条件式を持つ2つの「if」ステートメントがあります。 最初の「if」ステートメントにはメソッドの戻り値が含まれます。 これは、2番目の「if」ステートメントが無意味であることを意味します。 SourceLocation.cs 156

ここでの説明は不要だと思います。

ご注意 C#のエラーの例は1つしかありませんが、C ++のエラーは2つありました。 ただし、C#の方がずっと安全であるという結論に急ぐことはお勧めしません。 実際、PVS-Studioアナライザーは比較的最近C#コードを分析することを学び、CおよびC ++よりもC#で記述されたプロジェクトをはるかに少なくチェックしただけです。

パターン:クラスメンバはそれ自身と比較します


比較関数は、多くの場合、フィールド構造/クラスの順次比較で構成されます。 このようなコードは、クラスのメンバーが自分自身と比較し始めるときにエラーになりやすいです。 この場合、エラーの2つのサブタイプを区別できます。

最初のケースでは、オブジェクトの名前を示すのを忘れて、次のように書きます。

 return m_x == foo.m_x && m_y == m_y && // <= m_z == foo.m_z; 

2番目の場合、同じオブジェクト名を書き込みます。
 return zzz.m_x == foo.m_x && zzz.m_y == zzz.m_y && // <= zzz.m_z == foo.m_z; 

実際にこのパターンのエラーに慣れてみましょう。 ちなみに、誤った比較は、同じテキストブロックの最後によく見られることに注意してください。「最後の行の効果」です。

Unreal Engine 4 (C ++)プロジェクトのコードでエラーが見つかりました:

 bool Compare(const FPooledRenderTargetDesc& rhs, bool bExact) const { .... return Extent == rhs.Extent && Depth == rhs.Depth && bIsArray == rhs.bIsArray && ArraySize == rhs.ArraySize && NumMips == rhs.NumMips && NumSamples == rhs.NumSamples && Format == rhs.Format && LhsFlags == RhsFlags && TargetableFlags == rhs.TargetableFlags && bForceSeparateTargetAndShaderResource == rhs.bForceSeparateTargetAndShaderResource && ClearValue == rhs.ClearValue && AutoWritable == AutoWritable; // <= } 

警告PVS-Studio: V501 「==」演算子の左右に同じサブ式があります:AutoWritable == AutoWritable rendererinterface.h 180

Sambaプロジェクトコード(C):

 static int compare_procids(const void *p1, const void *p2) { const struct server_id *i1 = (struct server_id *)p1; const struct server_id *i2 = (struct server_id *)p2; if (i1->pid < i2->pid) return -1; if (i2->pid > i2->pid) return 1; return 0; } 

警告PVS-Studio: V501 「>」演算子の左右に同じ副次式があります:i2-> pid> i2-> pid brlock.c 1901

MongoDBプロジェクトコード(C ++):

 bool operator==(const MemberCfg& r) const { .... return _id==r._id && votes == r.votes && h == rh && priority == r.priority && arbiterOnly == r.arbiterOnly && slaveDelay == r.slaveDelay && hidden == r.hidden && buildIndexes == buildIndexes; // <= } 

警告PVS-Studio: V501 '=='演算子の左右に同じ副次式があります:buildIndexes == buildIndexes rs_config.h 101

Geant4ソフトウェアプロジェクトコード (C ++):

 inline G4bool G4FermiIntegerPartition:: operator==(const G4FermiIntegerPartition& right) { return (total == right.total && enableNull == enableNull && // <= partition == right.partition); } 

PVS-Studioの警告V501 「==」演算子の左右に同じ副次式があります。enableNull == enableNull G4hadronic_deex_fermi_breakup g4fermiintegerpartition.icc 58

LibreOfficeプロジェクトコード(C ++):

 class SvgGradientEntry { .... bool operator==(const SvgGradientEntry& rCompare) const { return (getOffset() == rCompare.getOffset() && getColor() == getColor() // <= && getOpacity() == getOpacity()); // <= } .... } 

PVS-Studio警告: V501 「==」演算子の左右に同じ副次式があります:getColor()== getColor()svggradientprimitive2d.hxx 61

Chromiumプロジェクトコード(C ++):

 bool FileIOTest::MatchesResult(const TestStep& a, const TestStep& b) { .... return (a.data_size == a.data_size && // <= std::equal(a.data, a.data + a.data_size, b.data)); } 

PVS-Studio警告: V501 「==」演算子の左右に同一のサブ式があります:a.data_size == a.data_size cdm_file_io_test.cc 367

FreeCADプロジェクトコード(C ++):

 bool FaceTypedBSpline::isEqual(const TopoDS_Face &faceOne, const TopoDS_Face &faceTwo) const { .... if (surfaceOne->IsURational() != surfaceTwo->IsURational()) return false; if (surfaceTwo->IsVRational() != // <= surfaceTwo->IsVRational()) // <= return false; if (surfaceOne->IsUPeriodic() != surfaceTwo->IsUPeriodic()) return false; if (surfaceOne->IsVPeriodic() != surfaceTwo->IsVPeriodic()) return false; if (surfaceOne->IsUClosed() != surfaceTwo->IsUClosed()) return false; if (surfaceOne->IsVClosed() != surfaceTwo->IsVClosed()) return false; if (surfaceOne->UDegree() != surfaceTwo->UDegree()) return false; if (surfaceOne->VDegree() != surfaceTwo->VDegree()) return false; .... } 

PVS-Studioの警告V501 「!=」演算子の左側と右側には、同一の部分式「surfaceTwo-> IsVRational()」があります。 modelrefine.cpp 780

深刻なエンジンプロジェクトコード(C ++):

 class CTexParams { public: inline BOOL IsEqual( CTexParams tp) { return tp_iFilter == tp.tp_iFilter && tp_iAnisotropy == tp_iAnisotropy && // <= tp_eWrapU == tp.tp_eWrapU && tp_eWrapV == tp.tp_eWrapV; }; .... }; 

警告PVS-Studio: V501 '=='演算子の左右に同じ副次式があります:tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180

Qtプロジェクトコード(C ++):

 inline bool qCompare(QImage const &t1, QImage const &t2, ....) { .... if (t1.width() != t2.width() || t2.height() != t2.height()) { .... } 

PVS-Studio警告: V501 '!='演算子の左右に同じ副次式があります:t2.height()!= T2.height()qtest_gui.h 101

FreeBSDプロジェクトコード(C):

 static int compare_sh(const void *_a, const void *_b) { const struct ipfw_sopt_handler *a, *b; a = (const struct ipfw_sopt_handler *)_a; b = (const struct ipfw_sopt_handler *)_b; .... if ((uintptr_t)a->handler < (uintptr_t)b->handler) return (-1); else if ((uintptr_t)b->handler > (uintptr_t)b->handler) // <= return (1); return (0); } 

PVS-Studioの警告V501 「>」演算子の左右には、同一の副次式「(uintptr_t)b->ハンドラー」があります。 ip_fw_sockopt.c 2893

モノプロジェクトコード(C#):

 static bool AreEqual (VisualStyleElement value1, VisualStyleElement value2) { return value1.ClassName == value1.ClassName && // <= value1.Part == value2.Part && value1.State == value2.State; } 

PVS-Studio警告: V3001 「==」演算子の左側と右側に同一のサブ式「value1.ClassName」があります。 ThemeVisualStyles.cs 2141

モノプロジェクトコード(C#):

 public int ExactInference (TypeSpec u, TypeSpec v) { .... var ac_u = (ArrayContainer) u; var ac_v = (ArrayContainer) v; .... var ga_u = u.TypeArguments; var ga_v = v.TypeArguments; .... if (u.TypeArguments.Length != u.TypeArguments.Length) // <= return 0; .... } 

PVS-Studio警告: V3001 「!=」演算子の左側と右側には、同一のサブ式「u.TypeArguments.Length」があります。 generic.cs 3135

MonoDevelopプロジェクトコード(C#):

 Accessibility DeclaredAccessibility { get; } bool IsStatic { get; } private bool MembersMatch(ISymbol member1, ISymbol member2) { if (member1.Kind != member2.Kind) { return false; } if (member1.DeclaredAccessibility != // <=1 member1.DeclaredAccessibility // <=1 || member1.IsStatic != member1.IsStatic) // <=2 { return false; } if (member1.ExplicitInterfaceImplementations().Any() || member2.ExplicitInterfaceImplementations().Any()) { return false; } return SignatureComparer .HaveSameSignatureAndConstraintsAndReturnTypeAndAccessors( member1, member2, this.IsCaseSensitive); } 

PVS-Studio警告: V3001 「!=」演算子の左側と右側には、同一の部分式「member1.IsStatic」があります。 CSharpBinding AbstractImplementInterfaceService.CodeAction.cs 545

Haikuプロジェクトコード(C ++):

 int __CORTEX_NAMESPACE__ compareTypeAndID(....) { int retValue = 0; .... if (lJack && rJack) { if (lJack->m_jackType < lJack->m_jackType) // <= { return -1; } if (lJack->m_jackType == lJack->m_jackType) // <= { if (lJack->m_index < rJack->m_index) { return -1; } else { return 1; } } else if (lJack->m_jackType > rJack->m_jackType) { retValue = 1; } } return retValue; } 

PVS-Studio警告: V501 「<」演算子の左右に同じ副次式があります:lJack-> m_jackType <lJack-> m_jackType MediaJack.cpp 783

すぐ下に、まったく同じエラーがもう1つあります。 私が理解するように、どちらの場合も、彼らはlJackrJackに置き換えるのを忘れていました

CryEngine Vプロジェクトコード(C ++):

 bool CompareRotation(const Quat& q1, const Quat& q2, float epsilon) { return (fabs_tpl(q1.vx - q2.vx) <= epsilon) && (fabs_tpl(q1.vy - q2.vy) <= epsilon) && (fabs_tpl(q2.vz - q2.vz) <= epsilon) // <= && (fabs_tpl(q1.w - q2.w) <= epsilon); } 

警告PVS-Studio: V501 「-」演算子の左右に同じ副次式があります:q2.vz-q2.vz entitynode.cpp 93

パターン:構造/クラスサイズの代わりにポインターサイズを計算する


このタイプのエラーは、CおよびC ++プログラムで発生し、 sizeof演算子の誤った使用に関連しています。 エラーはオブジェクトのサイズではなく、ポインターのサイズを計算しています。 例:

 T *a = foo1(); T *b = foo2(); x = memcmp(a, b, sizeof(a)); 

構造体Tのサイズの代わりに、ポインターのサイズが計算されます。 ポインターのサイズは使用されるデータモデルによって異なりますが、通常は4または8バイトです。 その結果、構造が占有するよりも多いまたは少ないバイトのメモリが比較されます。

正しいコードは次のとおりです。

 x = memcmp(a, b, sizeof(T)); 

または

 x = memcmp(a, b, sizeof(*a)); 

さあ、練習しましょう。 CryEngine V (C ++)プロジェクトのコードでこのようなエラーがどのように見えるかを次に示します。

 bool operator==(const SComputePipelineStateDescription& other) const { return 0 == memcmp(this, &other, sizeof(this)); } 

PVS-Studio警告: V579 memcmp関数は、ポインターとそのサイズを引数として受け取ります。 間違いかもしれません。 3番目の引数を調べます。 graphicspipelinestateset.h 58

アンリアルエンジン4 (C ++)プロジェクトコード:

 bool FRecastQueryFilter::IsEqual( const INavigationQueryFilterInterface* Other) const { // @NOTE: not type safe, should be changed when // another filter type is introduced return FMemory::Memcmp(this, Other, sizeof(this)) == 0; } 

PVS-Studio警告: V579 Memcmp関数は、ポインターとそのサイズを引数として受け取ります。 間違いかもしれません。 3番目の引数を調べます。 pimplrecastnavmesh.cpp 172

パターン:Cmp(A、A)の形式の繰り返し引数


多くの場合、比較関数は他の比較関数を呼び出します。 この場合、考えられるエラーの1つは、同じオブジェクトへのリンク/ポインターが2回送信されることです。 例:

 x = memcmp(A, A, sizeof(T)); 

ここでは、オブジェクトAがそれ自体と比較されますが、これはもちろん意味がありません。

GDB (C)デバッガーコードで見つかったエラーから始めます。

 static int psymbol_compare (const void *addr1, const void *addr2, int length) { struct partial_symbol *sym1 = (struct partial_symbol *) addr1; struct partial_symbol *sym2 = (struct partial_symbol *) addr2; return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value, // <= sizeof (sym1->ginfo.value)) == 0 && sym1->ginfo.language == sym2->ginfo.language && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2) && PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2) && sym1->ginfo.name == sym2->ginfo.name); } 

警告PVS-Studio: V549 'memcmp'関数の最初の引数は2番目の引数と同じです。 psymtab.c 1580

CryEngineSDKプロジェクトコード(C ++):

 inline bool operator != (const SEfResTexture &m) const { if (stricmp(m_Name.c_str(), m_Name.c_str()) != 0 || // <= m_TexFlags != m.m_TexFlags || m_bUTile != m.m_bUTile || m_bVTile != m.m_bVTile || m_Filter != m.m_Filter || m_Ext != m.m_Ext || m_Sampler != m.m_Sampler) return true; return false; } 

警告PVS-Studio: V549 'stricmp'関数の最初の引数は2番目の引数と同じです。 ishader.h 2089

PascalABC.NETプロジェクトコード(C#):

 private List<string> enum_consts = new List<string>(); public override bool IsEqual(SymScope ts) { EnumScope es = ts as EnumScope; if (es == null) return false; if (enum_consts.Count != es.enum_consts.Count) return false; for (int i = 0; i < es.enum_consts.Count; i++) if (string.Compare(enum_consts[i], this.enum_consts[i], true) != 0) return false; return true; } 

PVS-Studio警告: V3038 'enum_consts [i]'引数が 'Compare'メソッドに数回渡されました。 代わりに他の引数を渡す必要があります。 CodeCompletion SymTable.cs 2206

ここで少し説明します。 Compare関数の実際の引数のエラー:

 string.Compare(enum_consts[i], this.enum_consts[i], true) 

実際には、 enum_consts [i]this.enum_consts [i]まったく同じです。 私が理解しているように、正しい呼び出しは次のようになります。

 string.Compare(es.enum_consts[i], this.enum_consts[i], true) 

または

 string.Compare(enum_consts[i], es.enum_consts[i], true) 

パターン:チェックの繰り返しA == B && A == B


一般的なプログラミングエラーは、同じチェックが2回実行される場合です。 例:

 return A == B && C == D && // <= C == D && // <= E == F; 

このような場合、2つのオプションが可能です。 1つ目は無害です。1つの比較は不要であり、単純に削除できます。 2番目はさらに悪いことです。彼らは他の変数をチェックしたかったのですが、封印されていました。

いずれにせよ、そのようなコードには細心の注意が必要です。 もっとひどく怖がらせて、そのようなエラーはGCC (C)コンパイラコードでも見つけられることを示しましょう。

 static bool dw_val_equal_p (dw_val_node *a, dw_val_node *b) { .... case dw_val_class_vms_delta: return (!strcmp (a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1) && !strcmp (a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1)); .... } 

PVS-Studioの警告V501 「&&」演算子の左右に同一の副次式「!Strcmp(a-> v.val_vms_delta.lbl1、b-> v.val_vms_delta.lbl1)」があります。 dwarf2out.c 1428

strcmp関数は、同じ引数のセットで2回呼び出されます。

アンリアルエンジン4 (C ++)プロジェクトコード:

 FORCEINLINE bool operator==(const FShapedGlyphEntryKey& Other) const { return FontFace == Other.FontFace && GlyphIndex == Other.GlyphIndex // <= && FontSize == Other.FontSize && FontScale == Other.FontScale && GlyphIndex == Other.GlyphIndex; // <= } 

PVS-Studio警告: V501 「&&」演算子の左側と右側には、同一の副次式「GlyphIndex == Other.GlyphIndex」があります。 fontcache.h 139

深刻なエンジンプロジェクトコード(C ++):

 inline BOOL CValuesForPrimitive::operator==(....) { return ( (....) && (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) && .... (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) && .... ); 

PVS-Studioの警告V501 「&&」演算子の左側と右側に同一の副次式「(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType)」があります。 worldeditor.h 580

Oracle VM Virtual Box Project Code(C ++):

 typedef struct SCMDIFFSTATE { .... bool fIgnoreTrailingWhite; bool fIgnoreLeadingWhite; .... } SCMDIFFSTATE; /* Pointer to a diff state. */ typedef SCMDIFFSTATE *PSCMDIFFSTATE; /* Compare two lines */ DECLINLINE(bool) scmDiffCompare(PSCMDIFFSTATE pState, ....) { .... if (pState->fIgnoreTrailingWhite // <= || pState->fIgnoreTrailingWhite) // <= return scmDiffCompareSlow(....); .... } 

PVS-Studio警告: V501 「||」の左右に同一のサブ式「pState-> fIgnoreTrailingWhite」があります。 演算子。 scmdiff.cpp 238

パターン:memcmp関数によって返される値の誤用


memcmp関数は、 int型の次の値を返します。


注意してください。 「0より大きい」とは、任意の数値を意味し、1ではありません。これらの数値は、2、3、100、256、1024、5555、65536などです。 これは、この結果をchar型またはshort型の変数に配置できないことを意味します。 有意なビットがドロップされる可能性があり、プログラム実行ロジックに違反します。

このことから、結果を定数1または-1と比較できないことにもなります。 言い換えれば、そう間違っている:

 if (memcmp(a, b, sizeof(T)) == 1) if (memcmp(x, y, sizeof(T)) == -1) 

正しい比較:

 if (memcmp(a, b, sizeof(T)) > 0) if (memcmp(a, b, sizeof(T)) < 0) 

記載されているエラーの陰湿さは、コードが長時間正常に動作できることです。 新しいプラットフォームに切り替えたり、コンパイラのバージョンを変更したりすると、エラーが発生し始める可能性があります。

ReactOSプロジェクトコード(C ++):

 HRESULT WINAPI CRecycleBin::CompareIDs(....) { .... return MAKE_HRESULT(SEVERITY_SUCCESS, 0, (unsigned short)memcmp(pidl1->mkid.abID, pidl2->mkid.abID, pidl1->mkid.cb)); } 

PVS-Studio警告: V642 「memcmp」関数の結果を「unsigned short」型変数内に保存することは不適切です。 プログラムのロジックを壊して、重要なビットが失われる可能性があります。 recyclebin.cpp 542

Firebirdプロジェクトコード(C ++):

 SSHORT TextType::compare(ULONG len1, const UCHAR* str1, ULONG len2, const UCHAR* str2) { .... SSHORT cmp = memcmp(str1, str2, MIN(len1, len2)); if (cmp == 0) cmp = (len1 < len2 ? -1 : (len1 > len2 ? 1 : 0)); return cmp; } 

PVS-Studio警告: V642 「memcmp」関数の結果を「short」型変数内に保存することは不適切です。 プログラムのロジックを壊して、重要なビットが失われる可能性があります。 texttype.cpp 338

CoreCLRプロジェクトコード (C ++):

 bool operator( )(const GUID& _Key1, const GUID& _Key2) const { return memcmp(&_Key1, &_Key2, sizeof(GUID)) == -1; } 

PVS-Studioの警告V698式 'memcmp(....)== -1'は正しくありません。 この関数は、値「-1」だけでなく、負の値も返すことができます。 代わりに 'memcmp(....)<0'の使用を検討してください。 sos util.cpp 142

OpenToonzプロジェクトコード (C ++):

 bool TFilePath::operator<(const TFilePath &fp) const { .... char differ; differ = _wcsicmp(iName.c_str(), jName.c_str()); if (differ != 0) return differ < 0 ? true : false; .... } 

PVS-Studio警告: V642 「_wcsicmp」関数の結果を「char」型変数内に保存することは不適切です。 重要なビットが失われ、プログラムのロジックが破損する可能性があります。 tfilepath.cpp 328

パターン:無効なnull参照チェック


このエラーパターンは、C#プログラムの典型的なものです。 比較関数では、 as演算子を使用して型キャストが実行される場合があります。 エラーは、 nullへの不注意が新しいリンクではなく、元のリンクに対してチェックされるという事実にあります。 合成例を考えてみましょう:

 ChildT foo = obj as ChildT; if (obj == null) return false; if (foo.zzz()) {} 

obj変数にnull参照が含まれている場合、 (obj == null)が状況を保護するかどうかを確認します。 ただし、 as演算子がnull参照を返すことが判明した場合、ケースに対する保護はありません。 正しいコードは次のようになります。

 ChildT foo = obj as ChildT; if (foo == null) return false; if (foo.zzz()) {} 

原則として、プログラマーの不注意によりエラーが発生します。 CおよびC ++プログラムでも同様のエラーが発生する可能性がありますが、見つかったエラーのデータベースにはそのようなケースは見つかりませんでした。

MonoDevelopプロジェクトコード(C#):

 public override bool Equals (object o) { SolutionItemReference sr = o as SolutionItemReference; if (o == null) return false; return (path == sr.path) && (id == sr.id); } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「o」、「sr」を確認します。 MonoDevelop.Core SolutionItemReference.cs 81

CoreFXプロジェクトコード (C#):

 public override bool Equals(object comparand) { CredentialHostKey comparedCredentialKey = comparand as CredentialHostKey; if (comparand == null) { // This covers also the compared == null case return false; } bool equals = string.Equals(AuthenticationType, comparedCredentialKey.AuthenticationType, .... .... } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「comparand」、「comparedCredentialKey」を確認してください。 CredentialCache.cs 4007

ロズリンプロジェクトコード(C#):

 public override bool Equals(object obj) { var d = obj as DiagnosticDescription; if (obj == null) return false; if (!_code.Equals(d._code)) return false; .... } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「obj」、「d」を確認します。 DiagnosticDescription.cs 201

ロズリンプロジェクトコード(C#):

 protected override bool AreEqual(object other) { var otherResourceString = other as LocalizableResourceString; return other != null && _nameOfLocalizableResource == otherResourceString._nameOfLocalizableResource && _resourceManager == otherResourceString._resourceManager && _resourceSource == otherResourceString._resourceSource && .... } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「other」、「otherResourceString」を確認してください。 LocalizableResourceString.cs 121

MSBuildプロジェクトコード(C#):

 public override bool Equals(object obj) { AssemblyNameExtension name = obj as AssemblyNameExtension; if (obj == null) // <= { return false; } .... } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「obj」、「name」を確認してください。 AssemblyRemapping.cs 64

モノプロジェクトコード(C#):

 public override bool Equals (object o) { UrlMembershipCondition umc = (o as UrlMembershipCondition); if (o == null) // <= return false; .... return (String.Compare (u, 0, umc.Url, ....) == 0); // <= } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「o」、「umc」を確認してください。 UrlMembershipCondition.cs 111

Media Portal 2プロジェクトコード(C#):

 public override bool Equals(object obj) { EpisodeInfo other = obj as EpisodeInfo; if (obj == null) return false; if (TvdbId > 0 && other.TvdbId > 0) return TvdbId == other.TvdbId; .... } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数「obj」、「other」を確認してください。 EpisodeInfo.cs 560

NASA World Windプロジェクトコード(C#):

 public int CompareTo(object obj) { RenderableObject robj = obj as RenderableObject; if(obj == null) // <= return 1; return this.m_renderPriority.CompareTo(robj.RenderPriority); } 

PVS-Studio警告: V3019 「as」キーワードを使用した型変換後、おそらく誤った変数がnullと比較されます。 変数 'obj'、 'robj'を確認してください。 RenderableObject.cs 199

パターンタイプ:間違ったサイクル


一部の関数は、アイテムのコレクションを比較します。 当然、それらを比較するために、さまざまなサイクルオプションが使用されます。 比較関数で起こるように、コードを不注意に書くと、ループと何かを混同しやすくなります。 そのような状況をいくつか考えてみましょう。

トランスプロテオームパイプライン (C ++)プロジェクトコード:

 bool Peptide::operator==(Peptide& p) { .... for (i = 0, j = 0; i < this->stripped.length(), j < p.stripped.length(); i++, j++) { .... } 

PVS-Studio警告: V521 「、」演算子を使用したこのような式は危険です。 式が正しいことを確認してください。 tpplibペプチド.cpp 191

条件はコンマ演算子を使用することに注意してください。 カンマの左側の条件は考慮されないため、コードは明らかに正しくありません。 つまり、左側の条件が計算されますが、その結果はまったく使用されません。

Qtプロジェクトコード(C ++):

 bool equals( class1* val1, class2* val2 ) const { ... size_t size = val1->size(); ... while ( --size >= 0 ){ if ( !comp(*itr1,*itr2) ) return false; itr1++; itr2++; } ... } 

PVS-Studio 警告V547式 ' -size > = 0'は常にtrueです。符号なしの型の値は常に> = 0です。QtCLucene array.h 154 CLucene

プロジェクトコード(C ++):

 class Arrays { .... bool equals( class1* val1, class2* val2 ) const{ static _comparator comp; if ( val1 == val2 ) return true; size_t size = val1->size(); if ( size != val2->size() ) return false; _itr1 itr1 = val1->begin(); _itr2 itr2 = val2->begin(); while ( --size >= 0 ){ if ( !comp(*itr1,*itr2) ) return false; itr1++; itr2++; } return true; } .... } 

PVS-Studio 警告V547式 ' -size > = 0'は常にtrueです。符号なしの型の値は常に> = 0です。arrays.h154 Mono

プロジェクトコード(C#):

 public override bool Equals (object obj) { .... for (int i=0; i < list.Count; i++) { bool found = false; for (int j=0; i < ps.list.Count; j++) { // <= if (list [i].Equals (ps.list [j])) { found = true; break; } } if (!found) return false; } return true; } 

PVS-Studio警告:V3015「for」演算子内で間違った変数が比較されている可能性があります。「I」corlib-net_4_x見直しを検討 PermissionSet.cs 607

変数を使用するネストされたループの状態では、ほとんどのタイプミスをして、実際にJを代わりに、I

 for (int j=0; j < ps.list.Count; j++) 

パターン:A = getA()、B = GetA()


多くの場合、比較関数では、このタイプのコードを作成する必要があります。

 if (GetA().x == GetB().x && GetA().y == GetB().y) 

条件のサイズを小さくするため、または最適化するために、中間変数が使用されます。

 Type A = GetA(); Type B = GetB(); if (Ax == Bx && Ay == By) 

同時に、彼らは不注意で間違いを犯し、同じ値で一時変数を初期化します。

 Type A = GetA(); Type B = GetA(); 

実際のアプリケーションのコードでこのようなエラーがどのように見えるかを見てみましょう。LibreOffice

プロジェクトコード(C ++):

 bool CmpAttr( const SfxPoolItem& rItem1, const SfxPoolItem& rItem2) { .... bool bNumOffsetEqual = false; ::boost::optional<sal_uInt16> oNumOffset1 = static_cast<const SwFmtPageDesc&>(rItem1).GetNumOffset(); ::boost::optional<sal_uInt16> oNumOffset2 = static_cast<const SwFmtPageDesc&>(rItem1).GetNumOffset(); if (!oNumOffset1 && !oNumOffset2) { bNumOffsetEqual = true; } else if (oNumOffset1 && oNumOffset2) { bNumOffsetEqual = oNumOffset1.get() == oNumOffset2.get(); } else { bNumOffsetEqual = false; } .... } 

PVS-Studioの警告V656変数 'oNumOffset1'、 'oNumOffset2'は、同じ関数の呼び出しによって初期化されます。おそらくエラーまたは最適化されていないコードです。68、69行を確認します。findattr.cxx69 Qt

プロジェクトコード(C ++):

 AtomicComparator::ComparisonResult IntegerComparator::compare(const Item &o1, const AtomicComparator::Operator, const Item &o2) const { const Numeric *const num1 = o1.as<Numeric>(); const Numeric *const num2 = o1.as<Numeric>(); if(num1->isSigned() || num2->isSigned()) .... } 

PVS-Studio警告:V656変数 'num1'、 'num2'は、同じ関数の呼び出しによって初期化されます。おそらくエラーまたは最適化されていないコードです。「o1.as <Numeric>()」式の検査を検討してください。行を確認してください:220、221。qatomiccomparators.cpp 221

パターン:失敗したコードのコピー


前述のエラーの多くは、コピーペーストの失敗の結果と呼ぶことができます。それらはある種の誤ったパターンに該当し、それらを対応するセクションで説明することが論理的であると判断しました。ただし、コードのコピーが失敗したために明らかに発生したいくつかのエラーがありますが、分類方法はわかりません。そこで、これらのエラーをここでコンパイルしました。CoreCLR

プロジェクトコード(C ++):

 int __cdecl Compiler::RefCntCmp(const void* op1, const void* op2) { .... if (weight1) { .... if (varTypeIsGC(dsc1->TypeGet())) { weight1 += BB_UNITY_WEIGHT / 2; } if (dsc1->lvRegister) { weight1 += BB_UNITY_WEIGHT / 2; } } if (weight1) { .... if (varTypeIsGC(dsc2->TypeGet())) { weight1 += BB_UNITY_WEIGHT / 2; // <= } if (dsc2->lvRegister) { weight2 += BB_UNITY_WEIGHT / 2; } } .... } 

PVS-Studio警告:V778同様の2つのコードフラグメントが見つかりました。おそらく、これはタイプミスであり、「weight1」の代わりに「weight2」変数を使用する必要があります。clrjit lclvars.cpp 2702

この関数は長かったため、この記事では完全に短縮されています。私たちは、この関数のコードを考えた場合、コードの一部がコピーされたことに顕著であるが、一つの場所にプログラマは、変数置き換えるのを忘れてweight1WEIGHT2をMicrosoft

プロジェクト(C#)によるWPFサンプルコード

 public int Compare(GlyphRun a, GlyphRun b) { .... if (aPoint.Y > bPoint.Y) // <= { return -1; } else if (aPoint.Y > bPoint.Y) // <= { result = 1; } else if (aPoint.X < bPoint.X) { result = -1; } else if (aPoint.X > bPoint.X) { result = 1; } .... } 

PVS-Studio 警告V3003「if(A){...} else if(A){...}」パターンの使用が検出されました。論理エラーが存在する可能性があります。行を確認してください:418、422。txtserializerwriter.cs 418 PascalABC.NET

プロジェクトコード(C#):

 public void CompareInternal(....) { .... else if (left is int64_const) CompareInternal(left as int64_const, right as int64_const); .... else if (left is int64_const) CompareInternal(left as int64_const, right as int64_const); .... } 

PVS-Studio 警告V3003「if(A){...} else if(A){...}」パターンの使用が検出されました。論理エラーが存在する可能性があります。行を確認してください:597、631。ParserTools SyntaxTreeComparer.cs 597 SharpDevelop

プロジェクトコード(C#):

 public int Compare(SharpTreeNode x, SharpTreeNode y) { .... if (typeNameComparison == 0) { if (x.Text.ToString().Length < y.Text.ToString().Length) return -1; if (x.Text.ToString().Length < y.Text.ToString().Length) return 1; } .... } 

PVS-Studio警告:V3021同一の条件式を持つ2つの「if」ステートメントがあります。最初の「if」ステートメントにはメソッドの戻り値が含まれます。 これは、2番目の「if」ステートメントが無意味なNamespaceTreeNode.csであることを意味します87

Coin3Dプロジェクトコード(C ++):

 int SbProfilingData::operator == (const SbProfilingData & rhs) const { if (this->actionType != rhs.actionType) return FALSE; if (this->actionStartTime != rhs.actionStopTime) return FALSE; if (this->actionStartTime != rhs.actionStopTime) return FALSE; .... } 

PVS-Studio警告:V649同一の条件式を持つ2つの「if」ステートメントがあります。最初の「if」ステートメントには、関数の戻り値が含まれます。これは、2番目の「if」ステートメントが無意味であることを意味します。行を確認してください:1205、1206。sbprofilingdata.cpp 1206 Spring

プロジェクトコード(C ++):

 bool operator < (const aiFloatKey& o) const {return mTime < o.mTime;} bool operator > (const aiFloatKey& o) const {return mTime < o.mTime;} 

PVS-Studio警告:V524「>」関数の本体が「<」関数の本体と完全に同等であることは奇妙です。assimp 3dshelper.h 470

PVS-StudioコードアナライザーがMySQL(C ++)プロジェクトで見つけた最後の特に興味深いコードは次のとおりです。

 static int rr_cmp(uchar *a,uchar *b) { if (a[0] != b[0]) return (int) a[0] - (int) b[0]; if (a[1] != b[1]) return (int) a[1] - (int) b[1]; if (a[2] != b[2]) return (int) a[2] - (int) b[2]; if (a[3] != b[3]) return (int) a[3] - (int) b[3]; if (a[4] != b[4]) return (int) a[4] - (int) b[4]; if (a[5] != b[5]) return (int) a[1] - (int) b[5]; // <= if (a[6] != b[6]) return (int) a[6] - (int) b[6]; return (int) a[7] - (int) b[7]; } 

警告PVS-Studio:V525同様のブロックのコレクションを含むコード。行680、682、684、689、691、693、695の項目「0」、「1」、「2」、「3」、「4」、「1」、「6」を確認します。sqlrecords.cc 680

おそらく、プログラマーは最初の比較を書き、次に2番目の比較を書き、退屈になりました。したがって、彼はテキストブロックをクリップボードにコピーしました。

 if (a[1] != b[1]) return (int) a[1] - (int) b[1]; 

そして、必要な回数だけプログラムテキストに挿入しました。その後、彼はインデックスを変更しましたが、ある場所でミスをし、結果は間違った比較でした:

 if (a[5] != b[5]) return (int) a[1] - (int) b[5]; 

ご注意このエラーについては、ミニブック「プログラミング、リファクタリング、その他すべての主な問題」で詳しく説明しています(「コンパイラを想定しない」の章を参照)。

パターン:Equalsメソッドがnull参照を誤って処理します


C#では、Equalsメソッドを実装して、null参照が引数として来た場合に状況を正しく処理するのが慣習となっています。残念ながら、すべてのメソッドの実装がこの規則に準拠しているわけではありません。GitExtensions

プロジェクトコード(C#):

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

PVS-Studio 警告V3115 'null'を 'Equals(object obj)'メソッドに渡すと、 'NullReferenceException'になりません。Git.hub Organization.cs 14 PascalABC.NET

プロジェクトコード(C#):

 public override bool Equals(object obj) { var rhs = obj as ServiceReferenceMapFile; return FileName == rhs.FileName; } 

PVS-Studioの警告V3115 'null'を 'Equals'メソッドに渡すと、 'NullReferenceException'になりません。ICSharpCode.SharpDevelop ServiceReferenceMapFile.cs 31

その他のその他のエラー


G3Dコンテンツパック(C ++)プロジェクトコード

 bool Matrix4::operator==(const Matrix4& other) const { if (memcmp(this, &other, sizeof(Matrix4) == 0)) { return true; } ... } 

PVS-Studio 警告V575「memcmp」関数は「0」要素を処理します。「3番目」の引数を調べます。graphics3D matrix4.cpp 269

1つの閉じ括弧が適切ではありません。その結果、比較されるバイト数は式sizeof(Matrix4)== 0によって計算されますクラスのサイズは0より大きいため、式の結果は0です。したがって、0バイトが比較されます。

正しいオプション:

 if (memcmp(this, &other, sizeof(Matrix4)) == 0) { 

Wolfenstein 3Dプロジェクトコード(C ++):

 inline int operator!=( quat_t a, quat_t b ) { return ( ( ax != bx ) || ( ay != by ) || ( az != bz ) && ( aw != bw ) ); } 

PVS-Studio警告:「&&」操作のV648優先度が「||」の優先度よりも高い操作。math_quaternion.h 167

ある場所では、明らかに、||の代わりに誤って&&演算子を書いたようです。

FlightGearプロジェクトコード(C):

 static int tokMatch(struct Token* a, struct Token* b) { int i, l = a->strlen; if(!a || !b) return 0; .... } 

PVS-Studio警告:V595 nullptrに対して検証される前に、「a」ポインターが使用されました。行を確認してください:478、479。codegen.c 478

関数の最初の引数としてNULLを渡すと、関数は0を返すという考え方ですが、NULLポインターは逆参照されますWinMerge

プロジェクトコード(C ++):

 int TimeSizeCompare::CompareFiles(int compMethod, const DIFFITEM &di) { UINT code = DIFFCODE::SAME; ... if (di.left.size != di.right.size) { code &= ~DIFFCODE::SAME; code = DIFFCODE::DIFF; } ... } 

PVS-Studio警告:V519「コード」変数には連続して2回値が割り当てられます。おそらくこれは間違いです。行をチェック:79、80。マージtimesizecompare.cpp 80 ReactOS

プロジェクトコード(C ++):

 #define IsEqualGUID(rguid1, rguid2) \ (!memcmp(&(rguid1), &(rguid2), sizeof(GUID))) static int ctl2_find_guid(....) { MSFT_GuidEntry *guidentry; ... if (IsEqualGUID(guidentry, guid)) return offset; ... } 

PVS-Studio警告:V512「memcmp」関数を呼び出すと、バッファー「guidentry」のアンダーフローが発生します。oleaut32 typelib2.c 320

ポインターはマクロの最初の引数です。その結果、ポインタのアドレスが計算されますが、これは意味がありません。

正しいオプション:

 if (IsEqualGUID(*guidentry, guid)) return offset; 

IronPythonおよびIronRubyプロジェクトコード(C#):

 public static bool Equals(float x, float y) { if (x == y) { return !Single.IsNaN(x); } return x == y; } 

PVS-Studio警告:V3024奇妙な正確な比較:x == y。定義された精度の比較を使用することを検討してください:Math.Abs​​(A-B)<Epsilon。FloatOps.cs 1048 NaNの

特別なチェックのポイントは明確ではありません条件(x == y)が満たされる場合、これは、NaNがそれ自体を含む他の値と等しくないため、xyの両方NaN異なることを意味します。NaNのチェックは不要で、コードを次のように減らすことができます。

 public static bool Equals(float x, float y) { return x == y; } 

モノプロジェクトコード(C#):

 public bool Equals (CounterSample other) { return rawValue == other.rawValue && baseValue == other.counterFrequency && // <= counterFrequency == other.counterFrequency && // <= systemFrequency == other.systemFrequency && timeStamp == other.timeStamp && timeStamp100nSec == other.timeStamp100nSec && counterTimeStamp == other.counterTimeStamp && counterType == other.counterType; } 

PVS-Studio警告:V3112同様の比較での異常。式 'baseValue == other.counterFrequency'内にタイプミスが存在する可能性があります。System-net_4_x CounterSample.cs 139

これらのプログラムはどのように機能しますか?


これらすべてのエラーを見ると、これらのプログラムがすべて正常に機能することは驚くべきことです。確かに、比較関数はプログラムで非常に重要で責任のあるタスクを実行します。

このようなエラーのあるプログラムの健全性については、いくつかの説明があります。

  1. 多くの関数では、オブジェクトの一部のみが誤って比較されます。同時に、このプログラムのほとんどのタスクでは、部分的な比較で十分です。
  2. () , . , , , memcmp char . .
  3. .
  4. , ? - !


比較関数で検出できるエラーの数を示しました。そして、ユニットテストの助けを借りて、そのような機能のパフォーマンスをチェックする必要があります。

比較演算子、Equals関数などの単体テストを必ず作成してください

この記事を読む前に、多くの人はそのようなテストは冗長であり、それでもエラーを明らかにしないように思われました:結局のところ、比較関数は一見とても単純です...さて、今私はそれらに隠されているかもしれないすべての恐怖を示しました。

コードのレビューと静的解析ツールの使用も冗長ではありません。

おわりに


この記事では、有能な専門家によって開発された多くの有名なプロジェクトに言及しました。これらのプロジェクトは、さまざまな方法論を使用して厳密にテストされています。それでも、これはPVS-Studioアナライザーがエラーを検出することを妨げませんでした。これは、PVS-Studioがコードの品質と信頼性を向上させるために使用される他の方法論に大きく追加できることを示唆しています。

当社のウェブサイトにアクセスして、PVS-Studioをお試しください



この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください:Andrey Karpov。 比較関数内の悪

Source: https://habr.com/ru/post/J329090/


All Articles