Esta es la segunda revisión de una serie de artículos sobre la verificación de programas de código abierto para trabajar con el protocolo RDP. En él, consideraremos el cliente rdesktop y el servidor xrdp.
PVS-Studio se utilizó como herramienta para detectar errores. Es un analizador de código estático para C, C ++, C # y Java, disponible en Windows, Linux y macOS.
El artículo presenta solo aquellos errores que me parecieron interesantes. Sin embargo, los proyectos son pequeños, por lo que hubo pocos errores :).
Nota El artículo anterior sobre cómo verificar el proyecto FreeRDP se puede encontrar
aquí .
rdesktop
rdesktop es una implementación gratuita del cliente RDP para sistemas basados en UNIX. También se puede usar en Windows, si crea un 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 viejo: el primer lanzamiento tuvo lugar el 4 de abril de 2001; en el momento de escribir este artículo, su edad es de 17 años.
Como señalé anteriormente, el proyecto es muy pequeño. Contiene alrededor de 30 mil líneas de código, lo cual es un poco extraño dada su antigüedad. A modo de comparación, FreeRDP contiene 320 mil líneas. Aquí está la salida del programa 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 error nos encuentra de inmediato en la función
principal : vemos el código que viene después de la
declaración de devolución : este fragmento limpia la memoria. Sin embargo, el error no representa una amenaza: el sistema operativo limpia toda la memoria asignada después de que finaliza el programa.
Falta de 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 fragmento de código en este caso se lee desde el archivo al búfer hasta que finaliza el archivo. Sin embargo, no hay manejo de errores aquí: si algo sale mal, entonces
leer devolverá -1, y luego la matriz de salida irá más allá de los límites de la matriz.
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++; } .... }
Aquí vemos un procesamiento incorrecto de llegar al final del archivo: si
fgetc devuelve un carácter cuyo código es 0xFF, se interpretará como el final del archivo (
EOF ).
EOF es una constante, generalmente definida como -1. Por ejemplo, al codificar CP1251, la última letra del alfabeto ruso tiene el código 0xFF, que corresponde al número -1, si estamos hablando de una variable de tipo
char . Resulta que el carácter 0xFF, como
EOF (-1), se percibe como el final del archivo. Para evitar tales errores, el resultado de la función
fgetc debe almacenarse en una variable
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;
Quizás el autor de este código confundió
|| y
&& proporcionado. Considere las posibles opciones para
write_time y
change_time :
- Ambas variables son 0: en este caso, llegamos a la rama else : la variable mod_time siempre será 0 independientemente de la siguiente condición.
- Una de las variables es 0: mod_time será 0 (siempre que la otra variable tenga un valor no negativo), porque MIN elegirá la más pequeña de las dos opciones.
- Ambas variables no son iguales a 0: seleccionamos el valor mínimo.
Al reemplazar una condición con
write_time && change_time, el comportamiento se verá correcto:
- Una o ambas variables no son iguales a 0: seleccione un valor distinto de cero.
- Ambas variables no son iguales a 0: seleccionamos 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; .... }
Aparentemente, los operadores
|| y
&& , ya sea
== y
! = : la variable no puede tomar los valores 20 y 9 al mismo tiempo.
Copia de línea ilimitada
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); .... }
Al considerar la función, quedará completamente claro que este código no causa problemas. Sin embargo, pueden surgir en el futuro: un cambio descuidado, y obtenemos un desbordamiento del búfer:
sprintf no está limitado por nada, por lo que al concatenar rutas, podemos ir más allá de los límites de la matriz. Se recomienda notar esta llamada en
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) { .... } }
Comprobar
add> 0 es inútil: la variable siempre será mayor que cero, porque
leer% 4 devolverá el resto de la división, y nunca será 4.
xrdp
xrdp es una implementación de servidor RDP de código 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, tuve que usar un servidor VNC separado para trabajar con gráficos, o un servidor X11 especial con soporte RDP - X11rdp, pero con el advenimiento de xorgxrdp, ya no eran necesarios.
En este artículo, no tocaremos xorgxrdp.
El proyecto xrdp, como el anterior, es muy pequeño y contiene alrededor de 80 mil líneas.
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 fue tomado de la biblioteca librfxcodec que implementa el códec jpeg2000 para que RemoteFX funcione. Aquí, aparentemente, los canales de datos gráficos se mezclan, en lugar de color "azul", se escribe "rojo". Tal error probablemente apareció como resultado de copiar y pegar.
El mismo problema recayó en una función similar
rfx_encode_format_argb , que el analizador también nos contó sobre:
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
La declaración y la definición de la matriz en estos dos archivos es incompatible: 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 forma de salir de los límites. Así que esto es solo una falla que es fácil de solucionar.
Comparación inválida
V560 Una parte de la expresión condicional siempre es falsa: (cap_len <0). xrdp_caps.c 616
En una función, una variable de tipo
unsigned short se lee en una variable de tipo
int . La verificación no es necesaria aquí, porque leemos una variable de un tipo sin signo y asignamos el resultado a una variable más grande, por lo que la variable no puede tomar un valor negativo.
Controles innecesarios
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 comprobaciones 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 el desarrollador quisiera utilizar el
|| para filtrar argumentos inválidos.
Conclusión
Durante la inspección, no se detectaron errores graves, pero se encontraron muchas deficiencias. Sin embargo, estos proyectos se utilizan en muchos sistemas, aunque de pequeño alcance. Un proyecto pequeño no tiene que tener muchos errores, por lo que no debe juzgar el trabajo del analizador solo en proyectos pequeños. Puede leer más sobre esto en el artículo "
Sensaciones, que se confirman con números ".
Puede descargar una versión de prueba de PVS-Studio en nuestro
sitio web .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Sergey Larin.
Comprobación de rdesktop y xrdp con PVS-Studio