PVS-StudioでTelegramを検証する、またはその逆

プロジェクトをチェックすることは興味深いです。特に自分で使用する場合は、既知のプロジェクトをチェックすることは二重に興味深いです。 高品質のコードを使用してプロジェクトを分析することはさらに興味深いでしょう。 その後、1石で2羽の鳥を殺すことができます-プロジェクト自体をチェックし、コードの品質を確認または反証し、アナライザーがどれだけうまくいったかを確認します。 少し考えた後、人気のある電報メッセンジャーはこれに最適であるという結論に達しました。

プロジェクトについて

Telegramは、国際市場に特化した無料のメッセンジャーであり、テキストメッセージとさまざまな形式のメディアファイルの両方を交換できます。 Android、iOS、Windows Phone、OS X、Windows、Linuxのバージョンがあります。



このプロジェクトの作者は、人気のVKontakteソーシャルネットワークに精通しているPavelとNikolai Durovsです。 アプリケーションでは、通信の安全性と保護の強化に特に重点が置かれます（その結果、秘密の自己削除チャットなどが作成される可能性があります）。 通信を暗号化するために、ニコライ・デュロフが開発したMTProtoテクノロジーが使用されます。



分析のために、デスクトップWindowsアプリケーションが選択されました。そのソースコードはGitHubの対応するリポジトリにあります 。



アプリケーションが多くのサードパーティライブラリを使用していることは注目に値するため、アプリケーションを自分で構築することに決めた場合は、いじくり回す必要があります。 一方、開発者は、サードパーティソフトウェアのアセンブリとインストールに関する優れたドキュメントを提供してくれたので、問題はないはずです。

タイトルについて

記事のタイトルについて質問があるかもしれません。 「どのように？」と尋ねます。 アナライザーを使用してプロジェクトのソースコードを検証する方法は明らかですが、逆チェックとはどういう意味ですか？



上記で書いたように、事前にコードから高品質を期待できます。 このプロジェクトは、アプリケーションセキュリティを優先する専門家によって処理されていると言っても、私はだましません。多くのエラーを見つけるのは奇妙です。 さらに、脆弱性を検索するためのコンペティションが定期的に開催されます。これにより、コードのレベルも維持されます。 そのため、プロジェクトをチェックすることは、アナライザーがタスクをどのように処理するかを確認する良い方法です。 しかし、それについては以下で詳しく説明します。

分析結果

プロジェクトをテストするために、 PVS-Studio静的コードアナライザーを使用しました。 1番目と2番目の重大度レベルの汎用警告（GA）と最適化（OP）が考慮されました。



原則として、Pavelがまだ管理職の地位にあったVkontakteネットワークの品質を知っているので、コードの品質を事前に評価できます。 ここですべてが同じくらい良いとすぐに言います。 2つの要因によって引き起こされるエラーはそれほど多くありませんでした。

• 比較的少数の分析されたファイル（159）。
• 高いコード品質スコア。

ですから、彼らは素晴らしい仕事をしていると言えます。 それにもかかわらず、アナライザーの助けを借りて、いくつかのかなり興味深い場所を見つけることができました。これについては後で検討します。



この記事では、すべてのアナライザー警告が選択されたわけではなく、いくつかの最も興味深い警告が選択されました。



一部の場所では、コードフラグメントにエラーがあるかどうか、それを修正する方法を明確に答えることが不可能です。これには、ソースコードのより詳細な調査が必要なためです。 ここで、コードを記述する開発者が静的アナライザーを直接使用する必要性を再び強調することに注意してください。



また、アナライザーによるソースコードのチェック手順にも注意してください。 .slnファイルがあるため、テストの開始は非常に簡単です。 すべてのサードパーティライブラリをアセンブルしてインストールしたら、「ソリューション」自体がエラーなしでビルドされたことを確認するだけで十分です。その後、マウスを数回クリックするだけでプロジェクトの分析を開始できます。 完了すると、検出されたエラーに関する受信レポートを確認するだけです。



ご注意 ソースコードが確認されてから、開発チームによっていくつかの更新がリリースされているため、コードの一部の断片が記事の断片と異なる場合があります。

見つかったバグと疑わしい場所

コードの次のセクションを見てみましょう。 個別に記述されているため、このコードフラグメントはエラーを検出するための問題を示しません。

void Window::placeSmallCounter(.... int size, int count, ....) { .... QString cnt = (count < 100) ? QString("%1").arg(count) : QString("..%1").arg(count % 10, 1, 10, QChar('0')); int32 cntSize = cnt.size(); .... int32 fontSize; if (size == 16) { fontSize = 8; } else if (size == 32) { fontSize = (cntSize < 2) ? 12 : 12; } else { fontSize = (cntSize < 2) ? 22 : 22; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V583 「？：」演算子は、その条件式に関係なく、常に1つの同じ値を返します：12. Telegram window.cpp 1607



ここでエラー（または-2）を見つけてください。エラーを含むコードが個別に記述されていれば、難しくありません。 三項演算子を使用すると、条件の論理結果に関係なく、変数 'fontSize'に同じ値が割り当てられます。 この例のように、三項演算子のそれぞれで数字「12」と「22」を繰り返す代わりに、それぞれの数字は繰り返しなしで数字「12」と「22」を持つ必要があります。



エラーが明らかに顕著であることに同意します。 どうしてそれが許されるのでしょうか？ 私たちはすべて人間であり、間違いを犯します。この場合、見つけやすい場合、このファイルの1700行以上で、このエラーは残りのコードに対して問題なく失われます。



多くの場合 、プロジェクトでは、ポインターが逆参照されるとエラーが発生し、その後でnullptrの同等性がチェックされます。 テレグラムも例外ではありませんでした。

 void DialogsWidget::dialogsReceived(....) { const QVector<MTPDialog> *dlgList = 0; .... unreadCountsReceived(*dlgList); .... if (dlgList) .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V595 nullptrに対して検証される前に、「dlgList」ポインターが使用されました。 行を確認してください：1620、1626。Telegram dialogswidget.cpp 1620



すでにこのコードスニペットから、ポインター「dlgList」のチェックは、参照解除後にのみ適用されることがわかります。 nullポインターの参照解除は未定義の動作です。つまり、プログラムが正常に動作したり、クラッシュしたり、すべてのパスワードを中国のハッカーに送信したりする可能性があります。 そのため、nullポインターのチェックは、参照解除する前に配置する必要がありました。



14個の類似のメッセージに出会いました。 いくつかの場所では、物事が良くなり、間違いはありません。 チェックは単純に繰り返されます（check-> dereferenceencing-> check、ポインタは変わりません）が、これに焦点を合わせることはしませんが、先に進みましょう。



次の不審なコードスニペット：

 bool psShowOpenWithMenu(....) { .... IEnumAssocHandlers *assocHandlers = 0; .... if (....) { .... IEnumAssocHandlers *assocHandlers = 0; .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V561新しい値を宣言するよりも、 'assocHandlers'変数に値を割り当てる方がおそらく良いでしょう。 以前の宣言：pspecific_wnd.cpp、2031行目。Telegrampspecific_wnd.cpp 2107



繰り返しますが、コードが書き出され、コードから余分なものがすべて削除されると、変数のオーバーライドが簡単にわかります。 モニター上で全体を見ることができない長さのメソッドでは、これはそれほど単純ではありません。



変数 'assocHandlers'が定義され、その後いくつかの操作が実行されますが、同じタイプと名前を持つ別の変数が以下で（そしてまったく同じ方法で）定義され、この変数は使用されなくなりました。 あなたは間違いがないと主張するかもしれません。 これまでのところ、レーキはすでに配置されており、踏まれる瞬間を待っています。 将来、コードを操作する人はこの再定義に気付かない可能性があり、その後エラーが現れます。 しかし、それが複数回言及されているように、エラーが早く修正されるほど良いので、そのような場所を避ける必要があります。



別の同様のコードを見つけました。 対応する診断メッセージ：



V561新たに宣言するよりも、 'ms'変数に値を割り当てる方がおそらく良いでしょう。 前の宣言：window.cpp、行1371。Telegramwindow.cpp 1467



どうぞ

 void HistoryImageLink::getState(.... const HistoryItem *parent, ....) const { .... int skipx = 0, skipy = 0, height = _height; const HistoryReply *reply = toHistoryReply(parent); const HistoryForwarded *fwd = reply ? 0 : toHistoryForwarded(parent); .... if (reply) { skipy = st::msgReplyPadding.top() + st::msgReplyBarSize.height() + st::msgReplyPadding.bottom(); } if (fwd) { skipy = st::msgServiceNameFont->height + st::msgPadding.top(); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V646アプリケーションのロジックの検査を検討してください。 「else」キーワードが欠落している可能性があります。 Telegram history.cpp 5181



アナライザーの警告によると、キーワード「else」は新しい条件ではなく、想定されていた可能性があることは明らかです。 このコードを正しく修正する方法を言うのは難しいです。 おそらく編集する価値はありません。



これらは、 'skipy'変数が何らかの値で初期化される唯一の2つのブランチです。 フラグメントから、最初は0に初期化され、その後（ソースコードは多数あるため、ここでは示していません）、この変数がインクリメントされていることがわかります。



このことから、おそらく、2番目の条件「if」は不要であるか、さらには誤っている（両方の条件が真である場合）と結論付けることができます。 「else-if」コンストラクトが想定されていた可能性があります（フォーマットによって判断）、側面から見て明確に言うのは難しいです。 ただし、この場所は潜在的に誤っている可能性があります。



次の不審なコード：

 void DialogsListWidget::addDialog(const MTPDdialog &dialog) { History *history = App::history(App::peerFromMTP(dialog.vpeer), dialog.vunread_count.v, dialog.vread_inbox_max_id.v); .... SavedPeersByTime &saved(cRefSavedPeersByTime()); while (!saved.isEmpty() && history->lastMsg->date < saved.lastKey()) { History *history = App::history(saved.last()->id); .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V711このループを制御する変数と同じ名前のループ内にローカル変数を作成することは危険です。 電報ダイアログswidget.cpp 949



問題の本質は、アナライザーのメッセージから明らかです。 ループの本体では、ループの制御に使用される変数と一致する変数が宣言されます。 この状況はどうして危険なのでしょうか？ ループの本体の変数を変更しても、（別の変数が変更されているため）終了条件に影響しません。これは、ループを終了する条件の一部が間違っていることが判明する可能性があるためです（たとえば、無限ループが発生する可能性があります）。



これが間違いではない場合、少なくとも葉に隠れている熊手。



別の問題のある場所を検討してください。

 bool update() { .... wstring fname = from[i], tofname = to[i]; .... WCHAR errMsg[2048]; .... wsprintf(errMsg, L"Failed to update Telegram :(\n%s is not accessible.", tofname); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V510 「wsprintfW」関数は、3番目の実引数としてクラス型変数を受け取ることを期待されていません。 アップデータupdater.cpp 255



問題は、wstring型のオブジェクトである関数の3番目の引数にあります。 wsprintf関数の仮パラメーターのリストは省略記号で終わるため、任意の型の引数を渡すことができますが、これには既に特定の危険が伴います。 省略記号の実際の引数は、PODタイプのみである必要があります。 フォーマット文字列から、 'wchar_t *'型の引数が予期されていることがわかりますが、代わりにオブジェクトを渡すため、バッファ内でナンセンスになったり、プログラムがクラッシュしたりする可能性があります。



条件式に追加の部分式を使用してコードの一部を満たしました。

 QImage imageBlur(QImage img) { .... const int radius = 3; .... if (radius < 16 && ....) .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V560条件式の一部は常に真です：半径<16。Telegram images.cpp 241



警告の本質は非常に明確です-変数が宣言され、すぐに初期化され（定数以外）、その状態でその値が数値リテラルと比較されます。 定数も数値リテラル（自然）も変わらないため、条件は常にtrueまたはfalse（この場合は常に 'true'）になります。



変数に値が2回割り当てられたときにコードがありましたが、これらの割り当ての間では使用されませんでした。 別の変数が暗示されている場合、これは間違いの可能性があります。 この場合、このような危険はありません（少なくとも目に見えません）が、これが役に立たないことは明らかです。

 bool eBidiItemize(....) { .... dir = QChar::DirON; status.eor = QChar::DirEN; dir = QChar::DirAN; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告： V519 「dir」変数には連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：2084、2085。Telegram text.cpp 2085



後で使用されない変数が宣言されている場所は、奇妙に見えます。 コードの周りに散らばっている未使用の変数には良いものがないことは明らかです。 同様のコードの例：

 void extractMetaData(AVDictionary *dict) { .... for (....) { .... QString tmp = QString::fromUtf8(value); } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーの警告：「QString」タイプのV808 「tmp」オブジェクトは作成されましたが、使用されませんでした。 Telegram audio.cpp 2296



コードスニペットからわかるように、変数「tmp」が宣言されており、どこでも使用されていません。 それを初期化するためにメソッド呼び出しが使用されます;さらに、これらすべてはループの本体で発生し、状況をさらに悪化させます。



これはこのタイプの唯一の警告ではなく、さらに16がありました。

おわりに

Telegramチェックは非常に興味深いことが判明し、「i」にいくつかの点を付けました。



まず、私はこのプロジェクトを長い間チェックしたかったのですが、最終的には可能になりました。 直接検証の前にサードパーティ製ソフトウェアのインストールをいじらなければならなかったという事実にもかかわらず、インストールとアセンブリのわかりやすい指示があるため、これは問題を引き起こしませんでした。



第二に、プロジェクトのコードは高品質であることが判明しました。 このメッセンジャーは、通信の機密性を優先し、その中に多くのエラーを見つけることは奇妙です。



第三に、コードは仕事を知っている専門家によって作成され、検索コンテストが開催されているという事実にもかかわらず、PVS-Studioはまだいくつかの奇妙な場所を見つけることができました（すべてではなく、最も興味深い場所が記事に書かれていることを思い出したい）脆弱性。 これは、アナライザーの品質と、このようなツールを使用するプログラマーの必要性を強調しています。





この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：セルゲイヴァシリエフ。 PVS-StudioおよびVice Versaによる電報の分析 。