PVS-Studio アナライザーを使用した rdesktop と xrdp の確認

これは、RDP プロトコルで動作するオープン ソース プログラムのテストに関する一連の記事の XNUMX 番目のレビューです。 その中で、rdesktop クライアントと xrdp サーバーを見ていきます。

エラーを特定するツールとして使用される PVSスタジオこれは、C、C++、C#、Java 用の静的コードアナライザーで、プラットフォームで利用可能です。 Windows, Linux и macOS.

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

注意。 FreeRDP プロジェクトの検証に関する以前の記事はこちらをご覧ください。 ここで.

rdesktop

rdesktop — UNIXベースのシステム用のRDPクライアントの無料実装。 WindowsCygwin環境でプロジェクトをビルドする場合。GPLv3ライセンス。

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

先ほども述べたように、このプロジェクトは非常に小規模です。 これには約 30 行のコードが含まれており、古いものを考えると少し奇妙です。 比較のために、FreeRDP には 320 万行が含まれています。 Cloc プログラムの出力は次のとおりです。

到達不能なコード

V６０ 使用できないコードが検出されました。 エラーが存在する可能性があります。 rデスクトップ.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 — このフラグメントはメモリ クリーニングを実行します。 ただし、このエラーは脅威ではありません。割り当てられたメモリはすべて、プログラムの終了後にオペレーティング システムによってクリアされます。

エラー処理なし

V６０ 配列アンダーランの可能性があります。 「n」インデックスの値は -1 に達する可能性があります。 rデスクトップ.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] = ' '; // <=
    str_handle_lines(output, &rest, linehandler, data);
  }
  ....
}

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

char型でのEOFの使用

V６０ 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 であり、次のような変数について話している場合、これは数値 -1 に対応します。 チャリオット。 シンボル0xFFは次のようになります。 EOF (-1) はファイルの終わりとして解釈されます。 このようなエラーを避けるために、関数の結果は次のようになります。 fgetc 次のような変数に格納する必要があります int型.

タイプミス

フラグメント 1

V６０ 式「write_time」は常に false です。 ディスク.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; // <=
  ....
}

おそらくこのコードの作成者は間違えたのではないでしょうか || и && 状態で。 可能な値のオプションを検討してみましょう 書き込み時間 и 変更時間:

• 両方の変数が 0 に等しい: この場合、最終的に分岐になります。 ほかに： 変数 mod_time その後の条件に関係なく、常に 0 になります。
• 変数の 0 つは XNUMX です。 mod_time は 0 になります (他の変数の値が負でない場合)。 MIN XNUMX つのオプションのうち小さい方を選択します。
• 両方の変数が 0 に等しくありません。最小値を選択してください。

条件を次のように置き換えると、 書き込み時間 && 変更時間 動作は正しく見えるでしょう:

• 0 つまたは両方の変数が XNUMX に等しくありません。ゼロ以外の値を選択してください。
• 両方の変数が 0 に等しくありません。最小値を選択してください。

フラグメント 2

V６０ 式は常に true です。 おそらくここでは「&&」演算子を使用する必要があります。 ディスク.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 を同時に持つことはできません。

無制限の行コピー

V６０ 「sprintf」関数を呼び出すと、バッファ「fullpath」のオーバーフローが発生します。 ディスク.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);
  ....
}

関数全体を見ると、このコードが問題を引き起こさないことが明らかになります。 ただし、将来的には発生する可能性があります。XNUMX つの不注意な変更でバッファ オーバーフローが発生する可能性があります。 スプリント は何にも制限されないため、パスを連結するときに配列の境界を越えることができます。 この呼び出しに注目することをお勧めします snprintf(フルパス, PATH_MAX, ….).

冗長条件

V６０ 条件式の一部は常に true です: add > 0。shard.c 507

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

Проверка > 0 を追加 ここではその必要はありません。変数は常にゼロより大きくなります。 %4 を読み取ります 除算の余りを返しますが、4 に等しくなることはありません。

xrdp

xrdp — オープンソース コードを使用した RDP サーバーの実装。 プロジェクトは 2 つの部分に分かれています。

• xrdp - プロトコルの実装。 Apache 2.0 ライセンスに基づいて配布されます。
• xorgxrdp - xrdp で使用する Xorg ドライバーのセット。 ライセンス - X11 (MIT と同様ですが、広告での使用を禁止します)

プロジェクトの開発は、rdesktop と FreeRDP の結果に基づいています。 当初、グラフィックスを操作するには、別の VNC サーバー、または RDP サポートを備えた特別な X11 サーバー (X11rdp) を使用する必要がありましたが、xorgxrdp の登場により、その必要はなくなりました。

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

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

タイプミスが増える

V６０ コードには、類似したブロックのコレクションが含まれています。 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、アナライザーは次のようにも教えてくれました。

V６０ コードには、類似したブロックのコレクションが含まれています。 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++;
}

配列宣言

V６０ 配列オーバーランの可能性があります。 「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];
    ....
  }
  ....
}

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

誤った比較

V６０ 条件式の一部は常に 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))
  {
    ....
  }
  ....
}

関数は型変数を読み取ります 未署名のショート のような変数に int型。 符号なし変数を読み取り、その結果をより大きな変数に代入しているため、変数は負の値を取ることができないため、ここではチェックは必要ありません。

不要なチェック

V６０ 条件式の一部は常に true になります: (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 の試用版を弊社からダウンロードできます。 オンライン.

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

出所： habr.com