Esta é a segunda revisão de uma série de artigos sobre testes de programas de código aberto para trabalhar com o protocolo RDP. Nele veremos o cliente rdesktop e o servidor xrdp.
Usado como uma ferramenta para identificar erros . É um analisador de código estático para linguagens C, C++, C# e Java, disponível nas plataformas Windows, Linux e macOS.
O artigo apresenta apenas os erros que me pareceram interessantes. Porém, os projetos são pequenos, então houve poucos erros :).
Nota. Um artigo anterior sobre a verificação do projeto FreeRDP pode ser encontrado .
rdesktop
— uma implementação gratuita de um cliente RDP para sistemas baseados em UNIX. Também pode ser usado no Windows se você construir o projeto no Cygwin. Licenciado sob GPLv3.
Este cliente é muito popular - é usado por padrão no ReactOS, e você também pode encontrar front-ends gráficos de terceiros para ele. Porém, ele é bastante idoso: seu primeiro lançamento ocorreu em 4 de abril de 2001 - no momento em que este artigo foi escrito, ele tinha 17 anos.
Como observei anteriormente, o projeto é bastante pequeno. Ele contém aproximadamente 30 mil linhas de código, o que é um pouco estranho considerando sua idade. Para efeito de comparação, o FreeRDP contém 320 mil linhas. Aqui está a saída do programa Cloc:
Código inacessível
Código indisponível detectado. É possível que um erro esteja presente. 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 nos encontra imediatamente na função principal: vemos o código vindo depois do operador retorno — este fragmento realiza limpeza de memória. No entanto, o erro não representa uma ameaça: toda a memória alocada será limpa pelo sistema operacional após o encerramento do programa.
Sem tratamento de erros
A subexecução da matriz é possível. 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);
}
....
}O trecho de código, neste caso, lê o arquivo em um buffer até que o arquivo termine. No entanto, não há tratamento de erros aqui: se algo der errado, então ler retornará -1 e então o array será sobrecarregado saída.
Usando EOF no tipo char
EOF não deve ser comparado com um valor do tipo 'char'. O '(c = fgetc(fp))' deve 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++;
}
....
}Aqui vemos o tratamento incorreto para chegar ao final do arquivo: if fgetc retorna um caractere cujo código é 0xFF, será interpretado como o final do arquivo (EOF).
EOF é uma constante, geralmente definida como -1. Por exemplo, na codificação CP1251, a última letra do alfabeto russo possui o código 0xFF, que corresponde ao número -1 se estivermos falando de uma variável como carbonizar. Acontece que o símbolo 0xFF, como EOF (-1) é interpretado como o fim do arquivo. Para evitar tais erros, o resultado da função é fgetc deve ser armazenado em uma variável como int.
Erros de digitação
Fragmento 1
A expressão '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; // <=
....
}Talvez o autor deste código tenha entendido errado || и && em condição. Vamos considerar possíveis opções de valores hora_de_gravação и hora_da_mudança:
- Ambas as variáveis são iguais a 0: neste caso terminaremos em um ramo outro: variável hora_mod_ sempre será 0, independentemente da condição subsequente.
- Uma das variáveis é 0: hora_mod_ será igual a 0 (desde que a outra variável tenha valor não negativo), pois MIN escolherá a menor das duas opções.
- Ambas as variáveis não são iguais a 0: escolha o valor mínimo.
Ao substituir a condição por hora_de_gravação && hora_de_alteração o comportamento parecerá correto:
- Uma ou ambas as variáveis não são iguais a 0: escolha um valor diferente de zero.
- Ambas as variáveis não são iguais a 0: escolha o valor mínimo.
Fragmento 2
A expressão é sempre verdadeira. Provavelmente o operador '&&' deve ser usado aqui. 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;
....
}Aparentemente os operadores também estão confusos aqui || и &&Ou == и !=: Uma variável não pode ter os valores 20 e 9 ao mesmo tempo.
Cópia de linha ilimitada
Uma chamada da função 'sprintf' levará ao estouro do buffer '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);
....
}Ao observar a função por completo, ficará claro que esse código não causa problemas. No entanto, eles podem surgir no futuro: uma mudança descuidada e teremos um buffer overflow - corrida não é limitado por nada, portanto, ao concatenar caminhos, podemos ir além dos limites do array. Recomenda-se observar esta chamada em snprintf(caminho completo, PATH_MAX,….).
Condição redundante
Uma parte da expressão condicional é sempre verdadeira: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка adicionar> 0 não há necessidade aqui: a variável sempre será maior que zero, porque leia% 4 retornará o resto da divisão, mas nunca será igual a 4.
xrdp
— implementação de um servidor RDP com código-fonte aberto. O projeto está dividido em 2 partes:
- xrdp - implementação de protocolo. Distribuído sob a licença Apache 2.0.
- xorgxrdp - Um conjunto de drivers Xorg para uso com xrdp. Licença - X11 (como MIT, mas proíbe uso em publicidade)
O desenvolvimento do projeto é baseado nos resultados do rdesktop e do FreeRDP. Inicialmente, para trabalhar com gráficos, era necessário usar um servidor VNC separado, ou um servidor X11 especial com suporte RDP - X11rdp, mas com o advento do xorgxrdp, a necessidade deles desapareceu.
Neste artigo não abordaremos o xorgxrdp.
O projeto xrdp, assim como o anterior, é muito pequeno e contém aproximadamente 80 mil linhas.
Mais erros de digitação
O código contém a coleção de blocos semelhantes. Verifique os itens 'r', 'g', 'r' nas linhas 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 retirado da biblioteca librfxcodec, que implementa o codec jpeg2000 para RemoteFX. Aqui, aparentemente, os canais de dados gráficos estão confusos - em vez da cor “azul”, é gravado “vermelho”. Este erro provavelmente apareceu como resultado de copiar e colar.
O mesmo problema ocorreu em uma função semelhante rfx_encode_format_argb, que o analisador também nos disse:
O código contém a coleção de blocos semelhantes. Verifique os itens 'a', 'r', 'g', 'r' nas linhas 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++;
}Declaração de matriz
A superação da matriz é possível. O valor do índice 'i - 8' pode 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 declaração e a definição do array nesses dois arquivos são incompatíveis - o tamanho difere em 1. No entanto, nenhum erro ocorre - o tamanho correto é especificado no arquivo evdev-map.c, portanto não há limites. Portanto, este é apenas um bug que pode ser facilmente corrigido.
Comparação incorreta
Uma parte da expressão condicional é sempre falsa: (cap_len < 0). xrdp_caps.c616
// 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 função lê uma variável de tipo curto sem sinal em uma variável como int. A verificação não é necessária aqui porque estamos lendo uma variável sem sinal e atribuindo o resultado a uma variável maior, portanto a variável não pode assumir um valor negativo.
Verificações desnecessárias
Uma parte da expressão 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;
}
....
}As verificações de desigualdade não fazem sentido aqui, pois já temos uma comparação no início. É provável que seja um erro de digitação e o desenvolvedor queira usar o operador || para filtrar argumentos inválidos.
Conclusão
Durante a auditoria, não foram identificados erros graves, mas foram encontradas muitas deficiências. No entanto, esses projetos são usados em muitos sistemas, embora de escopo pequeno. Um projeto pequeno não necessariamente apresenta muitos erros, portanto você não deve julgar o desempenho do analisador apenas em projetos pequenos. Você pode ler mais sobre isso no artigo “".
Você pode baixar uma versão de teste do PVS-Studio conosco .
Se você quiser compartilhar este artigo com um público que fala inglês, use o link de tradução: Sergey Larin.
Fonte: habr.com