Esta es la segunda revisión de una serie de artículos sobre cómo probar programas de código abierto para trabajar con el protocolo RDP. En él veremos el cliente rdesktop y el servidor xrdp.
Se utiliza como herramienta para identificar errores. . Es un analizador de código estático para lenguajes C, C++, C# y Java, disponible en plataformas Windows, Linux y macOS.
El artículo presenta sólo aquellos errores que me parecieron interesantes. Sin embargo, los proyectos son pequeños, por lo que hubo pocos errores :).
Nota. Puede encontrar un artículo anterior sobre la verificación del proyecto FreeRDP. .
rdesktop
— una implementación gratuita de un cliente RDP para sistemas basados en UNIX. También se puede utilizar en Windows si crea el proyecto en Cygwin. Licenciado bajo GPLv3.
Este cliente es muy popular: se usa de forma predeterminada en ReactOS y también puede encontrar interfaces gráficas de terceros para él. Sin embargo, es bastante mayor: su primera liberación tuvo lugar el 4 de abril de 2001; en el momento de escribir este artículo tiene 17 años.
Como señalé anteriormente, el proyecto es muy pequeño. Contiene aproximadamente 30 mil líneas de código, lo cual es un poco extraño considerando su antigüedad. A modo de comparación, FreeRDP contiene 320 mil líneas. Aquí está el resultado del programa Cloc:
Código inalcanzable
Se ha detectado un código no disponible. Es posible que haya un error. 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);
}El error nos encuentra inmediatamente en la función. principal: vemos el código que viene después del operador volvemos — este fragmento realiza una limpieza de memoria. Sin embargo, el error no representa una amenaza: el sistema operativo borrará toda la memoria asignada después de que se cierre el programa.
Sin manejo de errores
Es posible que la matriz esté insuficiente. El valor del índice 'n' podría llegar 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);
}
....
}En este caso, el fragmento de código lee el archivo en un búfer hasta que finaliza el archivo. Sin embargo, aquí no hay manejo de errores: si algo sale mal, entonces leer devolverá -1, y luego la matriz será invadida salida.
Usando EOF en tipo char
EOF no debe compararse con un valor del tipo 'char'. El '(c = fgetc(fp))' debe ser del 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 manejo incorrecto de llegar al final del archivo: si fgetc devuelve un carácter cuyo código es 0xFF, será interpretado como el final del archivo (EOF).
EOF es una constante, generalmente definida como -1. Por ejemplo, en la codificación CP1251, la última letra del alfabeto ruso tiene el código 0xFF, que corresponde al número -1 si hablamos de una variable como tanque. Resulta que el símbolo 0xFF, como EOF (-1) se interpreta como el final del archivo. Para evitar este tipo de errores, el resultado de la función es fgetc debe almacenarse en una variable como int.
Errores tipográficos
Fragmento 1
La expresión 'write_time' siempre es 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; // <=
....
}Quizás el autor de este código se equivocó. || и && en condicion. Consideremos posibles opciones para los valores. tiempo_escritura и cambio de hora:
- Ambas variables son iguales a 0: en este caso terminaremos en una rama más: variable tiempo_mod siempre será 0 independientemente de la condición posterior.
- Una de las variables es 0: tiempo_mod será igual a 0 (siempre que la otra variable tenga un valor no negativo), porque MIN Elegirá la menor de las dos opciones.
- Ambas variables no son iguales a 0: elija el valor mínimo.
Al reemplazar la condición con tiempo_escritura && tiempo_cambio el comportamiento se verá correcto:
- Una o ambas variables no son iguales a 0: elija un valor distinto de cero.
- Ambas variables no son iguales a 0: elija el valor mínimo.
Fragmento 2
La expresión siempre es cierta. Probablemente aquí debería utilizarse el operador '&&'. 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;
....
}Al parecer, los operadores también están confundidos aquí. || и &&O == и !=: Una variable no puede tener el valor 20 y 9 al mismo tiempo.
Copia de líneas ilimitadas
Una llamada a la función 'sprintf' provocará un desbordamiento del 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);
....
}Cuando observe la función en su totalidad, quedará claro que este código no causa problemas. Sin embargo, pueden surgir en el futuro: un cambio descuidado y tendremos un desbordamiento del buffer. correr no está limitado por nada, por lo que al concatenar rutas podemos ir más allá de los límites del array. Se recomienda notar esta llamada en snprintf(ruta completa, PATH_MAX,….).
Condición redundante
Una parte de la expresión condicional siempre es verdadera: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка añadir > 0 aquí no hay necesidad: la variable siempre será mayor que cero, porque leer % 4 devolverá el resto de la división, pero nunca será igual a 4.
xrdp
— implementación de un servidor RDP con código fuente abierto. El proyecto se divide en 2 partes:
- xrdp: implementación del protocolo. Distribuido bajo la licencia Apache 2.0.
- xorgxrdp: un conjunto de controladores Xorg para usar con xrdp. Licencia: X11 (como MIT, pero prohíbe su uso en publicidad)
El desarrollo del proyecto se basa en los resultados de rdesktop y FreeRDP. Inicialmente, para trabajar con gráficos, era necesario usar un servidor VNC separado o un servidor X11 especial con soporte RDP: X11rdp, pero con la llegada de xorgxrdp, la necesidad desapareció.
En este artículo no cubriremos xorgxrdp.
El proyecto xrdp, como el anterior, es muy pequeño y contiene aproximadamente 80 mil líneas.
Más errores tipográficos
El código contiene la colección de bloques similares. Marque los elementos 'r', 'g', 'r' en las líneas 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 fue tomado de la biblioteca librfxcodec, que implementa el códec jpeg2000 para RemoteFX. Aquí, aparentemente, los canales de datos gráficos están mezclados: en lugar del color "azul", se escribe "rojo". Este error probablemente apareció como resultado de copiar y pegar.
El mismo problema ocurrió en una función similar. rfx_encode_formato_argb, que el analizador también nos dijo:
El código contiene la colección de bloques similares. Marque los elementos 'a', 'r', 'g', 'r' en las líneas 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
Es posible que la matriz se desborde. El valor del índice 'i — 8' podría llegar 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];
....
}
....
}La declaración y definición de la matriz en estos dos archivos son incompatibles: el tamaño difiere en 1. Sin embargo, no se producen errores: el tamaño correcto se especifica en el archivo evdev-map.c, por lo que no hay límites. Así que esto es sólo un error que se puede solucionar fácilmente.
Comparación incorrecta
Una parte de la expresión condicional siempre es 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))
{
....
}
....
}La función lee una variable de tipo. unsigned short en una variable como int. No es necesario verificar aquí porque estamos leyendo una variable sin signo y asignando el resultado a una variable más grande, por lo que la variable no puede tomar un valor negativo.
Controles innecesarios
Una parte de la expresión condicional siempre es verdadera: (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;
}
....
}Los controles de desigualdad no tienen sentido aquí porque ya tenemos una comparación al principio. Es probable que se trate de un error tipográfico y que el desarrollador quisiera utilizar el operador || para filtrar argumentos no válidos.
Conclusión
Durante la auditoría no se identificaron errores graves, pero sí muchas deficiencias. Sin embargo, estos diseños se utilizan en muchos sistemas, aunque de pequeño alcance. Un proyecto pequeño no necesariamente tiene muchos errores, por lo que no debe juzgar el rendimiento del analizador sólo en proyectos pequeños. Puedes leer más sobre esto en el artículo “«.
Puede descargar una versión de prueba de PVS-Studio con nosotros .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Sergey Larin.
Fuente: habr.com