กำลังตรวจสอบ rdesktop และ xrdp โดยใช้เครื่องวิเคราะห์ PVS-Studio

นี่เป็นการทบทวนครั้งที่สองในชุดบทความเกี่ยวกับการทดสอบโปรแกรมโอเพ่นซอร์สสำหรับการทำงานกับโปรโตคอล RDP ในนั้นเราจะดูที่ไคลเอนต์ rdesktop และเซิร์ฟเวอร์ xrdp

ใช้เป็นเครื่องมือในการระบุข้อผิดพลาด พีวีเอส-สตูดิโอ. เป็นตัววิเคราะห์โค้ดแบบคงที่สำหรับภาษา C, C++, C# และ Java ซึ่งพร้อมใช้งานบนแพลตฟอร์ม Windows, Linux และ macOS

บทความนี้นำเสนอเฉพาะข้อผิดพลาดที่ดูน่าสนใจสำหรับฉันเท่านั้น อย่างไรก็ตาม โครงการมีขนาดเล็ก จึงมีข้อผิดพลาดเล็กน้อย :)

หมายเหตุ. สามารถดูบทความก่อนหน้าเกี่ยวกับการตรวจสอบโครงการ FreeRDP ได้ ที่นี่.

เดสก์ท็อป

เดสก์ท็อป — การใช้งานไคลเอนต์ RDP สำหรับระบบที่ใช้ UNIX ฟรี นอกจากนี้ยังสามารถใช้งานได้ภายใต้ Windows หากคุณสร้างโปรเจ็กต์ภายใต้ Cygwin ได้รับอนุญาตภายใต้ GPLv3

ไคลเอนต์นี้ได้รับความนิยมอย่างมาก - มันถูกใช้เป็นค่าเริ่มต้นใน ReactOS และคุณยังสามารถค้นหาส่วนหน้าแบบกราฟิกของบุคคลที่สามได้อีกด้วย อย่างไรก็ตามเขาอายุค่อนข้างมาก: การเปิดตัวครั้งแรกเกิดขึ้นเมื่อวันที่ 4 เมษายน พ.ศ. 2001 ในขณะที่เขียนเขาอายุ 17 ปี

อย่างที่บอกไปแล้วว่าโปรเจ็กต์นี้ค่อนข้างเล็ก มีโค้ดประมาณ 30 บรรทัด ซึ่งค่อนข้างแปลกเมื่อพิจารณาจากอายุของมัน สำหรับการเปรียบเทียบ FreeRDP มี 320 บรรทัด นี่คือผลลัพธ์ของโปรแกรม 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);
}

ข้อผิดพลาดพบเราทันทีในฟังก์ชัน หลัก: เราเห็นโค้ดมาตามหลังโอเปอเรเตอร์ กลับ — ส่วนนี้ดำเนินการทำความสะอาดหน่วยความจำ อย่างไรก็ตามข้อผิดพลาดไม่ก่อให้เกิดภัยคุกคาม: หน่วยความจำที่จัดสรรทั้งหมดจะถูกล้างโดยระบบปฏิบัติการหลังจากออกจากโปรแกรม

ไม่มีการจัดการข้อผิดพลาด

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] = ' '; // <=
    str_handle_lines(output, &rest, linehandler, data);
  }
  ....
}

ข้อมูลโค้ดในกรณีนี้จะอ่านจากไฟล์ไปยังบัฟเฟอร์จนกว่าไฟล์จะสิ้นสุด อย่างไรก็ตาม ไม่มีการจัดการข้อผิดพลาดที่นี่ หากมีสิ่งผิดปกติเกิดขึ้น อ่าน จะส่งกลับ -1 จากนั้นอาร์เรย์จะถูกโอเวอร์รัน เอาท์พุต.

การใช้ 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 ซึ่งสอดคล้องกับตัวเลข -1 หากเรากำลังพูดถึงตัวแปรเช่น ถัง. ปรากฎว่าสัญลักษณ์ 0xFF เช่น EOF (-1) ถูกตีความเป็นจุดสิ้นสุดของไฟล์ เพื่อหลีกเลี่ยงข้อผิดพลาดดังกล่าว ผลลัพธ์ของฟังก์ชันคือ fgetc ควรเก็บไว้ในตัวแปรเช่น int.

ความผิดพลาด

ส่วนที่ 1

V547 นิพจน์ 'write_time' จะเป็นเท็จเสมอ ดิสก์.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 и เปลี่ยน_เวลา:

• ตัวแปรทั้งสองมีค่าเท่ากับ 0: ในกรณีนี้เราจะไปสิ้นสุดที่สาขา อื่น: ตัวแปร mod_time จะเป็น 0 เสมอโดยไม่คำนึงถึงเงื่อนไขที่ตามมา
• หนึ่งในตัวแปรคือ 0: mod_time จะเท่ากับ 0 (โดยมีเงื่อนไขว่าตัวแปรอื่นมีค่าไม่เป็นลบ) เพราะ นาที จะเลือกตัวเลือกที่เล็กกว่าจากทั้งสองตัวเลือก
• ตัวแปรทั้งสองไม่เท่ากับ 0: เลือกค่าต่ำสุด

เมื่อเปลี่ยนเงื่อนไขด้วย write_time && change_time พฤติกรรมจะดูถูกต้อง:

• ตัวแปรหนึ่งหรือทั้งสองไม่เท่ากับ 0: เลือกค่าที่ไม่ใช่ศูนย์
• ตัวแปรทั้งสองไม่เท่ากับ 0: เลือกค่าต่ำสุด

ส่วนที่ 2

V547 การแสดงออกเป็นจริงเสมอ อาจควรใช้ตัวดำเนินการ '&&' ที่นี่ ดิสก์.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' ดิสก์.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);
  ....
}

เมื่อดูฟังก์ชันทั้งหมดจะพบว่าโค้ดนี้ไม่ทำให้เกิดปัญหาแต่อย่างใด อย่างไรก็ตาม สิ่งเหล่านี้อาจเกิดขึ้นได้ในอนาคต: การเปลี่ยนแปลงที่ไม่ระมัดระวังเพียงครั้งเดียวและเราจะได้รับบัฟเฟอร์ล้น - วิ่ง ไม่ถูกจำกัดด้วยสิ่งใดๆ ดังนั้นเมื่อเชื่อมเส้นทางเข้าด้วยกัน เราสามารถก้าวข้ามขอบเขตของอาร์เรย์ได้ ขอแนะนำให้สังเกตการโทรนี้ snprintf (เส้นทางแบบเต็ม, PATH_MAX, ….).

สภาพที่ซ้ำซ้อน

V560 ส่วนหนึ่งของนิพจน์เงื่อนไขเป็นจริงเสมอ: เพิ่ม > 0. scard.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 - ชุดไดรเวอร์ Xorg สำหรับใช้กับ xrdp ใบอนุญาต - X11 (เช่น MIT แต่ห้ามใช้ในการโฆษณา)

การพัฒนาโครงการขึ้นอยู่กับผลลัพธ์ของ rdesktop และ FreeRDP เริ่มแรกในการทำงานกับกราฟิกคุณต้องใช้เซิร์ฟเวอร์ VNC แยกต่างหากหรือเซิร์ฟเวอร์ X11 พิเศษที่รองรับ RDP - X11rdp แต่ด้วยการถือกำเนิดของ xorgxrdp ความต้องการจึงหายไป

ในบทความนี้ เราจะไม่กล่าวถึง xorgxrdp

โปรเจ็กต์ xrdp เช่นเดียวกับโปรเจ็กต์ก่อนหน้า มีขนาดเล็กมากและมีประมาณ 80 บรรทัด

พิมพ์ผิดมากขึ้น

V525 รหัสประกอบด้วยชุดของบล็อกที่คล้ายกัน ตรวจสอบรายการ 'r', 'g', 'r' ในบรรทัด 87, 88, 89 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++;
      }
      ....
  }
  ....
}

รหัสนี้นำมาจากไลบรารี librfxcodec ซึ่งใช้ตัวแปลงสัญญาณ jpeg2000 สำหรับ RemoteFX เห็นได้ชัดว่าช่องข้อมูลกราฟิกผสมกัน - แทนที่จะเป็นสี "สีน้ำเงิน" บันทึก "สีแดง" ข้อผิดพลาดนี้น่าจะเกิดจากการคัดลอกและวาง

ปัญหาเดียวกันเกิดขึ้นในฟังก์ชันที่คล้ายกัน rfx_encode_format_argbซึ่งผู้วิเคราะห์บอกเราด้วยว่า:

V525 รหัสประกอบด้วยชุดของบล็อกที่คล้ายกัน ตรวจสอบรายการ 'a', 'r', 'g', 'r' ในบรรทัด 260, 261, 262, 263 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];
    ....
  }
  ....
}

การประกาศและคำจำกัดความของอาร์เรย์ในไฟล์ทั้งสองนี้เข้ากันไม่ได้ - ขนาดแตกต่างกัน 1 อย่างไรก็ตาม ไม่มีข้อผิดพลาดเกิดขึ้น - ขนาดที่ถูกต้องระบุไว้ในไฟล์ evdev-map.c ดังนั้นจึงไม่มีขอบเขต นี่เป็นเพียงข้อบกพร่องที่สามารถแก้ไขได้ง่าย

การเปรียบเทียบที่ไม่ถูกต้อง

V560 ส่วนหนึ่งของนิพจน์เงื่อนไขจะเป็นเท็จเสมอ: (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. ไม่จำเป็นต้องตรวจสอบที่นี่เนื่องจากเรากำลังอ่านตัวแปรที่ไม่ได้ลงชื่อและกำหนดผลลัพธ์ให้กับตัวแปรที่ใหญ่กว่า ดังนั้นตัวแปรจึงไม่สามารถรับค่าลบได้

การตรวจสอบที่ไม่จำเป็น

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 เวอร์ชันทดลองได้จากเรา เว็บไซต์.

หากคุณต้องการแบ่งปันบทความนี้กับผู้ชมที่พูดภาษาอังกฤษ โปรดใช้ลิงก์การแปล: Sergey Larin กำลังตรวจสอบ rdesktop และ xrdp ด้วย PVS-Studio

ที่มา: will.com