Đây là bài đánh giá thứ hai trong loạt bài viết về thử nghiệm các chương trình nguồn mở để làm việc với giao thức RDP. Trong đó chúng ta sẽ xem xét máy khách rdesktop và máy chủ xrdp.
Được sử dụng như một công cụ để xác định lỗi . Nó là một công cụ phân tích mã tĩnh cho các ngôn ngữ C, C++, C# và Java, có sẵn trên nền tảng Windows, Linux và macOS.
Bài viết chỉ trình bày những lỗi mà tôi thấy thú vị. Tuy nhiên dự án có quy mô nhỏ nên ít sai sót :).
Ghi. Có thể tìm thấy bài viết trước về xác minh dự án FreeRDP .
máy tính để bàn
- triển khai miễn phí ứng dụng khách RDP cho các hệ thống dựa trên UNIX. Nó cũng có thể được sử dụng trong Windows nếu bạn xây dựng dự án dưới Cygwin. Được cấp phép theo GPLv3.
Ứng dụng khách này rất phổ biến - nó được sử dụng theo mặc định trong ReactOS và bạn cũng có thể tìm thấy giao diện người dùng đồ họa của bên thứ ba cho nó. Tuy nhiên, anh ấy đã khá già: lần ra mắt đầu tiên của anh ấy diễn ra vào ngày 4 tháng 2001 năm 17 - tại thời điểm viết bài, anh ấy XNUMX tuổi.
Như tôi đã lưu ý trước đó, dự án này rất nhỏ. Nó chứa khoảng 30 nghìn dòng mã, điều này hơi lạ so với tuổi đời của nó. Để so sánh, FreeRDP chứa 320 nghìn dòng. Đây là kết quả của chương trình Cloc:
Mã không thể truy cập
Đã phát hiện thấy mã không khả dụng. Có thể có lỗi. 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);
}Lỗi gặp chúng ta ngay trong hàm chính: chúng ta thấy mã ở sau toán tử trở lại — đoạn này thực hiện việc dọn dẹp bộ nhớ. Tuy nhiên, lỗi không gây ra mối đe dọa: tất cả bộ nhớ được phân bổ sẽ bị hệ điều hành xóa sau khi thoát khỏi chương trình.
Không xử lý lỗi
Có thể chạy ngầm mảng. Giá trị của chỉ số 'n' có thể đạt -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);
}
....
}Đoạn mã trong trường hợp này đọc từ tệp vào bộ đệm cho đến khi tệp kết thúc. Tuy nhiên, không có cách xử lý lỗi nào ở đây: nếu có sự cố xảy ra thì đọc sẽ trả về -1, và sau đó mảng sẽ bị tràn đầu ra.
Sử dụng EOF trong kiểu char
Không nên so sánh EOF với giá trị thuộc loại 'char'. '(c = fgetc(fp))' phải thuộc loại '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++;
}
....
}Ở đây chúng ta thấy cách xử lý không chính xác khi đến cuối tệp: if fgetc trả về một ký tự có mã là 0xFF, nó sẽ được hiểu là phần cuối của tệp (EOF).
EOF nó là một hằng số, thường được định nghĩa là -1. Ví dụ: trong mã hóa CP1251, chữ cái cuối cùng của bảng chữ cái tiếng Nga có mã 0xFF, tương ứng với số -1 nếu chúng ta đang nói về một biến như xe tăng. Hóa ra ký hiệu 0xFF, như EOF (-1) được hiểu là phần cuối của tệp. Để tránh những lỗi như vậy, kết quả của hàm là fgetc nên được lưu trữ trong một biến như int.
Lỗi đánh máy
Đoạn 1
Biểu thức 'write_time' luôn sai. đĩa.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; // <=
....
}Có lẽ tác giả của mã này đã hiểu sai || и && trong điều kiện. Hãy xem xét các tùy chọn có thể có cho các giá trị viết_thời gian и thay đổi thời gian:
- Cả hai biến đều bằng 0: trong trường hợp này chúng ta sẽ kết thúc ở một nhánh khác: Biến đổi mod_time sẽ luôn bằng 0 bất kể điều kiện tiếp theo.
- Một trong các biến là 0: mod_time sẽ bằng 0 (với điều kiện biến còn lại có giá trị không âm), bởi vì MIN sẽ chọn cái nhỏ hơn trong hai lựa chọn.
- Cả hai biến đều không bằng 0: chọn giá trị nhỏ nhất.
Khi thay thế điều kiện bằng write_time && Change_time hành vi sẽ có vẻ chính xác:
- Một hoặc cả hai biến không bằng 0: chọn giá trị khác XNUMX.
- Cả hai biến đều không bằng 0: chọn giá trị nhỏ nhất.
Đoạn 2
Biểu hiện luôn luôn đúng. Có lẽ nên sử dụng toán tử '&&' ở đây. đĩa.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;
....
}Rõ ràng các nhà khai thác cũng bị trộn lẫn ở đây || и &&Hoặc == и !=: Một biến không thể có giá trị 20 và 9 cùng một lúc.
Sao chép dòng không giới hạn
Lệnh gọi hàm 'sprintf' sẽ dẫn đến tràn bộ đệm 'fullpath'. đĩa.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);
....
}Khi bạn xem xét toàn bộ hàm, bạn sẽ thấy rõ rằng mã này không gây ra vấn đề gì. Tuy nhiên, chúng có thể phát sinh trong tương lai: một thay đổi bất cẩn và chúng ta sẽ bị tràn bộ đệm - chạy nước rút không bị giới hạn bởi bất cứ điều gì nên khi nối các đường dẫn chúng ta có thể vượt ra ngoài ranh giới của mảng. Nên chú ý cuộc gọi này trên snprintf(đường dẫn đầy đủ, PATH_MAX, ….).
Tình trạng dư thừa
Một phần của biểu thức điều kiện luôn đúng: add > 0.scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка thêm > 0 không cần thiết ở đây: biến sẽ luôn lớn hơn XNUMX, bởi vì đọc% 4 sẽ trả về phần dư của phép chia, nhưng nó sẽ không bao giờ bằng 4.
xrdp
- triển khai máy chủ RDP với mã nguồn mở. Dự án được chia làm 2 phần:
- xrdp - triển khai giao thức. Được phân phối theo giấy phép Apache 2.0.
- xorgxrdp - Một bộ trình điều khiển Xorg để sử dụng với xrdp. Giấy phép - X11 (giống MIT, nhưng cấm sử dụng trong quảng cáo)
Sự phát triển của dự án dựa trên kết quả của rdesktop và FreeRDP. Ban đầu, để làm việc với đồ họa, bạn phải sử dụng một máy chủ VNC riêng hoặc máy chủ X11 đặc biệt có hỗ trợ RDP - X11rdp, nhưng với sự ra đời của xorgxrdp, nhu cầu về chúng đã không còn nữa.
Trong bài viết này, chúng tôi sẽ không đề cập đến xorgxrdp.
Dự án xrdp, giống như dự án trước, rất nhỏ và chứa khoảng 80 nghìn dòng.
Nhiều lỗi chính tả hơn
Mã này chứa tập hợp các khối tương tự. Kiểm tra các mục 'r', 'g', 'r' ở dòng 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++;
}
....
}
....
}Mã này được lấy từ thư viện librfxcodec, thư viện này triển khai codec jpeg2000 cho RemoteFX. Ở đây, rõ ràng, các kênh dữ liệu đồ họa đã bị trộn lẫn - thay vì màu “xanh”, màu “đỏ” được ghi lại. Lỗi này rất có thể xuất hiện do sao chép-dán.
Vấn đề tương tự xảy ra trong một chức năng tương tự rfx_encode_format_argb, mà máy phân tích cũng cho chúng tôi biết:
Mã này chứa tập hợp các khối tương tự. Kiểm tra các mục 'a', 'r', 'g', 'r' ở dòng 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++;
}Khai báo mảng
Có thể tràn mảng. Giá trị của chỉ số 'i — 8' có thể đạt tới 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];
....
}
....
}Việc khai báo và định nghĩa mảng trong hai tệp này không tương thích - kích thước chênh nhau 1. Tuy nhiên, không có lỗi xảy ra - kích thước chính xác được chỉ định trong tệp evdev-map.c, do đó không có giới hạn. Vì vậy, đây chỉ là một lỗi có thể dễ dàng sửa chữa.
So sánh không chính xác
Một phần của biểu thức điều kiện luôn sai: (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))
{
....
}
....
}Hàm đọc một biến kiểu không dấu ngắn thành một biến như int. Ở đây không cần kiểm tra vì chúng ta đang đọc một biến không dấu và gán kết quả cho một biến lớn hơn, do đó biến đó không thể nhận giá trị âm.
Kiểm tra không cần thiết
Một phần của biểu thức điều kiện luôn đúng: (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;
}
....
}Việc kiểm tra sự bất bình đẳng ở đây không có ý nghĩa gì vì chúng ta đã có sự so sánh ngay từ đầu. Có khả năng đây là lỗi đánh máy và nhà phát triển muốn sử dụng toán tử || để lọc ra các đối số không hợp lệ.
Kết luận
Trong quá trình kiểm toán, không phát hiện sai phạm nghiêm trọng nhưng phát hiện nhiều bất cập. Tuy nhiên, những thiết kế này được sử dụng trong nhiều hệ thống, mặc dù phạm vi nhỏ. Một dự án nhỏ không nhất thiết có nhiều lỗi, vì vậy bạn không nên chỉ đánh giá hiệu suất của máy phân tích trên các dự án nhỏ. Bạn có thể đọc thêm về điều này trong bài viết “".
Bạn có thể tải xuống phiên bản dùng thử của PVS-Studio từ chúng tôi .
Nếu bạn muốn chia sẻ bài viết này với khán giả nói tiếng Anh, vui lòng sử dụng liên kết dịch: Sergey Larin.
Nguồn: www.habr.com