0 AD é um jogo tridimensional do gênero de estratégia histórica em tempo real, desenvolvido pela comunidade de voluntários. O tamanho da base de código é pequeno e eu decidi verificar o jogo como uma pausa de grandes projetos como Android e XNU Kernel. Portanto, diante de nós, há um projeto que contém 165.000 linhas de código em C ++. Vamos ver o que é interessante, que pode ser encontrado usando o analisador estático PVS-Studio.
0 AD
0 AD (
0 AD ) é um jogo tridimensional gratuito no gênero de estratégia histórica em tempo real, desenvolvido por uma comunidade de voluntários (os principais desenvolvedores estão unidos na equipe dos Jogos Wildfire). O jogo permite controlar civilizações que existiam no período de 500 aC. e. - 1 ano aC. e A partir do verão de 2018, o projeto está na versão alfa. [
Descrição retirada da Wikipedia].
Por que exatamente 0 AD?
Pedi a um colega de
Egor Bredikhin (
EgorBredikhin ) que escolhesse e verificasse para mim algum pequeno projeto aberto com o qual eu pudesse trabalhar entre outras tarefas. Ele me enviou um log para o projeto do AD 0. À pergunta: "Por que esse projeto?" - houve uma resposta: "Sim, acabei de jogar este jogo, uma boa estratégia em tempo real". Ok, então será 0 AD :).
Densidade de erro
Quero elogiar os autores do 0 AD pela boa qualidade do código C ++. Bem feito, você raramente consegue encontrar uma densidade tão baixa de erros. Obviamente, esses nem todos são erros, mas aqueles que podem ser detectados usando o PVS-Studio. Como eu disse, embora o PVS-Studio não encontre todos os erros, no entanto, podemos falar com segurança sobre a relação entre a densidade dos erros e a qualidade do código como um todo.
Alguns números. O número total de linhas de código não vazias é 231270. Desses, 28,7% são comentários. No total, 165.000 linhas de código C ++ puro.
O número de mensagens emitidas pelo analisador era pequeno e, tendo analisado todas elas, escrevi 19 erros. Vou considerar todos esses erros posteriormente neste artigo. Talvez eu tenha perdido alguma coisa, considerando o erro um código desleixado e inofensivo. No entanto, em geral, isso não muda a imagem.
Então, encontrei 19 erros em 165.000 linhas de código. Calculamos a densidade do erro: 19 * 1000/165000 = 0,111.
Por uma questão de simplicidade, concluímos e assumimos que o analisador PVS-Studio detecta 0,1 erros por 1000 linhas de código no código do jogo.
Ótimo resultado! Para comparação, em meu recente
artigo sobre Android, calculei que havia detectado pelo menos 0,25 erros por 1.000 linhas de código. De fato, a densidade de erros é ainda maior, só não encontrei forças para analisar todo o relatório com cuidado.
Ou use as Bibliotecas principais do EFL como exemplo, que
estudei cuidadosamente e calculei o número de defeitos. Nele, o PVS-Studio detecta 0,71 erros por 1000 linhas de código.
Portanto, os autores de 0 AD são bons companheiros, no entanto, por uma questão de justiça, deve-se notar que uma pequena quantidade de código escrito em C ++ os ajuda. Infelizmente, quanto maior o projeto, mais rápida sua complexidade aumenta e a densidade de erros aumenta de maneira não linear (
mais ).
Erros
Vejamos agora os 19 erros que encontrei no jogo. Para analisar o projeto, usei o
PVS-Studio versão 6.24. Eu sugiro que você tente baixar a versão demo e verificar os projetos em que está trabalhando.
Nota Posicionamos o PVS-Studio como uma solução B2B. Para pequenos projetos e desenvolvedores individuais, temos uma opção de licença gratuita: "
Como usar o PVS-Studio gratuitamente ".
Erro N1Vamos começar analisando um erro complexo. Na verdade, não é complicado, mas você terá que se familiarizar com um pedaço de código bastante grande.
void WaterManager::CreateWaveMeshes() { .... int nbNeighb = 0; .... bool found = false; nbNeighb = 0; for (int p = 0; p < 8; ++p) { if (CoastalPointsSet.count(xx+around[p][0] + (yy + around[p][1])*SideSize)) { if (nbNeighb >= 2) { CoastalPointsSet.erase(xx + yy*SideSize); continue; } ++nbNeighb;
Aviso do PVS-Studio: A expressão
V547 CWE-570 'nbNeighb> = 2' é sempre falsa. WaterManager.cpp 581
À primeira vista, a mensagem do analisador parece estranha. Por que a condição
nbNeighb> = 2 sempre é falsa? De fato, no corpo do loop, há um incremento da variável
nbNeighb !
Veja abaixo e você verá uma declaração de interrupção que interrompe a execução do loop. Portanto, se a variável
nbNeighb for aumentada, o ciclo será interrompido. Portanto, o valor da variável
nbNeighb nunca alcançará um valor maior que 1.
O código contém claramente algum tipo de erro lógico.
Erro N2 void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments) { .... segments.erase(segments.end()); .... }
PVS-Studio Warning:
V783 CWE-119 A desreferenciação do iterador inválido 'segmentos.end ()' pode ocorrer. CCmpRallyPointRenderer.cpp 1290
Código muito, muito estranho. Talvez o programador desejasse remover o último elemento do contêiner. Nesse caso, o código deve ser assim:
segments.erase(segments.end() - 1);
Embora, seria mais fácil escrever:
segments.pop_back();
Para ser sincero, não está totalmente claro para mim o que exatamente deveria ter sido escrito aqui.
Erro N3, N4Decidi considerar dois erros juntos, pois eles estão relacionados a um vazamento de recurso e primeiro preciso mostrar o que é a macro
WARN_RETURN .
#define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0)
Portanto, como você pode ver, a macro
WARN_RETURN faz com que a função saia do corpo. Agora vamos ver maneiras imprecisas de usar essa macro.
O primeiro fragmento.
Status sys_generate_random_bytes(u8* buf, size_t count) { FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK; }
Aviso do PVS-Studio:
V773 CWE-401 A função foi encerrada sem liberar o identificador 'f'. Um vazamento de recurso é possível. unix.cpp 332
Se a função
fread não puder ler os dados, a função
sys_generate_random_bytes sairá sem liberar o descritor de arquivo. Na prática, isso é quase impossível. É duvidoso que você não consiga ler dados de "/ dev / urandom". No entanto, o código é desleixado.
O segundo fragmento.
Status sys_cursor_create(....) { .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK; }
Aviso do PVS-Studio: V773 CWE-401 A função foi encerrada sem liberar o ponteiro 'impl'. É possível um vazamento de memória. x.cpp 421
Se o cursor falhar ao carregar, ocorre um vazamento de memória.
Erro N5 Status LoadHeightmapImageOs(....) { .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); .... }
Aviso do PVS-Studio:
V554 CWE-762 Uso incorreto de shared_ptr. A memória alocada com 'new []' será limpa usando 'delete'. MapIO.cpp 54
A opção correta:
shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]);
Erro N6 FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; }
PVS-Studio Warning:
V570 A variável 'ptr' é atribuída a si mesma. FUTracker.h 122
Erro N7, N8 std::wstring TraceEntry::EncodeAsText() const { const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf; }
Aviso do PVS-Studio:
V576 CWE-628 Formato incorreto. Considere verificar o quinto argumento real da função 'swprintf_s'. O argumento do tipo char é esperado. trace.cpp 93
Aqui encontramos um histórico confuso e indistinto de uma implementação alternativa da função
swprintf no Visual C ++. Não vou
recontá- lo, mas consulte a documentação para o diagnóstico do
V576 (consulte a seção "Linhas largas").
Nesse caso, provavelmente, esse código funcionará corretamente ao compilar no Visual C ++ para Windows e incorretamente ao criar para Linux ou macOS.
Erro semelhante: V576 CWE-628 Formato incorreto. Considere verificar o quarto argumento real da função 'swprintf_s'. O argumento do tipo char é esperado. vfs_tree.cpp 211
Erro N9, N10, N11Clássico No início, o ponteiro é usado e só depois verificado.
static void TEST_CAT2(char* dst, ....) { strcpy(dst, dst_val);
Aviso do PVS-Studio:
V595 CWE-476 O ponteiro 'dst' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 140, 143. test_secure_crt.h 140
Eu acho que o erro não requer explicação. Avisos semelhantes:
- V595 CWE-476 O ponteiro 'dst' foi utilizado antes de ser verificado com relação ao nullptr. Verifique as linhas: 150, 153. test_secure_crt.h 150
- V595 CWE-476 O ponteiro 'dst' foi utilizado antes de ser verificado com relação ao nullptr. Verifique as linhas: 314, 317. test_secure_crt.h 314
Erro N12 typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... }
V674 CWE-682 O literal '0.5' do tipo 'double' é comparado com um valor do tipo 'int'. Considere inspecionar a expressão 'bIsOrientationPreserving> 0.5'. MikktspaceWrap.cpp 137
Não faz sentido comparar uma variável do tipo
int com uma constante de 0,5. Além disso, em termos de significado, essa geralmente é uma variável do tipo booleano, o que significa que compará-la com 0,5 parece muito estranha. Suponha que, em vez de
bIsOrientationPreserving, outra variável deva ser usada aqui.
Erro N13 virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size) { ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE);
PVS-Studio Warning:
V749 CWE-675 O destruidor do objeto 's' será invocado uma segunda vez após sair do escopo do objeto. vfs.cpp 165
Antes de criar um arquivo, é necessário que um objeto do tipo
ScopedLock "desbloqueie" um objeto. Para isso, um destruidor é chamado explicitamente. O problema é que o destruidor do objeto
s será chamado automaticamente novamente quando a função sair. I.e. o destruidor será chamado duas vezes. Não estudei o dispositivo da classe
ScopedLock , mas, de qualquer forma, você não deve fazer isso. Freqüentemente, uma chamada dupla para o destruidor leva a um comportamento indefinido ou outros erros desagradáveis. Mesmo se o código estiver funcionando bem agora, é muito fácil quebrar tudo alterando a implementação da classe
ScopedLock .
Erro N14, N15, N16, N17 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... }
Aviso do PVS-Studio:
V668 CWE-570 Não faz sentido testar o ponteiro 'pEvent' contra nulo, pois a memória foi alocada usando o operador 'novo'. A exceção será gerada no caso de erro de alocação de memória. fsm.cpp 259
A verificação do ponteiro não faz sentido, porque, no caso de um erro de alocação de memória, será
lançada uma exceção
std :: bad_alloc .
Portanto, a verificação é supérflua, mas o erro não é grave. No entanto, tudo fica muito pior quando alguma lógica é executada no corpo da
instrução if . Considere o seguinte caso:
CFsmTransition* CFsm::AddTransition(....) { .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL;
Aviso do analisador: V668 CWE-570 Não há sentido em testar o ponteiro 'pNewTransition' contra nulo, pois a memória foi alocada usando o operador 'new'. A exceção será gerada no caso de erro de alocação de memória. fsm.cpp 289
É feita uma tentativa de liberar memória cujo endereço está armazenado no ponteiro de
pEvent . Naturalmente, isso não acontecerá e ocorrerá um vazamento de memória.
De fato, quando comecei a lidar com esse código, tudo ficou mais complicado e talvez não houvesse um erro, mas dois. Agora vou explicar o que há de errado com este código. Para fazer isso, precisamos nos familiarizar com o dispositivo de função
AddEvent .
CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { CFsmEvent* pEvent = NULL;
Observe que a função nem sempre retorna um ponteiro para um novo objeto criado usando o
novo operador. Às vezes, é necessário um objeto existente do contêiner
m_Events . E o ponteiro para o objeto recém-criado, a propósito, também será colocado em
m_Events .
E aqui surge a pergunta: quem é o proprietário e deve destruir os objetos cujos ponteiros estão armazenados no contêiner
m_Events ? Não estou familiarizado com o projeto, mas provavelmente em algum lugar existe um código que destrói todos esses objetos. Em seguida, excluir um objeto dentro da
função CFsm :: AddTransition geralmente
é supérfluo.
Tive a impressão de que você pode simplesmente excluir o seguinte fragmento de código:
if ( !pNewTransition ) { delete pEvent; return NULL; }
Outros erros:
- V668 CWE-571 Não há sentido em testar o ponteiro 'ret' contra nulo, pois a memória foi alocada usando o operador 'novo'. A exceção será gerada no caso de erro de alocação de memória. TerrainTextureEntry.cpp 120
- V668 CWE-571 Não há sentido em testar o ponteiro 'resposta' contra nulo, pois a memória foi alocada usando o operador 'novo'. A exceção será gerada no caso de erro de alocação de memória. SoundManager.cpp 542
Erro N18, N19 static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) {
Aviso do PVS-Studio:
V701 CWE-401 realloc () possível vazamento: quando realloc () falha na alocação de memória, o ponteiro original 'dsd-> entradas' é perdido. Considere atribuir realloc () a um ponteiro temporário. mongoose.cpp 2462
Se o tamanho da matriz se tornar insuficiente, a memória será alocada usando a função
realloc . O erro é que o valor do ponteiro para o bloco de memória original é imediatamente substituído pelo novo valor retornado pela função
realloc .
Se a alocação de memória falhar, a função
realloc retornará NULL e esse NULL será gravado na variável
dsd-> inputs . Depois disso, será impossível liberar o bloco de memória cujo endereço foi armazenado anteriormente nas
entradas dsd-> . Um vazamento de memória ocorrerá.
Outro erro: V701 CWE-401 realloc () possível vazamento: quando realloc () falha na alocação de memória, o ponteiro original 'Buffer' é perdido. Considere atribuir realloc () a um ponteiro temporário. Preprocessor.cpp 84
Conclusão
Não posso dizer que desta vez o artigo tenha sido fascinante ou que consegui mostrar muitos erros terríveis. O que fazer, uma vez por vez, não é necessário.
O que eu vejo, então eu escrevo .
Obrigado pela atenção. Para variar, terminarei o artigo com um convite para me seguir no Instagram
@pvs_studio_unicorn e no Twitter
@Code_Analysis .
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Andrey Karpov.
Bom trabalho, autores do jogo 0 AD!