Explorando la calidad del código del sistema operativo Zephyr

PVS-Studio y Zephyr

Recientemente dijimos que el analizador de código PVS-Studio comenzó a integrarse con PlatformIO. Naturalmente, el equipo de desarrollo de PVS-Studio se comunicó con el equipo de PlatformIO y sugirieron, por interés, verificar el código del sistema operativo en tiempo real de Zephyr. Por qué no, pensamos, y aquí hay un artículo sobre tal estudio.

PlatformIO


Antes de comenzar la parte principal del artículo, me gustaría recomendar el proyecto PlatformIO a los desarrolladores de sistemas integrados , que pueden facilitarles la vida. Es una herramienta de programación de microcontroladores multiplataforma. El núcleo de PlatformIO es una herramienta de línea de comandos, pero se recomienda usarlo como un complemento para Visual Studio Code. Se admite una gran cantidad de chips modernos y placas base basadas en ellos. Es capaz de descargar automáticamente sistemas de ensamblaje adecuados, y el sitio tiene una gran colección de bibliotecas para administrar componentes electrónicos enchufables.

PVS-Studio


PVS-Studio todavía es poco conocido en el mundo de los sistemas embebidos, por lo que, por si acaso, haré una introducción para los nuevos lectores que aún no están familiarizados con esta herramienta. Nuestros lectores habituales pueden pasar directamente a la siguiente sección.

PVS-Studio es un analizador de código estático que permite detectar errores y vulnerabilidades potenciales en el código de programas escritos en C, C ++, C # y Java. Si hablamos solo de C y C ++, entonces los siguientes compiladores son compatibles:

  • Ventanas Visual Studio 2010-2019 C, C ++, C ++ / CLI, C ++ / CX (WinRT)
  • Ventanas IAR Embedded Workbench, compilador C / C ++ para ARM C, C ++
  • Ventanas QNX Momentics, QCC C, C ++
  • Windows / Linux Keil µVision, DS-MDK, compilador ARM 5/6 C, C ++
  • Windows / Linux Texas Instruments Code Composer Studio, Herramientas de generación de código ARM C, C ++
  • Windows/Linux/macOS. GNU Arm Embedded Toolchain, Arm Embedded GCC compiler, C, C++
  • Windows/Linux/macOS. Clang C, C++
  • Linux/macOS. GCC C, C++
  • Windows. MinGW C, C++

El analizador tiene su propio sistema de clasificación de advertencia , pero si es necesario, puede habilitar la visualización de alertas de acuerdo con los estándares de codificación CWE , SEI CERT , MISRA .

Puede comenzar rápidamente con regularidad utilizando PVS-Studio incluso en un gran proyecto heredado. Para ello se proporciona un mecanismo especial para la supresión masiva de advertencias . Todas las advertencias actuales se consideran deuda técnica y están ocultas, lo que le permite centrarse en las advertencias que se aplican solo a código nuevo o modificado. Esto permite que el equipo comience a usar el analizador a diario de inmediato en su trabajo, y usted puede volver al trabajo técnico de vez en cuando y mejorar el código.

Hay muchos otros escenarios para usar PVS-Studio. Por ejemplo, puede usarlo como complemento de SonarQube. Es posible la integración con sistemas como Travis CI, CircleCI, GitLab CI / CD, etc. Una descripción más detallada de PVS-Studio está más allá del alcance de este artículo. Por lo tanto, propongo familiarizarse con el artículo, que tiene muchos enlaces útiles y que responde muchas preguntas: " Razones para introducir el analizador de código estático PVS-Studio en el proceso de desarrollo ".

Céfiro


Trabajando en la integración de PVS-Studio en PlatformIO , nuestros equipos hablaron y se les pidió que revisaran un proyecto del mundo integrado, a saber, Zephyr. Nos gustó la idea, que fue la razón para escribir este artículo.

Zephyr es un sistema operativo ligero en tiempo real diseñado para trabajar en dispositivos con recursos limitados de varias arquitecturas. El código se distribuye bajo la licencia Apache 2.0 de código abierto. Funciona en las siguientes plataformas: ARM (Cortex-M0, Cortex-M3, Cortex-M4, Cortex-M23, Cortex-M33, Cortex-R4, Cortex-R5, Cortex-A53), x86, x86-64, ARC, RISC- V, Nios II, Xtensa.

Algunas caracteristicas:

  • Espacio de direcciones unificadas. El código de aplicación específico en combinación con el núcleo personalizado crea una imagen monolítica que se ejecuta en el dispositivo.
  • . , .
  • . .
  • . . .
  • : , , , , .

De los momentos interesantes para nosotros, Synopsys está involucrado en el desarrollo del sistema operativo . En 2014, Synopsys adquirió Coverity, que produjo el analizador de código estático del mismo nombre.

Es natural que desde el principio, el desarrollador de Zephyr utilice el analizador Coverity . El analizador es un líder del mercado y esto no podría sino mejorar la calidad del código del sistema operativo.

Calidad del código Zephyr


En mi opinión, el código del sistema operativo Zephyr es de calidad. Esto es lo que me da razones para pensar eso:

  • PVS-Studio 122 High 367 Medium. , , 560 C/C++ . . 7810 C/C++ 10075 . , . , , .
  • . «» , .
  • La utilidad SourceMonitor , después de analizar el código fuente, dio estadísticas de que el 48% del código son comentarios. Esto es mucho y en mi experiencia indica una gran preocupación por la calidad del código, su comprensión para otros desarrolladores.
  • Al desarrollar un proyecto, se utiliza un analizador de código estático Coverity. Probablemente, debido a este hecho, el analizador PVS-Studio, aunque encontró errores en el proyecto, no pudo mostrarse vívidamente, como a veces sucede al analizar otros proyectos.

Basado en esto, creo que los autores del proyecto se preocupan por la calidad y confiabilidad del código. Veamos ahora algunas advertencias emitidas por el analizador PVS-Studio (versión 7.06).

Advertencias semifalsas


El código del proyecto, debido a su bajo nivel, está escrito de manera bastante específica y con mucha compilación condicional (#ifdef). Esto genera una gran cantidad de advertencias que no indican un error real, pero no se pueden llamar simplemente falsas. Será más fácil aclarar esto con algunos ejemplos.

Un ejemplo de una actuación "semi-falsa" N1

static struct char_framebuffer char_fb;

int cfb_framebuffer_invert(struct device *dev)
{
  struct char_framebuffer *fb = &char_fb;

  if (!fb || !fb->buf) {
    return -1;
  }

  fb->inverted = !fb->inverted;

  return 0;
}

Advertencia de PVS-Studio: V560 Una parte de la expresión condicional siempre es falsa :! Fb . cfb.c 188

Al tomar la dirección de una variable estática, siempre se obtiene un puntero distinto de cero. Por lo tanto, el puntero fb siempre es distinto de cero y su verificación no tiene sentido.

Sin embargo, está claro que esto no es un error en absoluto, sino simplemente una verificación excesiva, que no hace daño. Además, al compilar la versión Release, el compilador la descartará, por lo que esto ni siquiera causará una desaceleración.

Un caso similar, en mi opinión, cae bajo el concepto de operación de analizador "semi-falso". Formalmente, el analizador tiene toda la razón. Y es mejor eliminar la verificación inútil adicional del código. Sin embargo, todo esto es insignificante y tales advertencias ni siquiera son interesantes de considerar en el marco del artículo.

Un ejemplo de una actuación "semi-corte" de N2

int hex2char(u8_t x, char *c)
{
  if (x <= 9) {
    *c = x + '0';
  } else if (x >= 10 && x <= 15) {
    *c = x - 10 + 'a';
  } else {
    return -EINVAL;
  }
  return 0;
}

Advertencia PVS-Studio: V560 Una parte de la expresión condicional siempre es verdadera: x> = 10. hex.c 31

El analizador vuelve a tener la razón formal al afirmar que parte de la condición siempre es verdadera. Si la variable x no es menor que / igual a 9, entonces resulta que siempre es mayor que / igual a 10. Y el código se puede simplificar:

} else if (x <= 15) {

Una vez más, está claro que no hay ningún error dañino real aquí, y la comparación adicional está escrita solo por la belleza del código.

Ahora veamos un ejemplo más complejo de N3.

Primero, veamos cómo se puede implementar la macro CHECKIF .

#if defined(CONFIG_ASSERT_ON_ERRORS)
#define CHECKIF(expr) \
  __ASSERT_NO_MSG(!(expr));   \
  if (0)
#elif defined(CONFIG_NO_RUNTIME_CHECKS)
#define CHECKIF(...) \
  if (0)
#else
#define CHECKIF(expr) \
  if (expr)
#endif

Dependiendo del modo de compilación del proyecto, la verificación puede realizarse o saltarse. En nuestro caso, al verificar el código usando PVS-Studio, se seleccionó esta implementación de macro:

#define CHECKIF(expr) \
  if (expr)

Ahora veamos a qué conduce esto.

int k_queue_append_list(struct k_queue *queue, void *head, void *tail)
{
  CHECKIF(head == NULL || tail == NULL) {
    return -EINVAL;
  }

  k_spinlock_key_t key = k_spin_lock(&queue->lock);
  struct k_thread *thread = NULL;
  if (head != NULL) {
    thread = z_unpend_first_thread(&queue->wait_q);
  }
  ....
}

Advertencia de PVS-Studio: V547 [CWE-571] La expresión 'head! = NULL' siempre es verdadera. queue.c 244

El analizador considera que la comprobación (head! = NULL) siempre da verdadero. Y de hecho lo es. Si el puntero de la cabeza fuera NULL, entonces la función dejaría de funcionar debido a una comprobación al comienzo de la función:

CHECKIF(head == NULL || tail == NULL) {
  return -EINVAL;
}

Recuerde que aquí la macro se expande de la siguiente manera:

if (head == NULL || tail == NULL) {
  return -EINVAL;
}

Entonces, el analizador PVS-Studio es correcto desde su punto de vista y emite una advertencia correcta. Sin embargo, esta verificación no se puede eliminar. Ella es necesaria En otro escenario, la macro se abrirá así:

if (0) {
  return -EINVAL;
}

Y luego es necesario volver a verificar el puntero. Por supuesto, el analizador no generará una advertencia en esta versión de la compilación de código. Sin embargo, da una advertencia para la versión de depuración de la compilación.

Espero que ahora esté claro para los lectores de dónde provienen las advertencias "semifalsas". Sin embargo, no hay nada malo con ellos. El analizador PVS-Studio proporciona varios mecanismos para suprimir alertas falsas, que se pueden encontrar en la documentación.

Advertencias de caso


¿Pero encontraste algo interesante después de todo? Fue un éxito, y ahora veremos varios errores. Al mismo tiempo, quiero señalar de inmediato dos puntos:

  1. . : , , , Coverity. , PVS-Studio - , .
  2. . , «» . , . GitHub, PVS-Studio.

N1,

static void gen_prov_ack(struct prov_rx *rx, struct net_buf_simple *buf)
{
  ....
  if (link.tx.cb && link.tx.cb) {
    link.tx.cb(0, link.tx.cb_data);
  }
  ....
}

Advertencia de PVS-Studio: V501 [CWE-571] Hay subexpresiones idénticas a la izquierda y a la derecha del operador '&&': link.tx.cb && link.tx.cb pb_adv.c 377

Uno está doblemente verificado la misma variable link.tx.cb . Aparentemente, este es un error tipográfico, y la segunda variable a verificar debe ser link.tx.cb_data .

Fragmento N2, desbordando el búfer

Considere la función net_hostname_get , que se utilizará más adelante.

#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}
#endif

En mi caso, al preprocesar, se seleccionó la opción relacionada con la rama #else . Es decir, en el archivo preprocesado, la función se implementa de la siguiente manera:
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}

La función devuelve un puntero a una matriz de 7 bytes (tenemos en cuenta el terminal cero al final de la línea).

Ahora considere el código que conduce a la salida del límite de la matriz.

static int do_net_init(void)
{
  ....
  (void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
  ....
}

Advertencia de PVS-Studio: V512 [CWE-119] Una llamada de la función 'memcpy' hará que el búfer 'net_hostname_get ()' quede fuera de rango. log_backend_net.c 114

Después del preprocesamiento, MAX_HOSTNAME_LEN se expande de la siguiente manera:

(void)memcpy(hostname, net_hostname_get(),
    sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));

En consecuencia, al copiar datos, se produce un literal de cadena fuera de los límites. Es difícil predecir cómo esto afectará la ejecución del programa, ya que esto conduce a un comportamiento indefinido.

Fragmento N3, potencial desbordamiento del búfer

int do_write_op_json(struct lwm2m_message *msg)
{
  u8_t value[TOKEN_BUF_LEN];
  u8_t base_name[MAX_RESOURCE_LEN];
  u8_t full_name[MAX_RESOURCE_LEN];
  ....
  /* combine base_name + name */
  snprintf(full_name, TOKEN_BUF_LEN, "%s%s", base_name, value);
  ....
}

Advertencia de PVS-Studio: V512 [CWE-119] Una llamada de la función 'snprintf' provocará el desbordamiento del búfer 'full_name'. lwm2m_rw_json.c 826

Si sustituimos los valores de macro, la imagen de lo que está sucediendo se ve así:

u8_t value[64];
u8_t base_name[20];
u8_t full_name[20];
....
snprintf(full_name, 64, "%s%s", base_name, value);

Solo se asignan 20 bytes bajo el búfer full_name en el que se forma la cadena. En este caso, las partes a partir de las cuales se forma la cadena se almacenan en memorias intermedias de 20 y 64 bytes de tamaño. Además, la constante 64, pasada a la función snprintf y diseñada para evitar que la matriz se vaya al extranjero, ¡obviamente es demasiado grande!

Este código no necesariamente conduce a un desbordamiento del búfer. Quizás siempre tenga suerte, y las subcadenas son siempre muy pequeñas. Sin embargo, en general, este código no está protegido contra el desbordamiento de ninguna manera y contiene el clásico defecto de seguridad CWE-119 .

Fragmento N4, la expresión siempre es verdadera

static int keys_set(const char *name, size_t len_rd, settings_read_cb read_cb,
                    void *cb_arg)
{
  ....
  size_t len;
  ....
  len = read_cb(cb_arg, val, sizeof(val));
  if (len < 0) {
    BT_ERR("Failed to read value (err %zu)", len);
    return -EINVAL;
  }
  ....
}

Advertencia de PVS-Studio: V547 [CWE-570] La expresión 'len <0' siempre es falsa. El valor de tipo sin signo nunca es <0. keys.c 312 La

variable len tiene un tipo sin signo y, por lo tanto, no puede ser inferior a 0. En consecuencia, el estado de error no se procesa de ninguna manera. En otros lugares , el tipo int o ssize_t se usa para almacenar el resultado de la función read_cb . Ejemplo:


static inline int mesh_x_set(....)
{
 ssize_t len;
 len = read_cb(cb_arg, out, read_len);
 if (len < 0) {
 ....
}

Nota. Todo parece estar mal con la función read_cb . El hecho es que se declara así:

static u8_t read_cb(const struct bt_gatt_attr *attr, void *user_data)

El tipo u8_t es un carácter sin signo.

La función siempre devuelve solo números positivos de tipo unsigned char . Si coloca este valor en una variable con signo de tipo int o ssize_t , de todos modos , el valor siempre será positivo. Por lo tanto, en otros lugares, la comprobación del estado de error tampoco funciona. Pero no profundicé en el estudio de este tema.

Fragmento N5, algo muy extraño.

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';
    memcpy(cpy_mntpt, mntpt, strlen(mntpt));
  }
  return cpy_mntpt;
}

Advertencia de PVS-Studio: V575 [CWE-628] La función 'memcpy' no copia toda la cadena. Use la función 'strcpy / strcpy_s' para preservar la terminal nula. shell.c 427

Código extraño

Alguien intentó hacer un análogo de la función strdup , pero no tuvo éxito.

Comencemos con la advertencia del analizador. Informa que la función memcpy copia la línea, pero no copia el terminal cero, y esto es muy sospechoso.

Parece que este terminal 0 se copia aquí:

((u8_t *)mntpt)[strlen(mntpt)] = '\0';

¡Pero no! Aquí hay un error tipográfico, por lo que el terminal cero se copia a sí mismo. Tenga en cuenta que escribir en la matriz mntpt , no cpy_mntpt . Como resultado, la función mntpt_prepare devuelve una cadena que está incompleta con un terminal cero.

De hecho, el programador quería escribir así:

((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';

Sin embargo, todavía no está claro por qué fue tan complicado. Este código se puede simplificar a la siguiente opción:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    strcpy(cpy_mntpt, mntpt);
  }
  return cpy_mntpt;
}

Fragmento N6, desreferenciando un puntero antes de la validación

int bt_mesh_model_publish(struct bt_mesh_model *model)
{
  ....
  struct bt_mesh_model_pub *pub = model->pub;
  ....
  struct bt_mesh_msg_ctx ctx = {
    .send_rel = pub->send_rel,
  };
  ....
  if (!pub) {
    return -ENOTSUP;
  }
  ....
}

Advertencia de PVS-Studio: V595 [CWE-476] El puntero 'pub' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 708, 719. access.c 708

Un patrón de error muy común . Primero, el puntero se desreferencia para inicializar un miembro de la estructura:

.send_rel = pub->send_rel,

Y solo entonces sigue una comprobación de que este puntero puede ser nulo.

Fragmento N7-N9, referencia de puntero antes de la validación

int net_tcp_accept(struct net_context *context, net_tcp_accept_cb_t cb,
                   void *user_data)
{
  ....
  struct tcp *conn = context->tcp;
  ....
  conn->accept_cb = cb;

  if (!conn || conn->state != TCP_LISTEN) {
    return -EINVAL;
  }
  ....
}

Advertencia de PVS-Studio: V595 [CWE-476] El puntero 'conn' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 1071, 1073. tcp2.c 1071

Lo mismo que en el caso anterior. No se requiere explicación aquí.

Aquí se pueden ver dos errores más:

  • V595 [CWE-476] El puntero 'context-> tcp' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 1512, 1518. tcp.c 1512
  • V595 [CWE-476] El puntero 'fsm' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 365, 382. fsm.c 365

Fragmento N10, verificación errónea

static int x509_get_subject_alt_name( unsigned char **p,
                                      const unsigned char *end,
                                      mbedtls_x509_sequence *subject_alt_name)
{
  ....
    while( *p < end )
    {
        if( ( end - *p ) < 1 )
            return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
                    MBEDTLS_ERR_ASN1_OUT_OF_DATA );
    ....
  }
  ....
}

Advertencia de PVS-Studio: V547 [CWE-570] La expresión '(final - * p) <1' ​​siempre es falsa. x509_crt.c 635

Eche un vistazo de cerca a las condiciones:

  • * p <fin
  • (final - * p) <1

Se contradicen entre sí.

Si (* p <fin), entonces (fin - * p) siempre dará un valor de 1 o más. En general, hay algo mal aquí, pero no sé cómo se escribe correctamente.

Fragmento N11, código inalcanzable

uint32_t lv_disp_get_inactive_time(const lv_disp_t * disp)
{
    if(!disp) disp = lv_disp_get_default();
    if(!disp) {
        LV_LOG_WARN("lv_disp_get_inactive_time: no display registered");
        return 0;
    }

    if(disp) return lv_tick_elaps(disp->last_activity_time);

    lv_disp_t * d;
    uint32_t t = UINT32_MAX;
    d          = lv_disp_get_next(NULL);
    while(d) {
        t = LV_MATH_MIN(t, lv_tick_elaps(d->last_activity_time));
        d = lv_disp_get_next(d);
    }

    return t;
}

Advertencia de PVS-Studio: V547 [CWE-571] La expresión 'disp' siempre es verdadera. lv_disp.c 148

La función termina si disp es un puntero nulo. Luego, por el contrario, se verifica que el puntero disp no sea nulo (y este es siempre el caso), y la función nuevamente termina su trabajo.

Como resultado, parte del código en una función nunca tendrá control.

Fragmento N12, valor de retorno extraño

static size_t put_end_tlv(struct lwm2m_output_context *out, u16_t mark_pos,
        u8_t *writer_flags, u8_t writer_flag,
        int tlv_type, int tlv_id)
{
  struct tlv_out_formatter_data *fd;
  struct oma_tlv tlv;
  u32_t len = 0U;

  fd = engine_get_out_user_data(out);
  if (!fd) {
    return 0;
  }

  *writer_flags &= ~writer_flag;

  len = out->out_cpkt->offset - mark_pos;

  /* use stored location */
  fd->mark_pos = mark_pos;

  /* set instance length */
  tlv_setup(&tlv, tlv_type, tlv_id, len);
  len = oma_tlv_put(&tlv, out, NULL, true) - tlv.length;
  return 0;
}

Advertencia de PVS-Studio: V1001 La variable 'len' se asigna pero no se usa al final de la función. lwm2m_rw_oma_tlv.c 338

La función contiene dos declaraciones de retorno que ambas devuelven 0. Es extraño que la función siempre devuelva 0. También es extraño que la variable len ya no se use después de la asignación. Tengo una gran sospecha de que en realidad debería escribirse así:

  len = oma_tlv_put(&tlv, out, NULL, true) - tlv.length;
  return len;
}

Fragmento N13-N16, error de sincronización

static int nvs_startup(struct nvs_fs *fs)
{
  ....
  k_mutex_lock(&fs->nvs_lock, K_FOREVER);
  ....
  if (fs->ate_wra == fs->data_wra && last_ate.len) {
    return -ESPIPE;
  }
  ....
end:
  k_mutex_unlock(&fs->nvs_lock);
  return rc;
}

Advertencia de PVS-Studio: V1020 La función salió sin llamar a la función 'k_mutex_unlock'. Verifique las líneas: 620, 549. nvs.c 620

Hay una situación en la que una función termina su trabajo sin desbloquear el mutex. Según tengo entendido, sería correcto escribir así:

static int nvs_startup(struct nvs_fs *fs)
{
  ....
  k_mutex_lock(&fs->nvs_lock, K_FOREVER);
  ....
  if (fs->ate_wra == fs->data_wra && last_ate.len) {
    rc = -ESPIPE;
    goto end;
  }
  ....
end:
  k_mutex_unlock(&fs->nvs_lock);
  return rc;
}

Tres errores más:

  • V1020 La función salió sin llamar a la función 'k_mutex_unlock'. Líneas de verificación: 574, 549. nvs.c 574
  • V1020 La función salió sin llamar a la función 'k_mutex_unlock'. Líneas de verificación: 908, 890. net_context.c 908
  • V1020 La función salió sin llamar a la función 'k_mutex_unlock'. Líneas de verificación: 1194, 1189. shell.c 1194

Conclusión


Espero que lo hayan disfrutado. Visite nuestro blog para leer sobre cheques en otros proyectos y otras publicaciones interesantes.

Use analizadores estáticos en su trabajo para reducir la cantidad de errores y vulnerabilidades potenciales incluso en la etapa de escritura de código. Especialmente la detección temprana de errores es relevante para sistemas embebidos, la actualización de programas en los que a menudo es un proceso costoso y que lleva mucho tiempo.

También sugiero no posponer e intentar verificar sus proyectos utilizando el analizador PVS-Studio. Vea el artículo: ¿Cómo ver rápidamente advertencias interesantes generadas por el analizador PVS-Studio para código C y C ++?



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Andrey Karpov. Comprobación del código del sistema operativo Zephyr .

All Articles