Explorando a qualidade do código do sistema operacional Zephyr

PVS-Studio e Zephyr

Recentemente, dissemos que o analisador de código PVS-Studio começou a se integrar ao PlatformIO. Naturalmente, a equipe de desenvolvimento do PVS-Studio se comunicou com a equipe do PlatformIO e sugeriu, por uma questão de interesse, verificar o código do sistema operacional Zephyr em tempo real. Por que não, pensamos, e aqui está um artigo sobre esse estudo.

PlatformIO


Antes de iniciar a parte principal do artigo, gostaria de recomendar o projeto PlatformIO aos desenvolvedores de sistemas embarcados , o que pode facilitar sua vida. É uma ferramenta de programação de microcontroladores multiplataforma. O núcleo do PlatformIO é uma ferramenta de linha de comando, mas é recomendável usá-lo como um plug-in para o Visual Studio Code. Um grande número de chips e placas-mãe modernos baseados neles são suportados. É capaz de baixar automaticamente sistemas de montagem adequados, e o site possui uma grande coleção de bibliotecas para gerenciar componentes eletrônicos de plug-in.

PVS-Studio


O PVS-Studio ainda é pouco conhecido no mundo dos sistemas embarcados; portanto, caso seja necessário, apresentarei novos leitores que ainda não estão familiarizados com esta ferramenta. Nossos leitores regulares podem pular para a próxima seção.

O PVS-Studio é um analisador de código estático que permite detectar erros e possíveis vulnerabilidades no código de programas escritos em C, C ++, C # e Java. Se falamos apenas de C e C ++, os seguintes compiladores são suportados:

  • janelas Visual Studio 2010-2019 C, C ++, C ++ / CLI, C ++ / CX (WinRT)
  • janelas IAR Embedded Workbench, Compilador C / C ++ para ARM C, C ++
  • janelas Momentics QNX, QCC C, C ++
  • Windows / Linux Keil µVision, DS-MDK, Compilador ARM 5/6 C, C ++
  • Windows / Linux Texas Instruments Code Composer Studio, Ferramentas de geração 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++

O analisador possui seu próprio sistema de classificação de aviso , mas, se necessário, é possível ativar a exibição de alertas de acordo com os padrões de codificação CWE , SEI CERT , MISRA .

Você pode iniciar rapidamente regularmente usando o PVS-Studio, mesmo em um grande projeto legado. Um mecanismo especial para supressão em massa de avisos é fornecido para isso . Todos os avisos atuais são considerados dívida técnica e estão ocultos, o que permite que você se concentre nos avisos que se aplicam apenas ao código novo ou modificado. Isso permite que a equipe comece imediatamente a usar o analisador diariamente em seu trabalho, e você pode voltar ao serviço técnico periodicamente e melhorar o código.

Existem muitos outros cenários para usar o PVS-Studio. Por exemplo, você pode usá-lo como um plugin para o SonarQube. É possível a integração com sistemas como Travis CI, CircleCI, GitLab CI / CD, etc. Uma descrição mais detalhada do PVS-Studio está além do escopo deste artigo. Portanto, proponho familiarizar-me com o artigo, que possui muitos links úteis e responde a muitas perguntas: " Motivos para introduzir o analisador de código estático PVS-Studio no processo de desenvolvimento ".

Zephyr


Trabalhando na integração do PVS-Studio ao PlatformIO , nossas equipes conversaram e pediram para conferir um projeto do mundo incorporado, o Zephyr. Gostamos da ideia, que foi o motivo para escrever este artigo.

Zephyr é um sistema operacional leve em tempo real, projetado para trabalhar em dispositivos com recursos limitados de várias arquiteturas. O código é distribuído sob a licença Apache 2.0 de código aberto. Funciona nas seguintes 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.

Algumas funcionalidades:

  • Espaço de endereço unificado. O código do aplicativo específico em combinação com o kernel personalizado cria uma imagem monolítica que é executada no dispositivo.
  • . , .
  • . .
  • . . .
  • : , , , , .

Dos momentos interessantes para nós, a Synopsys está envolvida no desenvolvimento do sistema operacional . Em 2014, a Synopsys adquiriu o Coverity, que produziu o analisador de código estático com o mesmo nome.

É natural que, desde o início, o desenvolvedor Zephyr use o analisador Coverity . O analisador é líder de mercado e isso não pode deixar de afetar a qualidade do código do sistema operacional para melhor.

Zephyr Code Quality


Na minha opinião, o código do sistema operacional Zephyr é de qualidade. Aqui está o que me dá motivos para pensar assim:

  • PVS-Studio 122 High 367 Medium. , , 560 C/C++ . . 7810 C/C++ 10075 . , . , , .
  • . «» , .
  • O utilitário SourceMonitor , depois de analisar o código fonte, forneceu estatísticas de que 48% do código são comentários. Isso é muito e, na minha experiência, indica uma grande preocupação com a qualidade do código, sua compreensibilidade para outros desenvolvedores.
  • Ao desenvolver um projeto, um analisador de código estático do Coverity é usado. Provavelmente, devido a esse fato, o analisador PVS-Studio, embora tenha encontrado erros no projeto, não pôde se mostrar vividamente, como às vezes acontece ao analisar outros projetos.

Com base nisso, acredito que os autores do projeto se preocupam com a qualidade e a confiabilidade do código. Vejamos agora alguns avisos emitidos pelo analisador PVS-Studio (versão 7.06).

Avisos semi-falsos


O código do projeto, devido ao seu baixo nível, é escrito de maneira bastante específica e com muita compilação condicional (#ifdef). Isso gera um grande número de avisos que não indicam um erro real, mas não podem ser chamados simplesmente de falsos. Será mais fácil esclarecer isso com alguns exemplos.

Um exemplo de uma atuação "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;
}

PVS-Studio Warning: V560 Uma parte da expressão condicional é sempre falsa :! Fb . cfb.c 188

Ao usar o endereço de uma variável estática, um ponteiro diferente de zero é sempre obtido. Portanto, o ponteiro fb é sempre diferente de zero e sua verificação não faz sentido.

No entanto, é claro que isso não é um erro, mas simplesmente uma verificação excessiva, que não causa danos. Além disso, ao compilar a versão Release, o compilador a jogará fora, portanto isso nem causará uma lentidão.

Um caso semelhante, no meu entender, se enquadra no conceito de operação do analisador "semi-falso". Formalmente, o analisador está absolutamente certo. E é melhor remover a verificação extra inútil do código. No entanto, tudo isso é mesquinho e esses avisos nem sequer são interessantes a considerar na estrutura do artigo.

Um exemplo de uma atuação "semi-falsa" 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;
}

Aviso PVS-Studio: V560 Uma parte da expressão condicional é sempre verdadeira: x> = 10. hex.c 31

O analisador está novamente formalmente certo ao afirmar que parte da condição é sempre verdadeira. Se a variável x não for menor que / igual a 9, acontece que é sempre maior que / igual a 10. E o código pode ser simplificado:

} else if (x <= 15) {

Mais uma vez, é claro que não há nenhum erro prejudicial real aqui, e a comparação extra é escrita apenas pela beleza do código.

Agora vamos ver um exemplo mais complexo da N3.

Primeiro, vamos ver como a macro CHECKIF pode ser implementada .

#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

Dependendo do modo de compilação do projeto, a verificação pode ser realizada ou ignorada. No nosso caso, ao verificar o código usando o PVS-Studio, essa implementação de macro foi selecionada:

#define CHECKIF(expr) \
  if (expr)

Agora vamos ver o que isso leva.

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);
  }
  ....
}

Aviso do PVS-Studio: V547 [CWE-571] A expressão 'head! = NULL' é sempre verdadeira. queue.c 244

O analisador considera que a verificação (cabeça! = NULL) sempre dá certo. E de fato é. Se o ponteiro principal fosse NULL, a função deixaria de funcionar devido a uma verificação no início da função:

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

Lembre-se de que aqui a macro se expande da seguinte maneira:

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

Portanto, o analisador PVS-Studio está certo do seu ponto de vista e emite um aviso correto. No entanto, essa verificação não pode ser excluída. Ela é necessária. Em outro cenário, a macro será aberta assim:

if (0) {
  return -EINVAL;
}

E, em seguida, é necessário verificar novamente o ponteiro. Obviamente, o analisador não gerará um aviso nesta versão da compilação de código. No entanto, ele fornece um aviso para a versão de depuração da compilação.

Espero que agora esteja claro para os leitores de onde vêm os avisos "semi-falsos". No entanto, não há nada de errado com eles. O analisador PVS-Studio fornece vários mecanismos para suprimir alertas falsos, que podem ser encontrados na documentação.

Avisos de caso


Mas você achou algo interessante, afinal? Foi um sucesso e agora veremos vários erros. Ao mesmo tempo, quero observar imediatamente dois pontos:

  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);
  }
  ....
}

PVS-Studio Warning: V501 [CWE-571] Existem subexpressões idênticas à esquerda e à direita do operador '&&': link.tx.cb && link.tx.cb pb_adv.c 377

Uma é verificada duas vezes a mesma variável link.tx.cb . Aparentemente, esse é um erro de digitação e a segunda variável a ser verificada deve ser link.tx.cb_data .

Fragmento N2, transbordando o buffer

Considere a função net_hostname_get , que será usada posteriormente.

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

No meu caso, ao pré-processamento, a opção relacionada ao ramo #else foi selecionada . Ou seja, no arquivo pré-processado, a função é implementada assim:
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}

A função retorna um ponteiro para uma matriz de 7 bytes (levamos em consideração o zero do terminal no final da linha).

Agora considere o código que leva à saída do limite da matriz.

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

PVS-Studio Warning: V512 [CWE-119] Uma chamada da função 'memcpy' levará o buffer 'net_hostname_get ()' a ficar fora do intervalo. log_backend_net.c 114

Após o pré-processamento, MAX_HOSTNAME_LEN é expandido da seguinte forma:

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

Dessa forma, ao copiar dados, ocorre uma literal fora dos limites da string. É difícil prever como isso afetará a execução do programa, pois isso leva a um comportamento indefinido.

Fragmento N3, saturação potencial de buffer

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);
  ....
}

PVS-Studio Warning: V512 [CWE-119] Uma chamada da função 'snprintf' levará ao estouro do buffer 'full_name'. lwm2m_rw_json.c 826

Se substituirmos os valores da macro, a imagem do que está acontecendo se parece com isso:

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

Apenas 20 bytes são alocados no buffer full_name no qual a seqüência de caracteres é formada. Nesse caso, as partes das quais a sequência é formada são armazenadas em buffers de 20 e 64 bytes de tamanho. Além disso, a constante 64, passada para a função snprintf e projetada para impedir que o array vá para o exterior, é obviamente grande demais!

Este código não leva necessariamente a um estouro de buffer. Talvez sempre tenha sorte e os substrings sejam sempre muito pequenos. No entanto, em geral, esse código não está protegido contra transbordamento e contém a falha de segurança clássica CWE-119 .

Fragmento N4, a expressão é sempre verdadeira

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

Aviso do PVS-Studio: V547 [CWE-570] A expressão 'len <0' é sempre falsa. O valor do tipo não assinado nunca é <0. keys.c 312 A

variável len possui um tipo não assinado e, portanto, não pode ser menor que 0. Portanto, o status do erro não é processado de forma alguma. Em outros lugares , o tipo int ou ssize_t é usado para armazenar o resultado da função read_cb . Exemplo:


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

Nota. Tudo parece estar ruim com a função read_cb . O fato é que é declarado assim:

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

Digite u8_t é um caractere não assinado.

A função sempre retorna apenas números positivos do tipo char não assinado . Se você colocar esse valor em uma variável assinada do tipo int ou ssize_t , mesmo assim, o valor será sempre positivo. Portanto, em outros lugares, a verificação do status do erro também não funciona. Mas não mergulhei no estudo dessa questão.

Fragmento N5, algo muito estranho

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;
}

PVS-Studio Warning: V575 [CWE-628] A função 'memcpy' não copia toda a string. Use a função 'strcpy / strcpy_s' para preservar o terminal nulo. shell.c 427

Código estranho

Alguém tentou fazer um análogo da função strdup , mas ele não teve sucesso.

Vamos começar com o aviso do analisador. Ele relata que a função memcpy copia a linha, mas não copia o terminal zero, e isso é muito suspeito.

Parece que este terminal 0 é copiado aqui:

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

Mas não! Aqui está um erro de digitação, pelo qual o terminal zero é copiado para si! Observe que a gravação na matriz mntpt , não cpy_mntpt . Como resultado, a função mntpt_prepare retorna uma string incompleta com um terminal zero.

De fato, o programador queria escrever assim:

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

No entanto, ainda não está claro por que era tão complicado! Este código pode ser simplificado para a seguinte opção:

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 um ponteiro antes da validação

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

Aviso do PVS-Studio: V595 [CWE-476] O ponteiro 'pub' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 708, 719. access.c 708

Um padrão de erro muito comum . Primeiro, o ponteiro é desreferenciado para inicializar um membro da estrutura:

.send_rel = pub->send_rel,

E somente então segue uma verificação de que esse ponteiro pode ser nulo.

Fragmento N7-N9, desreferenciamento de ponteiro antes da validação

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

Aviso do PVS-Studio: V595 [CWE-476] O ponteiro 'conn' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 1071, 1073. tcp2.c 1071

O mesmo que no caso anterior. A explicação não é necessária aqui.

Mais dois desses erros podem ser vistos aqui:

  • V595 [CWE-476] O ponteiro 'context-> tcp' foi utilizado antes de ser verificado com relação ao nullptr. Verifique as linhas: 1512, 1518. tcp.c 1512
  • V595 [CWE-476] O ponteiro 'fsm' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 365, 382. fsm.c 365

Fragmento N10, verificação incorreta

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 );
    ....
  }
  ....
}

Aviso do PVS-Studio: V547 [CWE-570] A expressão '(final - * p) <1' ​​é sempre falsa. x509_crt.c 635 Observe

atentamente as condições:

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

Eles se contradizem.

Se (* p <final), então (final - * p) sempre dará um valor de 1 ou mais. Em geral, há algo errado aqui, mas não sei como se escreve corretamente.

Fragmento N11, código inacessível

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;
}

Aviso do PVS-Studio: V547 [CWE-571] A expressão 'disp' é sempre verdadeira. lv_disp.c 148

A função termina se disp for um ponteiro nulo. Então, pelo contrário, é verificado que o ponteiro disp não é nulo (e esse é sempre o caso), e a função termina novamente seu trabalho.

Como resultado, parte do código em uma função nunca terá controle.

Fragmento N12, valor de retorno estranho

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;
}

PVS-Studio Warning: V1001 A variável 'len' é atribuída, mas não é usada no final da função. lwm2m_rw_oma_tlv.c 338

A função contém duas instruções de retorno que retornam 0. É estranho que a função sempre retorne 0. Também é estranho que a variável len não seja mais usada após a atribuição. Eu tenho uma grande suspeita de que realmente deve ser escrito assim:

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

Fragmento N13-N16, erro de sincronização

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;
}

PVS-Studio Warning: V1020 A função saiu sem chamar a função 'k_mutex_unlock'. Verifique as linhas: 620, 549. nvs.c 620

Há uma situação em que uma função termina seu trabalho sem desbloquear o mutex. Pelo que entendi, seria correto escrever assim:

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;
}

Mais três desses erros:

  • V1020 A função saiu sem chamar a função 'k_mutex_unlock'. Verifique as linhas: 574, 549. nvs.c 574
  • V1020 A função saiu sem chamar a função 'k_mutex_unlock'. Verifique as linhas: 908, 890. net_context.c 908
  • V1020 A função saiu sem chamar a função 'k_mutex_unlock'. Verifique as linhas: 1194, 1189. shell.c 1194

Conclusão


Espero que tenha gostado. Visite nosso blog para ler sobre verificações de outros projetos e outras publicações interessantes.

Use analisadores estáticos em seu trabalho para reduzir o número de erros e possíveis vulnerabilidades, mesmo no estágio de escrever o código. Especialmente, a detecção precoce de erros é relevante para sistemas embarcados, atualizando programas nos quais geralmente é um processo demorado e caro.

Também sugiro não adiar e tentar verificar seus projetos usando o analisador PVS-Studio. Veja o artigo: Como ver rapidamente avisos interessantes gerados pelo analisador PVS-Studio para código C e C ++?



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Andrey Karpov. Verificando o código do sistema operacional Zephyr .

All Articles