Dit is de twadde resinsje yn in searje artikels oer testen fan iepen boarneprogramma's foar wurkjen mei it RDP-protokol. Dêryn sille wy sjen nei de rdesktop-client en de xrdp-tsjinner.
Wurdt brûkt as ark om flaters te identifisearjen . It is in statyske koade-analyzer foar C, C++, C# en Java-talen, beskikber op Windows, Linux en macOS-platfoarms.
It artikel presintearret allinnich dy flaters dy't like nijsgjirrich foar my. De projekten binne lykwols lyts, dus der wiene in pear flaters :).
remark. In earder artikel oer FreeRDP-projektferifikaasje is te finen .
rdesktop
- in fergese ymplemintaasje fan in RDP-kliïnt foar UNIX-basearre systemen. It kin ek brûkt wurde ûnder Windows as jo it projekt bouwe ûnder Cygwin. Lisinsearre ûnder GPLv3.
Dizze klant is heul populêr - it wurdt standert brûkt yn ReactOS, en jo kinne ek grafyske front-ends fan tredden fine. Hy is lykwols frij âld: syn earste release fûn plak op 4 april 2001 - op it stuit fan dit skriuwen is hy 17 jier âld.
Lykas ik earder opmurken, is it projekt heul lyts. It befettet sawat 30 tûzen rigels koade, wat in bytsje frjemd is sjoen syn leeftyd. Foar ferliking befettet FreeRDP 320 tûzen rigels. Hjir is de útfier fan it Cloc-programma:
Unberikbere koade
Unbeskikbere koade ûntdutsen. It is mooglik dat der in flater is. 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);
}De flater komt ús daliks tsjin yn 'e funksje main: wy sjogge de koade komme nei de operator werom - dit fragmint fiert ûnthâld skjinmeitsjen. De flater foarmje lykwols gjin bedriging: alle tawiisde ûnthâld sil wurde wiske troch it bestjoeringssysteem nei it ôfsluten fan it programma.
Gjin flater ôfhanneling
Array underrun is mooglik. De wearde fan 'n' yndeks koe -1 berikke. 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);
}
....
}De koade snippet yn dit gefal lêst fan it bestân yn in buffer oant it bestân einiget. D'r is hjir lykwols gjin flaterôfhanneling: as der wat mis giet, dan lêze sil weromkomme -1, en dan sil de array oerrinne útfier.
Mei help fan EOF yn char type
EOF moat net fergelike wurde mei in wearde fan it type 'char'. De '(c = fgetc(fp))' moat fan it 'int'-type wêze. 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++;
}
....
}Hjir sjogge wy ferkearde ôfhanneling fan it berikken fan it ein fan it bestân: if fgetc jout in karakter werom wêrfan de koade 0xFF is, it sil ynterpretearre wurde as it ein fan it bestân (EOF).
EOF it is in konstante, meastal definiearre as -1. Bygelyks, yn 'e CP1251-kodearring hat de lêste letter fan it Russyske alfabet de koade 0xFF, dy't oerienkomt mei it nûmer -1 as wy it hawwe oer in fariabele lykas char. It docht bliken dat it symboal 0xFF, lykas EOF (-1) wurdt ynterpretearre as it ein fan it bestân. Om sokke flaters te foarkommen, is it resultaat fan 'e funksje fgetc moatte wurde opslein yn in fariabele lykas int.
Typos
Fragmint 1
Ekspresje 'write_time' is altyd falsk. 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; // <=
....
}Miskien hat de skriuwer fan dizze koade it ferkeard || и && yn steat. Litte wy mooglike opsjes foar wearden beskôgje skriuwe_tiid и feroarje_tiid:
- Beide fariabelen binne gelyk oan 0: yn dit gefal komme wy yn in branch oars: fariabele mod_tiid sil altyd 0 wêze, nettsjinsteande de folgjende betingst.
- Ien fan 'e fariabelen is 0: mod_tiid sil gelyk wêze oan 0 (mits de oare fariabele hat in net-negative wearde), omdat MIN sil kieze de lytste fan de twa opsjes.
- Beide fariabelen binne net gelyk oan 0: kies de minimale wearde.
By it ferfangen fan de betingst mei skriuwtiid && feroarje_tiid it gedrach sil der korrekt útsjen:
- Ien of beide fariabelen binne net gelyk oan 0: kies in wearde net-nul.
- Beide fariabelen binne net gelyk oan 0: kies de minimale wearde.
Fragmint 2
Ekspresje is altyd wier. Wierskynlik moat de operator '&&' hjir brûkt wurde. 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;
....
}Blykber binne de operators hjir ek trochinoar || и &&, of == и !=: In fariabele kin de wearde 20 en 9 net tagelyk hawwe.
Unbeheind line kopiearjen
In oprop fan 'e funksje 'sprintf' sil liede ta oerstreaming fan 'e buffer 'folslein paad'. 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);
....
}As jo de funksje folslein besjen, sil it dúdlik wurde dat dizze koade gjin problemen feroarsaket. Se kinne lykwols yn 'e takomst ûntstean: ien achtleaze feroaring en wy sille in bufferoerstream krije - sprint wurdt net beheind troch neat, dus as wy paden gearhingje kinne wy oer de grinzen fan 'e array gean. It is oan te rieden om dizze oprop op te merken snprintf(folslein paad, PATH_MAX, ….).
Oerstallich betingst
In diel fan betingstekspresje is altyd wier: tafoegje > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}ynspeksje tafoegje > 0 it is hjir net nedich: de fariabele sil altyd grutter wêze as nul, om't lês % 4 sil de rest fan 'e divyzje werombringe, mar it sil nea gelyk wêze oan 4.
xrdp
- ymplemintaasje fan in RDP-tsjinner mei iepen boarne koade. It projekt is ferdield yn 2 dielen:
- xrdp - protokol ymplemintaasje. Ferspraat ûnder de Apache 2.0-lisinsje.
- xorgxrdp - In set fan Xorg-bestjoerders foar gebrûk mei xrdp. Lisinsje - X11 (lykas MIT, mar ferbiedt gebrûk yn reklame)
De ûntwikkeling fan it projekt is basearre op de resultaten fan rdesktop en FreeRDP. Yn 't earstoan, om mei grafiken te wurkjen, moasten jo in aparte VNC-tsjinner brûke, as in spesjale X11-tsjinner mei RDP-stipe - X11rdp, mar mei de komst fan xorgxrdp ferdwûn de needsaak foar har.
Yn dit artikel sille wy xorgxrdp net dekke.
It xrdp-projekt, lykas it foarige, is heul lyts en befettet sawat 80 tûzen rigels.
Mear typfouten
De koade befettet de kolleksje fan ferlykbere blokken. Kontrolearje items 'r', 'g', 'r' yn rigels 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++;
}
....
}
....
}Dizze koade is nommen út 'e librfxcodec-bibleteek, dy't de jpeg2000-koade foar RemoteFX ymplemintearret. Hjir binne blykber de grafyske gegevenskanalen trochinoar mingd - ynstee fan 'e "blauwe" kleur wurdt "read" opnommen. Dizze flater ferskynde nei alle gedachten as gefolch fan copy-paste.
Itselde probleem barde yn in ferlykbere funksje rfx_encode_format_argb, dy't de analysator ús ek fertelde:
De koade befettet de kolleksje fan ferlykbere blokken. Kontrolearje items 'a', 'r', 'g', 'r' yn rigels 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++;
}Array Declaration
Array oerrin is mooglik. De wearde fan 'i — 8' yndeks koe berikke 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];
....
}
....
}De deklaraasje en definysje fan 'e array yn dizze twa triemmen binne ynkompatibel - de grutte ferskilt mei 1. Der komme lykwols gjin flaters foar - de juste grutte wurdt oanjûn yn it evdev-map.c-bestân, dus der is gjin bûten grinzen. Dat dit is gewoan in brek dy't maklik kin wurde reparearre.
Ferkearde ferliking
In diel fan betingsten útdrukking is altyd falsk: (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))
{
....
}
....
}De funksje lêst in type fariabele net ûndertekene koart yn in fariabele lykas int. Kontrolearje is hjir net nedich, om't wy in net-ûndertekene fariabele lêze en it resultaat tawize oan in gruttere fariabele, sadat de fariabele gjin negative wearde kin nimme.
Unnedige kontrôles
In diel fan 'e betingste ekspresje is altyd wier: (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;
}
....
}Ungelikenskontrôles hawwe hjir gjin sin, om't wy oan it begjin al in ferliking hawwe. It is wierskynlik dat dit in typflater is en de ûntwikkelder woe de operator brûke || om ûnjildige arguminten út te filterjen.
konklúzje
By de kontrôle waarden gjin serieuze flaters identifisearre, mar in protte tekoartkommingen waarden fûn. Dizze ûntwerpen wurde lykwols brûkt yn in protte systemen, hoewol lyts yn omfang. In lyts projekt hat net needsaaklik in protte flaters, dus jo moatte de prestaasjes fan 'e analysator net allinich beoardielje op lytse projekten. Jo kinne hjir mear oer lêze yn it artikel "".
Jo kinne in proefferzje fan PVS-Studio fan ús downloade .
As jo dit artikel wolle diele mei in Ingelsktalig publyk, brûk dan de oersettingskeppeling: Sergey Larin.
Boarne: www.habr.com