Гэта другі агляд з цыклу артыкулаў аб праверцы адчыненых праграм для працы з пратаколам RDP. У ёй мы разгледзім кліент rdesktop і сэрвер xrdp.
У якасці прылады для выяўлення памылак выкарыстоўваўся . Гэта статычны аналізатар кода для моваў C, C++, C# і Java, даступны на платформах Windows, Linux і macOS.
У артыкуле прадстаўлены толькі тыя памылкі, якія падаліся мне цікавымі. Зрэшты, праекты маленькія, таму і памылак было мала :).
Заўвага. Папярэдні артыкул аб праверцы праекта FreeRDP можна знайсці .
працоўны стол
- свабодная рэалізацыя кліента RDP для UNIX-based сістэм. Яго таксама можна выкарыстоўваць і пад Windows, калі збіраць праект пад Cygwin. Ліцэнзаваны пад GPLv3.
Гэты кліент мае вялікую папулярнасць - ён выкарыстоўваецца па змаўчанні ў ReactOS, таксама для яго можна знайсці іншыя графічныя front-end'ы. Тым не менш, ён даволі стары: першы рэліз адбыўся 4 красавіка 2001 года - на момант напісання артыкула яго ўзрост складае 17 гадоў.
Як я ўжо адзначыў раней, праект зусім маленькі. Ён змяшчае прыкладна 30 тысяч радкоў кода, што крыху дзіўна, улічваючы яго ўзрост. Для параўнання, FreeRDP утрымоўвае ў сабе 320 тысяч радкоў. Вось выснова праграмы Cloc:
Недасягальны код
Unreachable code пазначаны. Гэта магчыма, што памылка з'яўляецца. 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);
}Памылка нас сустракае адразу ж у функцыі асноўнай: мы бачым код, які ідзе пасля аператара вяртаць - Гэты фрагмент ажыццяўляе ачыстку памяці. Тым не менш, памылка не ўяўляе пагрозы: уся выдзеленая памяць вычысціцца аперацыйнай сістэмай пасля завяршэння працы праграмы.
Адсутнасць апрацоўкі памылак
Array underrun is possible. Зніжэнне 'n' index could reach -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 у тыпе char
EOF не павінен быць аб'яднаны з значэннем тыпу 'char'. The '(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++;
}
....
}Тут мы бачым некарэктную апрацоўку дасягнення канца файла: калі fget і г.д верне сімвал, код якога роўны 0xFF, то ён будзе ўспрыняты як канец файла (EOF).
EOF гэта канстанта, вызначаная звычайна як -1. Да прыкладу, у кадоўцы CP1251 апошняя літара рускага алфавіту мае код 0xFF, што адпавядае ліку -1, калі мы гаворым пра зменную тыпу калясьніца. Атрымліваецца, што сімвал 0xFF, як і EOF (-1) успрымаецца як канец файла. Каб пазбегнуць падобных памылак вынік працы функцыі fget і г.д варта захоўваць у зменнай тыпу INT.
памылкі друку
Фрагмент 1
Expression 'write_time' is always 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: у гэтым выпадку мы патрапім у галінку яшчэ: пераменная mod_time заўсёды будзе роўна 0 незалежна ад наступнай умовы.
- Адна са зменных роўная 0: mod_time будзе роўна 0 (пры ўмове, што іншая зменная мае неадмоўнае значэнне), т. да. MIN абярэ найменшы з двух варыянтаў.
- Абедзве зменныя не роўныя 0: выбіраемы мінімальнае значэнне.
Пры замене ўмовы на write_time && change_time паводзіны стануць выглядаць карэктнымі:
- Адна ці абедзве зменныя не роўныя 0: выбіраемы ненулявое значэнне.
- Абедзве зменныя не роўныя 0: выбіраемы мінімальнае значэнне.
Фрагмент 2
Expression is always true. Probably Operator '&&' павінен быць выкарыстаны тут. 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.
Неабмежаваную капіраванне радка
Тэлефон 'спрынт' функцыі павінен быць звязаны з узроўнем buffer '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);
....
}Пры разглядзе функцыі поўнасцю стане зразумела, што гэты код не выклікае праблем. Аднак яны могуць узнікнуць у будучыні: адна неасцярожная змена, і мы атрымаем перапаўненне буфера. спрынт нічым не абмежаваны, таму пры канкатэнацыі шляхоў мы можам выйсці за межы масіва. Рэкамендуецца заўважыць гэты выклік на snprintf(fullpath, PATH_MAX, ….).
Залішняя ўмова
Apart of conditional expression is always true: 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
- рэалізацыя RDP сервера з адкрытым зыходным кодам. Праект падзелены на 2 часткі:
- xrdp - рэалізацыя пратаколу. Распаўсюджваецца пад ліцэнзіяй Apache 2.0.
- xorgxrdp - набор драйвераў Xorg для выкарыстання з xrdp. Ліцэнзія - X11 (як MIT, але забараняе выкарыстанне ў рэкламе)
Распрацоўка праекту грунтуецца на выніках rdesktop і FreeRDP. Першапачаткова для працы з графікай даводзілася выкарыстоўваць асобны VNC сервер, альбо ж спецыяльны сервер X11 з падтрымкай RDP – X11rdp, аднак са з'яўленнем xorgxrdp патрэба ў іх адпала.
У гэтым артыкуле мы не будзем закранаць xorgxrdp.
Праект xrdp, як і папярэдні, зусім невялікі і змяшчае прыкладна 80 тысяч радкоў.
Яшчэ памылкі друку
Code contains the collection of similar blocks. Check items '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. Тут, па ўсёй бачнасці, пераблытаныя каналы графічных дадзеных - замест "сіняга" колеры запісваецца "чырвоны". Такая памылка, хутчэй за ўсё, зьявілася ў выніку copy-paste.
Гэтая ж праблема патрапіла і ў падобную функцыю rfx_encode_format_argb, пра што нам таксама паведаміў аналізатар:
Code contains the collection of similar blocks. Check items 'a', 'r', 'g', 'r' in lines 260, 261, 262, 263. 260
while (x < 64)
{
*la_buf++ = a;
*lr_buf++ = r;
*lg_buf++ = g;
*lb_buf++ = r;
x++;
}Аб'ява масіва
Array overrun is possible. The value of 'i - 8' index could reach 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 паказаны дакладны памер, таму выхаду за межы няма. Так што гэта проста недахоп, які проста выправіць.
Некарэктнае параўнанне
Apart of conditional expression is always 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. Праверка тут не патрэбна, бо мы счытваем зменную беззнакового тыпу і прысвойваем вынік зменнай большага памеру, таму зменная не можа прыняць адмоўнае значэнне.
Непатрэбныя праверкі
Apart of conditional expression is always 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.
Крыніца: habr.com