Esta es la segunda publicación de nuestra serie de artículos sobre los resultados de verificar el software de código abierto que funciona con el protocolo RDP. Hoy vamos a echar un vistazo al cliente rdesktop y al servidor xrdp.
El análisis fue realizado por
PVS-Studio . Este es un analizador estático para código escrito en C, C ++, C # y Java, y se ejecuta en Windows, Linux y macOS.
Discutiré solo los errores que me parecieron más interesantes. Por otro lado, dado que los proyectos son bastante pequeños, no hay muchos errores en ellos de todos modos :).
Nota El artículo anterior sobre la verificación de FreeRDP está disponible
aquí .
rdesktop
rdesktop es un cliente RDP gratuito para sistemas basados en UNIX. También puede ejecutarse en Windows si está construido bajo Cygwin. rdesktop se lanza bajo GPLv3.
Este es un cliente muy popular. Se utiliza como cliente predeterminado en ReactOS, y también puede encontrar interfaces gráficas de terceros para acompañarlo. Sin embargo, el proyecto es bastante antiguo: se lanzó por primera vez el 4 de abril de 2001 y tiene 17 años, a partir de este escrito.
Como ya dije, el proyecto es muy pequeño: alrededor de 30 KLOC, lo cual es un poco extraño teniendo en cuenta su antigüedad. Compare eso con FreeRDP con sus 320 KLOC. Aquí está la salida de Cloc:
Código inalcanzable
V779 Código inalcanzable detectado. Es posible que haya un error 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); }
El primer error se encuentra inmediatamente en la función
principal : el código que sigue a la declaración de
retorno estaba destinado a liberar la memoria asignada anteriormente. Pero este defecto no es peligroso porque el sistema operativo liberará toda la memoria asignada previamente una vez que el programa finalice.
Sin manejo de errores
V557 Array underrun es posible. El valor del índice 'n' podría alcanzar -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] = '\0';
El contenido del archivo se lee en el búfer hasta que se alcanza EOF. Al mismo tiempo, este código carece de un mecanismo de manejo de errores, y si algo sale mal,
leer devolverá -1 y la ejecución comenzará a leer más allá de los límites de la matriz de
salida .
Usando EOF en char
V739 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++; } .... }
Este código implementa un manejo
EOF incorrecto: si
fgetc devuelve un carácter cuyo código es 0xFF, se interpretará como el final del archivo (
EOF ).
EOF es una constante típicamente definida como -1. Por ejemplo, en la codificación CP1251, la última letra del alfabeto ruso se codifica como 0xFF, que corresponde al número -1 en el tipo
char . Significa que el carácter 0xFF, al igual que
EOF (-1), se interpretará como el final del archivo. Para evitar errores como ese, el resultado devuelto por la función
fgetc debe almacenarse en una variable de tipo
int .
Errores tipográficos
Fragmento 1V547 La expresión 'write_time' siempre es falsa. 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;
El autor de este código debe haber usado accidentalmente el
|| operador en lugar de
&& en la condición. Veamos qué valores pueden tener las variables
write_time y
change_time :
- Ambas variables tienen 0. En este caso, la ejecución pasa a la rama else : la variable mod_time siempre se evaluará a 0 sin importar cuál sea la siguiente condición.
- Una de las variables tiene 0. En este caso, a mod_time se le asignará 0 (dado que la otra variable tiene un valor no negativo) ya que MIN elegirá la menor de las dos.
- Ninguna variable tiene 0: se elige el valor mínimo.
Cambiar esa línea a
write_time && change_time arreglará el comportamiento:
- Solo una o ninguna de las variables tiene 0: se elige el valor distinto de cero.
- Ninguna variable tiene 0: se elige el valor mínimo.
Fragmento 2La expresión
V547 es siempre cierta. Probablemente el operador '&&' debería usarse aquí. 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; .... }
De nuevo, parece ser el problema de usar el operador incorrecto, ya sea
|| en lugar de
&& o
== en lugar de
! = porque la variable no puede almacenar los valores 20 y 9 al mismo tiempo.
Copia ilimitada de cadenas
V512 Una llamada de la función 'sprintf' conducirá al desbordamiento del búfer 'fullpath'. disk.c 1257
RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }
Si pudieras seguir la función hasta el final, verías que el código está bien, pero puede romperse un día: solo un cambio descuidado terminará con un desbordamiento del búfer ya que
sprintf no está limitado de ninguna manera, por lo que es concatenante los caminos podrían llevar la ejecución más allá de los límites de la matriz. Recomendamos reemplazar esta llamada con
snprintf (fullpath, PATH_MAX, ....) .
Condición redundante
V560 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) { .... } }
La verificación
add> 0 no hace ninguna diferencia ya que la variable siempre será mayor que cero porque
leer% 4 devuelve el resto, que nunca será igual a 4.
xrdp
xrdp es un servidor RDP de código abierto. El proyecto consta de dos partes:
- xrdp: la implementación del protocolo. Se lanza bajo Apache 2.0.
- xorgxrdp: una colección de controladores Xorg para usar con xrdp. Se lanza bajo X11 (al igual que MIT, pero el uso en publicidad está prohibido)
El desarrollo se basa en rdesktop y FreeRDP. Inicialmente, para poder trabajar con gráficos, tendría que usar un servidor VNC separado o un servidor X11 especial con soporte RDP, X11rdp, pero estos se volvieron innecesarios con el lanzamiento de xorgxrdp.
No hablaremos de xorgxrdp en este artículo.
Al igual que el proyecto anterior, xrdp es pequeño, y consta de aproximadamente 80 KLOC.
Más errores tipográficos
V525 El código contiene la colección de bloques similares. Verifique 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;
Este código proviene de la biblioteca librfxcodec, que implementa el códec jpeg2000 para trabajar con RemoteFX. El canal de color "rojo" se lee dos veces, mientras que el canal "azul" no se lee en absoluto. Defectos como este generalmente resultan del uso de copiar y pegar.
Se encontró el mismo error en la función similar
rfx_encode_format_argb :
V525 El código contiene la colección de bloques similares. Verifique 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
V557 Array overrun es posible. El valor del índice 'i - 8' podría alcanzar 129. genkeymap.c 142
En el archivo genkeymap.c, la matriz se declara 1 elemento más corto de lo que implica la implementación. Sin embargo, no se producirá ningún error, porque el archivo evdev-map.c almacena el tamaño correcto, por lo que no habrá desbordamiento de la matriz, lo que lo convierte en un defecto menor en lugar de un verdadero error.
Comparación incorrecta
V560 Una parte de la expresión condicional siempre es falsa: (cap_len <0). xrdp_caps.c 616
El valor de una variable de tipo
sin signo corto se lee en una variable de tipo
int y luego se verifica para que sea negativo, lo cual no es necesario porque un valor leído de un tipo sin signo en un tipo más grande nunca puede volverse negativo.
Cheques redundantes
V560 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; } .... }
Las verificaciones no iguales no son necesarias porque la primera verificación hace el trabajo. El programador probablemente iba a usar el
|| operador para filtrar argumentos incorrectos.
Conclusión
La comprobación de hoy no reveló ningún error crítico, pero sí reveló un montón de defectos menores. Dicho esto, estos proyectos, por pequeños que sean, todavía se usan en muchos sistemas y, por lo tanto, necesitan algo de pulido. Un proyecto pequeño no necesariamente debe tener toneladas de errores, por lo que probar el analizador solo en proyectos pequeños no es suficiente para evaluar de manera confiable su efectividad. Este tema se trata con más detalle en el artículo "
Sentimientos confirmados por números ".
La versión demo de PVS-Studio está disponible en nuestro
sitio web .