Verificación de rdesktop y xrdp utilizando el analizador PVS-Studio

Esta es la segunda reseña de una serie de artículos sobre la revisión de programas abiertos para trabajar con el protocolo RDP. En ella, examinaremos el cliente rdesktop y el servidor xrdp.

Como herramienta para detectar errores se utilizó PVS-Studio. Este es un analizador estático de código para lenguajes C, C++, C# y Java, disponible en plataformas Windows, Linux y macOS.

En el artículo se presentan solo los errores que me parecieron interesantes. Sin embargo, los proyectos son pequeños, por lo que también hubo pocos errores :).

Nota. El artículo anterior sobre la revisión del proyecto FreeRDP se puede encontrar aquí.

rdesktop

rdesktop — una implementación libre del cliente RDP para sistemas basados en UNIX. También se puede utilizar en Windows si se compila el proyecto bajo Cygwin. Licenciado bajo GPLv3.

Este cliente tiene una gran popularidad: se utiliza por defecto en ReactOS, además se pueden encontrar front-ends gráficos de terceros para él. Sin embargo, es bastante antiguo: el primer lanzamiento fue el 4 de abril de 2001; al momento de escribir este artículo, tiene 17 años.

Como mencioné anteriormente, el proyecto es bastante pequeño. Contiene aproximadamente 30,000 líneas de código, lo cual es un poco extraño, considerando su edad. Para comparación, FreeRDP contiene 320,000 líneas. Esta es la salida del programa Cloc:

Código inalcanzable

V779 Se detectó código inalcanzable. 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 main: vemos el código que sigue al operador return — este fragmento realiza la limpieza de memoria. Sin embargo, el error no representa una amenaza: toda la memoria asignada será liberada por el sistema operativo después de que finalice el programa.

Falta de manejo de errores

V557 Es posible que se produzca un sub-desbordamiento de matriz. 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] = ' '; // <=
    str_handle_lines(output, &rest, linehandler, data);
  }
  ....
}

El fragmento de código en este caso lee del archivo a un búfer hasta que el archivo se agote. Sin embargo, carece de manejo de errores: si algo falla, read devolverá -1, y entonces ocurrirá un desbordamiento de matriz output.

Uso de EOF en tipo char

V739 EOF no debería ser comparado con un valor del tipo 'char'. El '(c = fgetc(fp))' debería 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 del final del archivo: si fgetc devuelve un carácter cuyo código es 0xFF, será interpretado como el final del archivo (EOF).

EOF esta es una constante que generalmente se define 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 del tipo char. Así, el carácter 0xFF, al igual que EOF (-1) se interpreta como el final del archivo. Para evitar tales errores, el resultado de la función fgetc debe almacenarse en una variable del tipo int.

Errores tipográficos

Fragmento 1

V547 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; <=
  ....
}

Es posible que el autor de este código confundiera || y && en la condición. Consideremos los posibles valores de write_time y change_time:

• Ambas variables son iguales a 0: en este caso, caeremos en la rama else: la variable mod_time siempre será igual a 0 independientemente de la condición posterior.
• Una de las variables es igual a 0: mod_time será igual a 0 (asumiendo que la otra variable tiene un valor no negativo), ya que MIN elegirá el menor de los dos valores.
• Ambas variables no son iguales a 0: elegimos el valor mínimo.

Al cambiar la condición por write_time && change_time el comportamiento se verá correcto:

• Una o ambas variables no son iguales a 0: elegimos un valor no nulo.
• Ambas variables no son iguales a 0: elegimos el valor mínimo.

Fragmento 2

V547 La expresión siempre es verdadera. Probablemente, aquí debería usarse el operador '&&'. 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;
  ....
}

Parece que aquí también se confundieron los operadores || y &&, o == y !=: la variable no puede tomar simultáneamente el valor 20 y 9.

Copia de cadena sin restricciones

V512 Una llamada a la función 'sprintf' llevará a un desbordamiento del búfer 'fullpath'. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Obtener información para la entrada del directorio */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Al considerar la función en su totalidad, queda claro que este código no provoca problemas. Sin embargo, pueden surgir en el futuro: un cambio descuidado y obtendremos un desbordamiento de búfer — sprintf no está restringido, por lo que al concatenar rutas podemos salir de los límites del array. Se recomienda observar esta llamada en snprintf(fullpath, PATH_MAX, ….).

Condición redundante

V560 Una parte de la expresión condicional es siempre verdadera: añadir > 0. scard.c 507

static void
inRepos(STREAM in, unsigned int read)
{
  SERVER_DWORD add = 4 - read % 4;
  if (add  0)
  {
    ....
  }
}

Verificación add > 0 aquí no tiene sentido: la variable siempre será mayor que cero, ya que read % 4 devolverá el residuo de la división, que nunca será igual a 4.

xrdp

xrdp — implementación de un servidor RDP de código abierto. El proyecto se divide en 2 partes:

• xrdp — implementación del protocolo. Se distribuye bajo la licencia Apache 2.0.
• xorgxrdp — conjunto de controladores Xorg para usar con xrdp. Licencia — X11 (como MIT, pero prohíbe el uso en publicidad)

El desarrollo del proyecto se basa en los resultados de rdesktop y FreeRDP. Originalmente, para trabajar con gráficos era necesario usar un servidor VNC independiente o un servidor X11 especial con soporte para RDP — X11rdp, sin embargo, con la llegada de xorgxrdp, esa necesidad ha desaparecido.

En este artículo no abordaremos xorgxrdp.

El proyecto xrdp, al igual que el anterior, es bastante pequeño y contiene aproximadamente 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; // <=
          x++;
      }
      ....
  }
  ....
}

Este código fue tomado de la biblioteca librfxcodec, que implementa el códec jpeg2000 para trabajar con RemoteFX. Aquí, aparentemente, se han confundido los canales de datos gráficos: en lugar de escribir el color «azul», se escribe el «rojo». Este error probablemente surgió como resultado de un copia-pega.

El mismo problema también apareció en una función similar rfx_encode_format_argb, sobre lo cual el analizador también nos informó:

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 Es posible un desbordamiento de la matriz. El valor del índice ‘i — 8’ podría alcanzar 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; la diferencia de tamaño es de 1. Sin embargo, no ocurren errores; en el archivo evdev-map.c se indica el tamaño correcto, por lo tanto, no hay acceso fuera de los límites. Así que esto es solo una omisión que se puede corregir fácilmente.

Comparación incorrecta

V560 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))
  {
    ....
  }
  ....
}

En la función se lee una variable del tipo unsigned short en una variable del tipo intNo es necesaria una verificación aquí, ya que leemos la variable de tipo sin signo y asignamos el resultado a una variable de mayor tamaño, por lo que la variable no puede tomar un valor negativo.

Verificaciones innecesarias

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 de desigualdad aquí no tienen sentido, ya que ya tenemos una comparación al principio. Es bastante probable que esto sea un error tipográfico y que el desarrollador quisiera usar el operador || para filtrar argumentos incorrectos.

Conclusión

Durante la revisión no se encontraron errores graves, pero sí muchos detalles. Sin embargo, estos proyectos se utilizan en muchos sistemas, aunque sean pequeños en su extensión. Un proyecto pequeño no necesariamente debe tener muchos errores, por lo que no se debe juzgar el trabajo del analizador solo por proyectos pequeños. Se puede leer más sobre esto en el artículo "Sensaciones que fueron confirmadas por los números«.

Puede descargar la versión de prueba de PVS-Studio en nuestro sitio el sitio web.

Si desea compartir este artículo con una audiencia de habla inglesa, le pido que use el enlace a la traducción: Sergey Larin. Verificando rdesktop y xrdp con PVS-Studio

Fuente: habr.com