👷🏻 🛌🏿 👨🏻‍🌾 E, novamente, sobre incorporado: procurando bugs no projeto Embox 🧢 🏛️ 👅

O Embox é um sistema operacional em tempo real, multiplataforma, multiplataforma, para sistemas embarcados. Ele foi projetado para funcionar com poucos recursos de computação e permite executar aplicativos baseados em Linux em microcontroladores sem usar o próprio Linux. Obviamente, como qualquer outro aplicativo, os erros da Embox também não são poupados. Este artigo é dedicado à análise de erros encontrados no código do projeto Embox.

Alguns meses atrás, eu já escrevi um artigo sobre a verificação do FreeRTOS, outro sistema operacional para sistemas embarcados. Eu não encontrei erros nele, mas os encontrei nas bibliotecas adicionadas pelos caras da Amazon ao desenvolver sua própria versão do FreeRTOS.

O artigo que você vê agora à sua frente, em certo sentido, continua o tema do artigo anterior. Frequentemente recebíamos pedidos para verificar o FreeRTOS e o fizemos. Dessa vez, não houve pedidos para verificar um projeto específico, mas comecei a receber cartas e comentários de desenvolvedores incorporados que gostavam e queriam mais.

Bem, a nova edição da revista PVS-Studio for Embedded amadureceu e está à sua frente. Gostam de assistir!

Análise

A análise foi realizada no PVS-Studio - um analisador de código estático para C, C ++, C # e Java. Antes da análise, o projeto precisa ser montado - para garantir que o código do projeto esteja funcionando e também daremos ao analisador a oportunidade de coletar as informações da montagem que podem ser úteis para uma melhor verificação do código.

As instruções no repositório oficial da Embox oferecem a capacidade de compilar em diferentes sistemas (Arch Linux, macOS, Debian) e usar o Docker. Decidi adicionar um pouco de variedade à minha vida e criar e analisar no Debian, que eu instalei recentemente na minha máquina virtual.

A assembléia correu bem. Agora era necessário realizar uma análise. O Debian é um dos sistemas suportados pelo PVS-Studio Linux. Uma maneira conveniente de verificar projetos no Linux é compilar o rastreamento. Esse é um modo especial no qual o analisador coleta todas as informações necessárias sobre a montagem para que você possa iniciar a análise com um clique. Tudo o que eu precisava fazer era:

1) Baixe e instale o PVS-Studio;

2) Inicie o rastreamento de montagem indo para a pasta com a Embox e digitando no terminal

pvs-studio-analyzer analyze -- make

3) Após aguardar a montagem, execute o comando:

pvs-studio-analyzer analyze -o /path/to/output.log

4) Converta o relatório bruto para qualquer formato conveniente para você. O analisador vem com um utilitário especial PlogConverter, com o qual você pode fazer isso. Por exemplo, o comando para converter o relatório em lista de tarefas (para exibição, por exemplo, no QtCreator) terá a seguinte aparência:

plog-converter -t tasklist -o /path/to/output.tasks /path/to/project

Todos! Não demorei mais de 15 minutos para concluir esses pontos. O relatório está pronto, agora você pode visualizar os erros. Bem, vamos começar :)

Ciclo estranho

Um dos erros encontrados pelo analisador foi o estranho loop while:

int main(int argc, char **argv) {
  ....

  while (dp.skip != 0 ) {
    n_read = read(ifd, tbuf, dp.bs);
    if (n_read < 0) {
      err = -errno;
      goto out_cmd;
    }
    if (n_read == 0) {
      goto out_cmd;
    }

    dp.skip --;
  } while (dp.skip != 0);       // <=

  do {
    n_read = read(ifd, tbuf, dp.bs);
    if (n_read < 0) {
      err = -errno;
      break;
    }

    if (n_read == 0) {
      break;
    }

    ....

    dp.count --;
  } while (dp.count != 0);
  ....
}

Aviso do PVS-Studio: V715 O operador 'while' possui o corpo vazio. Padrão suspeito detectado: 'while (expr) {...} while (dp.skip! = 0);'. dd.c 225 hm

. Loop realmente estranho. A expressão while (dp.skip! = 0) é escrita duas vezes, uma logo acima do loop e a segunda logo abaixo. De fato, agora existem dois ciclos diferentes: um contém expressões entre chaves e o segundo está vazio. Nesse caso, o segundo ciclo nunca será executado.

Abaixo está um loop do ... while com uma condição semelhante, o que me leva a pensar: o loop estranho foi originalmente concebido como o ... while, mas algo deu errado. Eu acho que esse pedaço de código com uma alta probabilidade contém um erro lógico.

Perdas de memória

Sim, não sem eles.

int krename(const char *oldpath, const char *newpath) {
  
  char *newpatharg, *oldpatharg;

  ....

  oldpatharg =
    calloc(strlen(oldpath) + diritemlen + 2, sizeof(char));
  newpatharg =
    calloc(strlen(newpath) + diritemlen + 2, sizeof(char));
  if (NULL == oldpatharg || NULL == newpatharg) {
    SET_ERRNO(ENOMEM);
    return -1;
  }

  ....
}

Avisos do PVS-Studio:

V773 The function was exited without releasing the 'newpatharg' pointer. A memory leak is possible. kfsop.c 611
V773 The function was exited without releasing the 'oldpatharg' pointer. A memory leak is possible. kfsop.c 611

A função cria as variáveis locais newpatharg e oldpatharg dentro de si . Esses ponteiros recebem os endereços dos novos locais de memória alocados internamente usando calloc . Se ocorrer um problema ao alocar memória, o calloc retornará um ponteiro nulo.

E se apenas um bloco de memória for alocado? A função trava sem que nenhuma memória seja liberada. O site que foi alocado permanecerá na memória sem qualquer oportunidade de acessá-lo novamente e liberá-lo para uso posterior.

Outro exemplo de vazamento de memória é um pouco mais claro:

static int block_dev_test(....) {
  int8_t *read_buf, *write_buf;
  
  ....

  read_buf = malloc(blk_sz * m_blocks);
  write_buf = malloc(blk_sz * m_blocks);

  if (read_buf == NULL || write_buf == NULL) {
    printf("Failed to allocate memory for buffer!\n");

    if (read_buf != NULL) {
      free(read_buf);
    }

    if (write_buf != NULL) {
      free(write_buf);
    }

    return -ENOMEM;
  }

  if (s_block >= blocks) {
    printf("Starting block should be less than number of blocks\n");
    return -EINVAL;            // <=
  }

  ....
}

Avisos do PVS-Studio:

V773 The function was exited without releasing the 'read_buf' pointer. A memory leak is possible. block_dev_test.c 195
V773 The function was exited without releasing the 'write_buf' pointer. A memory leak is possible. block_dev_test.c 195

Aqui, o programador já mostrou precisão e processou corretamente o caso em que foi possível alocar apenas uma parte da memória. Processado corretamente ... e literalmente na próxima expressão cometeu outro erro.

Graças a uma verificação escrita corretamente, podemos ter certeza de que, no momento em que a expressão é executada, retorne -EINVAL; nós definitivamente teremos memória alocada para read_buf e write_buf . Assim, com esse retorno da função, teremos dois vazamentos ao mesmo tempo.

Eu acho que conseguir um vazamento de memória em um dispositivo incorporado pode ser mais doloroso do que em um PC clássico. Em condições em que os recursos são severamente limitados, é necessário monitorá-los com cuidado.

Manuseio incorreto de ponteiros

O seguinte código de erro é conciso e simples o suficiente:

static int scsi_write(struct block_dev *bdev, char *buffer,
                      size_t count, blkno_t blkno) {
  struct scsi_dev *sdev;
  int blksize;

  ....

  sdev = bdev->privdata;
  blksize = sdev->blk_size; // <=

  if (!sdev) {              // <=
    return -ENODEV;
  }

  ....
}

PVS-Studio Warning: V595 O ponteiro 'sdev' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 116, 118. scsi_disk.c 116

O ponteiro sdev é desreferenciado logo antes de ser verificado como NULL. É lógico supor que, se alguém escreveu essa verificação, esse ponteiro pode ser nulo. Nesse caso, temos a possível desreferenciação do ponteiro nulo na string blksize = sdev-> blk_size .

O erro é que a verificação não está localizada onde é necessária. Deveria ter vindo após a linha " sdev = bdev-> privdata; ", mas antes da linha " blksize = sdev-> blk_size; ". Em seguida, o apelo potencial ao endereço zero poderia ser evitado.

O PVS-Studio encontrou mais dois erros no código a seguir:

void xdrrec_create(....)
{
  char *buff;

  ....

  buff = (char *)malloc(sendsz + recvsz);
  assert(buff != NULL);

  ....

  xs->extra.rec.in_base = xs->extra.rec.in_curr = buff;
  xs->extra.rec.in_boundry 
    = xs->extra.rec.in_base + recvsz;                    // <=

  ....
  xs->extra.rec.out_base
    = xs->extra.rec.out_hdr = buff + recvsz;             // <= 
  xs->extra.rec.out_curr 
    = xs->extra.rec.out_hdr + sizeof(union xdrrec_hdr);

  ....
}

Avisos do PVS-Studio:

V769 O ponteiro 'xs-> extra.rec.in_base' na expressão 'xs-> extra.rec.in_base + recvsz' pode ser nullptr. Nesse caso, o valor resultante será insensato e não deve ser usado. Verifique as linhas: 56, 48. xdr_rec.c 56
V769 O ponteiro 'buff' na expressão 'buff + recvsz' pode ser nullptr. Nesse caso, o valor resultante será insensato e não deve ser usado. Verifique as linhas: 61, 48. xdr_rec.c 61

O ponteiro buf é inicializado com malloc e, em seguida, seu valor é usado para inicializar outros ponteiros. A função malloc pode retornar um ponteiro nulo, e isso sempre deve ser verificado. Parece que aqui está uma declaração de verificação de buf para NULL , e tudo deve funcionar bem.

Mas não! O fato é que as afirmações são usadas para depuração e, ao criar o projeto na configuração do Release, essa afirmação será excluída. Acontece que, ao trabalhar no Debug, o programa funcionará corretamente e, ao criar no Release, o ponteiro nulo "avança".

Use nullnas operações aritméticas está incorreto, porque o resultado dessa operação não fará sentido e esse resultado não pode ser usado. É sobre isso que o analisador nos alerta.

Alguns podem argumentar que não verificar o ponteiro após malloc / realloc / calloc é destemido. Como, no primeiro acesso por um ponteiro nulo, um sinal / exceção ocorrerá e não haverá nada errado. Na prática, tudo é muito mais complicado e, se a falta de verificação não parecer perigosa para você, sugiro que você dê uma olhada no artigo “ Por que é importante verificar o que a função malloc retornou ”?

Manuseio incorreto de matrizes

O erro a seguir é muito semelhante ao exemplo anterior ao último:


int fat_read_filename(struct fat_file_info *fi,
                      void *p_scratch,
                      char *name) {
  int offt = 1;

  ....

  offt = strlen(name);
  while (name[offt - 1] == ' ' && offt > 0) { // <=
    name[--offt] = '\0';
  }
  log_debug("name(%s)", name);

  return DFS_OK;
}

Aviso do PVS-Studio: V781 O valor do índice 'offt' é verificado após ser utilizado. Talvez haja um erro na lógica do programa. fat_common.c 1813 A

variável offt é usada pela primeira vez dentro da operação de indexação e somente então é verificado que seu valor é maior que zero. Mas o que acontece se o nome for uma string vazia? A função strlen () retornará 0 e , em seguida, um tiro alto na perna. O programa acessará em um índice negativo, o que levará a um comportamento indefinido. Tudo pode acontecer, incluindo uma falha no programa. Bagunça!

Condições suspeitas

E onde sem eles? Encontramos esses erros literalmente em todos os projetos que verificamos.

int index_descriptor_cloexec_set(int fd, int cloexec) {
  struct idesc_table *it;

  it = task_resource_idesc_table(task_self());
  assert(it);

  if (cloexec | FD_CLOEXEC) {
    idesc_cloexec_set(it->idesc_table[fd]);
  } else {
    idesc_cloexec_clear(it->idesc_table[fd]);
  }
  return 0;
}

Aviso do PVS-Studio: V617 Considere inspecionar a condição. O argumento '0x0010' do '|' operação bit a bit contém um valor diferente de zero. index_descriptor.c 55

Para entender em que consiste o erro, consulte a definição da constante FD_CLOEXEC :

#define FD_CLOEXEC 0x0010

Acontece que na expressão if (cloexec | FD_CLOEXEC) à direita do bit a bit “ou” sempre existe uma constante diferente de zero. O resultado dessa operação sempre será um número diferente de zero. Portanto, essa expressão sempre será equivalente à expressão if (true) e sempre processaremos apenas o ramo então da expressão if.

Eu suspeito que essa constante macro seja usada para pré-configurar o sistema operacional Embox, mas mesmo assim essa condição sempre verdadeira parece estranha. Talvez eles quisessem usar o operador & aqui , mas cometeram um erro de digitação.

Divisão inteira

O erro a seguir está relacionado a um recurso do idioma C:

#define SBSIZE    1024

static int ext2fs_format(struct block_dev *bdev, void *priv) {
  size_t dev_bsize;
  float dev_factor;

  ....

  dev_size = block_dev_size(bdev);
  dev_bsize = block_dev_block_size(bdev);
  dev_factor = SBSIZE / dev_bsize;            // <=

  ext2_dflt_sb(&sb, dev_size, dev_factor);
  ext2_dflt_gd(&sb, &gd);

  ....
}

Aviso do PVS-Studio: V636 A expressão '1024 / dev_bsize' foi convertida implicitamente do tipo 'int' para o tipo 'float'. Considere utilizar uma conversão de tipo explícita para evitar a perda de uma parte fracionária. Um exemplo: double A = (double) (X) / Y; ext2.c 777

Esse recurso é o seguinte: se dividirmos dois valores inteiros, o resultado da divisão será inteiro. Assim, a divisão ocorrerá sem restos ou, em outras palavras, a parte fracionária será descartada do resultado da divisão.

Às vezes, os programadores esquecem e erros como esse são cometidos. A constante SBSIZE e a variável dev_bsize têm um tipo inteiro (int e size_t respectivamente). Portanto, o resultado da expressão SBSIZE / dev_bsizetambém será do tipo inteiro.

Mas espere um momento. A variável dev_factor é do tipo float ! Obviamente, o programador esperava obter um resultado de divisão fracionária. Isso pode ser verificado ainda mais se você prestar atenção ao uso adicional dessa variável. Por exemplo, a função ext2_dflt_sb , em que dev_factor é passada como o terceiro parâmetro, possui a seguinte assinatura:

static void ext2_dflt_sb(struct ext2sb *sb, size_t dev_size, float dev_factor);

Da mesma forma, em outros lugares onde a variável dev_factor é usada : tudo indica que é esperado um número de ponto flutuante.

Para corrigir esse erro, basta converter um dos operandos de divisão em um tipo real. Por exemplo:

dev_factor = float(SBSIZE) / dev_bsize;

Então o resultado da divisão será um número fracionário.

Entrada não verificada

O erro a seguir está associado ao uso de dados não verificados recebidos de fora do programa.

int main(int argc, char **argv) {
  int ret;
  char text[SMTP_TEXT_LEN + 1];

  ....

  if (NULL == fgets(&text[0], sizeof text - 2, /* for \r\n */
      stdin)) { ret = -EIO; goto error; }
    text[strlen(&text[0]) - 1] = '\0'; /* remove \n */    // <=

  ....
}

PVS-Studio Warning: V1010 Dados contaminados não verificados são usados no índice: 'strlen (& text [0])'. sendmail.c 102

Primeiro, considere o que a função fgets retorna . No caso de uma leitura bem-sucedida de uma linha, a função retorna um ponteiro para essa linha. Se a leitura encontrar um final de arquivo antes de pelo menos um elemento ser lido, ou se ocorrer um erro de entrada, a função fgets retornará NULL .

Portanto, a expressão é NULL == fgets (....)verifica se a entrada recebida está correta. Mas há uma ressalva. Se você transferir um terminal nulo como o primeiro caractere a ser lido (isso pode ser feito, por exemplo, pressionando Ctrl + 2 no modo Legado da linha de comando do Windows), a função fgets o considera sem retornar NULL . Nesse caso, a linha na qual a gravação é feita terá apenas um elemento - ' \ 0 '.

O que vai acontecer à seguir? A expressão strlen (& text [0]) retornará 0 . Como resultado, recebemos uma chamada de índice negativa:

text[ 0 - 1 ] = '\0';

Como resultado, podemos "derrubar" o programa simplesmente passando o caractere de terminação da linha para a entrada. Isso é constrangedor e pode potencialmente ser usado para atacar sistemas usando o Embox.

Meu colega, que estava desenvolvendo essa regra de diagnóstico, até fez uma anotação com um exemplo de um ataque desse tipo ao projeto NcFTP:

Eu recomendo ver se você ainda não acredita nessa oportunidade :)

Além disso, o analisador encontrou mais dois lugares com o mesmo erro:

V1010 Dados contaminados não verificados são usados no índice: 'strlen (& from [0])'. sendmail.c 55
V1010 Dados contaminados não verificados são usados no índice: 'strlen (& a [0])'. sendmail.c 65

Misra

MISRA é um conjunto de diretrizes e diretrizes para escrever código C e C ++ seguro para sistemas embarcados altamente responsáveis. De certa forma, este é um manual de treinamento, que permitirá que você se livre não apenas dos chamados "odores de código", mas também proteja seu programa contra vulnerabilidades.

O MISRA é usado onde as vidas humanas dependem da qualidade do seu sistema embarcado: nas indústrias médica, automotiva, aeronáutica e militar.

O PVS-Studio possui um extenso conjunto de regras de diagnóstico que permitem verificar seu código quanto à conformidade com os padrões MISRA C e MISRA C ++. Por padrão, o modo com esses diagnósticos está desativado, mas como estamos procurando erros no projeto de sistemas embarcados, simplesmente não poderia ficar sem o MISRA.

Aqui está o que eu consegui encontrar:

/* find and read symlink file */
static int ext2_read_symlink(struct nas *nas,
                             uint32_t parent_inumber,
                             const char **cp) {
  char namebuf[MAXPATHLEN + 1];

  ....

  *cp = namebuf;              // <=
  if (*namebuf != '/') {
    inumber = parent_inumber;
  } else {
    inumber = (uint32_t) EXT2_ROOTINO;
  }
  rc = ext2_read_inode(nas, inumber);

  return rc;
}

PVS-Studio Warning: V2548 [MISRA C 18.6] O endereço da matriz local 'namebuf' não deve ser armazenado fora do escopo desta matriz. ext2.c 298

O analisador detectou uma atribuição suspeita que poderia levar a um comportamento indefinido.

Vamos dar uma olhada no código. Aqui, namebuf é uma matriz criada no escopo local da função e o ponteiro cp é passado para a função pelo ponteiro.

De acordo com a sintaxe C, o nome da matriz é um ponteiro para o primeiro elemento na área de memória na qual a matriz está armazenada. Acontece que a expressão * cp = namebuf atribuirá o endereço da matriz namebuf à variável apontada por cp. Como cp é passado para a função pelo ponteiro, uma alteração no valor para o qual ela aponta será refletida no local em que a função foi chamada.

Acontece que após a função ext2_read_symlink terminar seu trabalho, seu terceiro parâmetro indicará a área que a matriz namebuf ocupou uma vez .

Há apenas um "mas": como namebuf é uma matriz reservada na pilha, ela será excluída quando a função sair. Assim, um ponteiro que exista fora da função apontará para a memória liberada.

Qual será esse endereço? É impossível prever. É possível que por algum tempo o conteúdo da matriz continue na memória, ou é possível que o programa apague imediatamente essa área com outra coisa. Em geral, o acesso a um endereço retornará um valor indefinido e o uso desse valor é um erro grave.

O analisador também encontrou outro erro com o mesmo aviso:

V2548 [MISRA C 18.6] O endereço da variável local 'dst_haddr' não deve ser armazenado fora do escopo desta variável. net_tx.c 82

Conclusão

Gostei de trabalhar com o projeto Embox. Apesar de não ter anotado todos os erros encontrados no artigo, o número total de avisos foi relativamente pequeno e, em geral, o código do projeto foi escrito com alta qualidade. Portanto, expresso minha gratidão aos desenvolvedores, bem como àqueles que contribuíram para o projeto em nome da comunidade. Você é ótimo!

Aproveito esta oportunidade para transmitir saudações aos desenvolvedores de Tula. Eu acredito que em São Petersburgo não está muito frio agora :)

É aqui que o meu artigo chega ao fim. Espero que tenham gostado de ler e tenham encontrado algo novo para si.

Se você está interessado no PVS-Studio e gostaria de verificar independentemente algum projeto usando-o, faça o download e experimente . Isso não levará mais que 15 minutos.

Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: George Gribkov. Sobre incorporado novamente: pesquisando bugs no projeto Embox .

E, novamente, sobre incorporado: procurando bugs no projeto Embox

Análise

Ciclo estranho

Perdas de memória

Manuseio incorreto de ponteiros

Manuseio incorreto de matrizes

Condições suspeitas

Divisão inteira

Entrada não verificada

Misra

Conclusão

More articles: