PVS-Studioアナライザーを使用したrdesktopおよびxrdpの検証

これは、RDPプロトコルを使用するためのオープンソースプログラムのチェックに関する一連の記事の2番目のレビューです。 その中で、rdesktopクライアントとxrdpサーバーを検討します。



PVS-Studioは 、エラーを検出するためのツールとして使用されました。 C、C ++、C＃、およびJava用の静的コードアナライザーであり、Windows、Linux、およびmacOSプラットフォームで使用できます。



この記事では、私にとって興味深いと思われるエラーのみを紹介しています。 ただし、プロジェクトは小さいため、エラーはほとんどありませんでした:)。



ご注意 FreeRDPプロジェクトのチェックに関する以前の記事は、 こちらにあります 。

rdesktop

rdesktopは、UNIXベースのシステム用のRDPクライアントの無料実装です。 Cygwinでプロジェクトをビルドする場合、Windowsでも使用できます。 GPLv3でライセンスされています。



このクライアントは非常に人気があります-ReactOSでデフォルトで使用されますが、サードパーティのグラフィカルフロントエンドも見つけることができます。 それにもかかわらず、彼は非常に古いです。最初のリリースは2001年4月4日に行われました-執筆時点で、彼の年齢は17歳です。



前述したように、プロジェクトは非常に小さいです。 約3万行のコードが含まれていますが、その年齢を考えると少し奇妙です。 比較のため、FreeRDPには32万行が含まれています。 Clocプログラムからの出力は次のとおりです。

到達不能コード

V779到達不能コードが検出されました。 エラーが存在する可能性があります。 rdesktop.c 1502

int main(int argc, char *argv[]) { .... return handle_disconnect_reason(deactivated, ext_disc_reason); if (g_redirect_username) xfree(g_redirect_username); xfree(g_username); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このエラーはメイン関数ですぐに見つかります。return ステートメントの後に来るコードが表示されます-このフラグメントはメモリを消去します。 それにもかかわらず、エラーは脅威になりません。割り当てられたすべてのメモリは、プログラムの終了後にオペレーティングシステムによってクリーンアップされます。

エラー処理の欠如

V557アレイアンダーランが可能です。 'n'インデックスの値は-1に達する可能性があります。 rdesktop.c 1872

 RD_BOOL subprocess(char *const argv[], str_handle_lines_t linehandler, void *data) { int n = 1; char output[256]; .... while (n > 0) { n = read(fd[0], output, 255); output[n] = '\0'; // <= str_handle_lines(output, &rest, linehandler, data); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この場合のコードスニペットは、ファイルが終了するまでファイルからバッファーに読み取ります。 ただし、エラー処理はありません。何か問題が発生した場合、 readは-1を返し、出力配列は配列の境界を超えます。

charでEOFを使用する

V739 EOFは、「char」タイプの値と比較しないでください。 '（c = fgetc（fp））'は 'int'型でなければなりません。 ctrl.c 500

 int ctrl_send_command(const char *cmd, const char *arg) { char result[CTRL_RESULT_SIZE], c, *escaped; .... while ((c = fgetc(fp)) != EOF && index < CTRL_RESULT_SIZE && c != '\n') { result[index] = c; index++; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここで、ファイルの最後に到達する処理が正しくないことがわかります。fgetcがコードが0xFFの文字を返す場合、ファイルの終わり（ EOF ）として解釈されます。



EOFは定数であり、通常は-1として定義されます。 たとえば、CP1251のコーディングでは、ロシア語のアルファベットの最後の文字にコード0xFFがあります。これは、 char型の変数について話している場合、数値-1に対応しています。 EOF （-1）のような文字0xFFは、ファイルの終わりとして認識されることがわかります。 このようなエラーを回避するには、 fgetc関数の結果をint変数に保存する必要があります。

タイプミス

フラグメント1



V547式 'write_time'は常にfalseです。 disk.c 805

 RD_NTSTATUS disk_set_information(....) { time_t write_time, change_time, access_time, mod_time; .... if (write_time || change_time) mod_time = MIN(write_time, change_time); else mod_time = write_time ? write_time : change_time; // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

おそらくこのコードの作者は混乱している|| および&&が提供されます。 write_timeおよびchange_timeの可能なオプションを検討してください 。

• 両方の変数は0です。この場合、 elseブランチに行きます。mod_time変数は 、次の条件に関係なく常に0です。
• MINは2つのオプションのうち最小のものを選択するため、変数の1つは0です。mod_timeは0です（他の変数の値が負でない場合）。
• 両方の変数が0に等しくありません：最小値を選択します。

条件をwrite_time && change_timeに置き換えると、動作は正しくなります。

• 1つまたは両方の変数が0と等しくない：0以外の値を選択します。
• 両方の変数が0に等しくありません：最小値を選択します。

フラグメント2



V547式は常に真です。 ここでは、おそらく「&&」演算子を使用する必要があります。 disk.c 1419

 static RD_NTSTATUS disk_device_control(RD_NTHANDLE handle, uint32 request, STREAM in, STREAM out) { .... if (((request >> 16) != 20) || ((request >> 16) != 9)) return RD_STATUS_INVALID_PARAMETER; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

どうやら、演算子|| および&& 、 ==と！=のいずれか：変数は同時に値20と9を取ることはできません。

無制限の行コピー

V512 「sprintf」関数を呼び出すと、バッファー「fullpath」がオーバーフローします。 disk.c 1257

 RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Get information for directory entry */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

機能を検討すると、このコードが問題を引き起こさないことが完全に明らかになります。 ただし、それらは将来発生する可能性があります。不注意な変更が1つあり、バッファオーバーフローが発生します。sprintfは何によっても制限されないため、パスを連結するときに配列の境界を超えることができます。 snprintf（fullpath、PATH_MAX、....）でこの呼び出しに注意することをお勧めします 。

冗長条件

V560条件式の一部は常に真です：add>0。scard.c 507

 static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

add> 0をチェックしても意味がありません。read ％4は除算の残りを返すため、変数は常にゼロより大きくなり、 4になることはありません。

xrdp

xrdpは、オープンソースのRDPサーバー実装です。 プロジェクトは2つの部分に分かれています。

• xrdp-プロトコル実装。 Apache 2.0ライセンスで配布されます。
• xorgxrdp-xrdpで使用するXorgドライバーのセット。 ライセンス-X11（MITとして、ただし広告での使用は禁止）

プロジェクト開発は、rdesktopとFreeRDPの結果に基づいています。 最初は、グラフィックスを処理するために別のVNCサーバー、またはRDPをサポートする特別なX11サーバー（X11rdp）を使用する必要がありましたが、xorgxrdpの出現により、それらは不要になりました。



この記事では、xorgxrdpについては触れません。



xrdpプロジェクトは、前のプロジェクトと同様に非常に小さく、約8万行が含まれています。

もっとタイプミス

V525コードには、同様のブロックのコレクションが含まれています。 87行目、88行目、89行目の項目「r」、「g」、「r」を確認します。rfxencode_rgb_to_yuv.c 87

 static int rfx_encode_format_rgb(const char *rgb_data, int width, int height, int stride_bytes, int pixel_format, uint8 *r_buf, uint8 *g_buf, uint8 *b_buf) { .... switch (pixel_format) { case RFX_FORMAT_BGRA: .... while (x < 64) { *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; // <= x++; } .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードは、RemoteFXが機能するためにjpeg2000コーデックを実装するlibrfxcodecライブラリから取得されました。 ここでは、グラフィックデータチャネルが混乱しているようです。「青」の代わりに「赤」が書き込まれています。 このようなエラーは、コピーアンドペーストの結果として発生した可能性が最も高いです。



同じ問題は、同様の関数rfx_encode_format_argbに該当し、アナライザーからも次のことがわかりました。



V525コードには、同様のブロックのコレクションが含まれています。 260、261、262、263行目の項目「a」、「r」、「g」、「r」を確認します。rfxencode_rgb_to_yuv.c 260

 while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

配列宣言

V557配列のオーバーランが可能です。 「i-8」インデックスの値は129に達する可能性があります。genkeymap.c 142

 // evdev-map.c int xfree86_to_evdev[137-8+1] = { .... }; // genkeymap.c extern int xfree86_to_evdev[137-8]; int main(int argc, char **argv) { .... for (i = 8; i <= 137; i++) /* Keycodes */ { if (is_evdev) e.keycode = xfree86_to_evdev[i-8]; .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

これら2つのファイルの配列の宣言と定義には互換性がありません-サイズは1ずつ異なります。ただし、エラーは発生しません-evdev-map.cファイルに正しいサイズが示されているため、範囲外になりません。 したがって、これは簡単に修正できる欠陥です。

無効な比較

V560条件式の一部は常にfalseです：（cap_len <0）。 xrdp_caps.c 616

 // common/parse.h #if defined(B_ENDIAN) || defined(NEED_ALIGN) #define in_uint16_le(s, v) do \ .... #else #define in_uint16_le(s, v) do \ { \ (v) = *((unsigned short*)((s)->p)); \ (s)->p += 2; \ } while (0) #endif int xrdp_caps_process_confirm_active(struct xrdp_rdp *self, struct stream *s) { int cap_len; .... in_uint16_le(s, cap_len); .... if ((cap_len < 0) || (cap_len > 1024 * 1024)) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

関数では、 unsigned short型の変数はint型の変数に読み込まれます。 ここでは、符号なしの型の変数を読み取り、結果をより大きな変数に割り当てるため、チェックは必要ありません。そのため、変数は負の値を取ることができません。

不要なチェック

V560条件式の一部は常に真です：（bpp！= 16）。 libxrdp.c 704

 int EXPORT_CC libxrdp_send_pointer(struct xrdp_session *session, int cache_idx, char *data, char *mask, int x, int y, int bpp) { .... if ((bpp == 15) && (bpp != 16) && (bpp != 24) && (bpp != 32)) { g_writeln("libxrdp_send_pointer: error"); return 1; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

不平等のチェックは、ここでは意味がありません。最初に比較がすでに行われているためです。 これはタイプミスであり、開発者は||を使用したかった可能性があります。 無効な引数を除外します。

おわりに

監査中に重大なエラーは検出されませんでしたが、多くの欠点が見つかりました。 ただし、これらのプロジェクトは、範囲は狭いものの、多くのシステムで使用されています。 小さなプロジェクトでは多くの間違いを犯す必要はありません。そのため、小さなプロジェクトでのみアナライザーの動作を判断しないでください。 これについての詳細は、記事「 数字で確認される感覚 」を参照してください。



PVS-Studioの試用版は、当社のWebサイトからダウンロードできます。

この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Sergey Larin。 PVS-Studioでrdesktopとxrdpを確認する