Esta é a segunda revisión dunha serie de artigos sobre a proba de programas de código aberto para traballar co protocolo RDP. Nel miraremos o cliente rdesktop e o servidor xrdp.
Úsase como ferramenta para identificar erros . É un analizador de código estático para linguaxes C, C++, C# e Java, dispoñible en plataformas Windows, Linux e macOS.
O artigo presenta só aqueles erros que me pareceron interesantes. Non obstante, os proxectos son pequenos, polo que houbo poucos erros :).
Nota. Pódese atopar un artigo anterior sobre a verificación do proxecto FreeRDP .
rdesktop
— unha implementación gratuíta dun cliente RDP para sistemas baseados en UNIX. Tamén se pode usar en Windows se creas o proxecto en Cygwin. Con licenza GPLv3.
Este cliente é moi popular: úsase por defecto en ReactOS e tamén podes atopar interfaces gráficas de terceiros para el. Non obstante, é bastante vello: o seu primeiro lanzamento tivo lugar o 4 de abril de 2001; no momento de escribir este artigo, ten 17 anos.
Como comentei anteriormente, o proxecto é moi pequeno. Contén aproximadamente 30 mil liñas de código, o que é un pouco estraño tendo en conta a súa idade. Para comparación, FreeRDP contén 320 mil liñas. Aquí está a saída do programa Cloc:
Código inalcanzable
Detectouse un código non dispoñible. É posible que exista un erro. 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);
}O erro atópase connosco inmediatamente na función Inicio: vemos que o código vén despois do operador retorno — este fragmento realiza a limpeza da memoria. Non obstante, o erro non supón unha ameaza: toda a memoria asignada será borrada polo sistema operativo despois de que se peche o programa.
Sen tratamento de erros
É posible a subtracción da matriz. O valor do índice "n" pode chegar a -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);
}
....
}Neste caso, o fragmento de código lese desde o ficheiro nun búfer ata que remata o ficheiro. Non obstante, aquí non hai ningún tratamento de erros: se algo sae mal, entón ler devolverá -1, e entón a matriz será superada saída.
Usando EOF en tipo char
EOF non debe compararse cun valor do tipo 'char'. O '(c = fgetc(fp))' debe ser do tipo '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++;
}
....
}Aquí vemos un manexo incorrecto de chegar ao final do ficheiro: se fgetc devolve un carácter cuxo código é 0xFF, interpretarase como o final do ficheiro (EOF).
EOF é unha constante, normalmente definida como -1. Por exemplo, na codificación CP1251, a última letra do alfabeto ruso ten o código 0xFF, que corresponde ao número -1 se estamos a falar dunha variable como tanque. Acontece que o símbolo 0xFF, como EOF (-1) interprétase como o final do ficheiro. Para evitar tales erros, o resultado da función é fgetc debe almacenarse nunha variable como int.
Erros tipográficos
Fragmento 1
A expresión 'write_time' sempre é falsa. disco.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; // <=
....
}Quizais o autor deste código se equivocou || и && en condicións. Consideremos posibles opcións para os valores hora_escribir и cambio_tempo:
- Ambas variables son iguais a 0: neste caso acabaremos nunha rama outro: variable mod_time sempre será 0 independentemente da condición posterior.
- Unha das variables é 0: mod_time será igual a 0 (sempre que a outra variable teña un valor non negativo), porque MIN escollerá a menor das dúas opcións.
- Ambas variables non son iguais a 0: escolla o valor mínimo.
Ao substituír a condición por escribir_hora e& cambiar_hora o comportamento parecerá correcto:
- Unha ou ambas variables non son iguais a 0: escolla un valor distinto de cero.
- Ambas variables non son iguais a 0: escolla o valor mínimo.
Fragmento 2
A expresión é sempre certa. Probablemente o operador '&&' debería usarse aquí. disco.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;
....
}Ao parecer, aquí tamén se mesturan os operadores || и &&Ou == и !=: Unha variable non pode ter o valor 20 e 9 ao mesmo tempo.
Copia de liñas ilimitadas
Unha chamada á función 'sprintf' provocará un desbordamento do búfer 'fullpath'. disco.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);
....
}Cando mire a función ao completo, quedará claro que este código non causa problemas. Non obstante, poden xurdir no futuro: un cambio descoidado e obteremos un desbordamento de búfer - sprint non está limitado por nada, polo que ao concatenar camiños podemos ir máis alá dos límites da matriz. Recoméndase notar esta chamada snprintf(ruta completa, PATH_MAX, ….).
Condición redundante
Unha parte da expresión condicional é sempre verdadeira: engade > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка engadir > 0 aquí non hai necesidade: a variable sempre será maior que cero, porque ler % 4 devolverá o resto da división, pero nunca será igual a 4.
xrdp
— implantación dun servidor RDP con código aberto. O proxecto divídese en 2 partes:
- xrdp - implementación de protocolos. Distribuído baixo a licenza Apache 2.0.
- xorgxrdp - Un conxunto de controladores Xorg para usar con xrdp. Licenza - X11 (como MIT, pero prohibe o uso en publicidade)
O desenvolvemento do proxecto baséase nos resultados de rdesktop e FreeRDP. Inicialmente, para traballar con gráficos, tiña que usar un servidor VNC separado ou un servidor X11 especial con soporte RDP - X11rdp, pero coa chegada de xorgxrdp, a necesidade deles desapareceu.
Neste artigo non trataremos xorgxrdp.
O proxecto xrdp, como o anterior, é moi pequeno e contén aproximadamente 80 mil liñas.
Máis erros tipográficos
O código contén a colección de bloques similares. Comprobe os elementos "r", "g", "r" nas liñas 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++;
}
....
}
....
}Este código foi tomado da biblioteca librfxcodec, que implementa o codec jpeg2000 para RemoteFX. Aquí, ao parecer, as canles de datos gráficos mestúranse: en lugar da cor "azul", grávase o "vermello". Este erro probablemente apareceu como resultado de copiar e pegar.
O mesmo problema ocorreu nunha función similar rfx_encode_format_argb, que tamén nos dixo o analizador:
O código contén a colección de bloques similares. Comprobe os elementos "a", "r", "g", "r" nas liñas 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++;
}Declaración de matriz
É posible o exceso de matriz. O valor do índice 'i — 8' podería chegar a 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];
....
}
....
}A declaración e definición da matriz nestes dous ficheiros son incompatibles - o tamaño difire en 1. Non obstante, non se producen erros - o tamaño correcto especifícase no ficheiro evdev-map.c, polo que non hai límites. Polo tanto, este é só un erro que se pode solucionar facilmente.
Comparación incorrecta
Unha parte da expresión condicional é sempre falsa: (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))
{
....
}
....
}A función le unha variable de tipo curto sen asinar nunha variable como int. Non é necesario verificar aquí porque estamos lendo unha variable sen asinar e asignando o resultado a unha variable maior, polo que a variable non pode tomar un valor negativo.
Comprobacións innecesarias
Unha parte da expresión condicional é sempre verdadeira: (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;
}
....
}Os controis de desigualdade non teñen sentido aquí xa que xa temos unha comparación ao principio. É probable que se trate dun erro tipográfico e o programador quixese utilizar o operador || para filtrar argumentos non válidos.
Conclusión
Durante a auditoría non se identificaron erros graves, pero si se atoparon moitas deficiencias. Non obstante, estes deseños úsanse en moitos sistemas, aínda que de pequeno alcance. Un proxecto pequeno non ten necesariamente moitos erros, polo que non debes xulgar o rendemento do analizador só en proxectos pequenos. Podes ler máis sobre isto no artigo "«.
Podes descargar unha versión de proba de PVS-Studio de nós .
Se queres compartir este artigo cun público de fala inglesa, utiliza a ligazón de tradución: Sergey Larin.
Fonte: www.habr.com