Nossos leitores estão crescendo, por isso escrevemos artigos repetidamente que explicam como usar corretamente a metodologia de análise de código estático. Consideramos muito importante explicar que as ferramentas de análise estática não devem ser usadas esporadicamente, mas regularmente. Mais uma vez, demonstramos isso com um exemplo prático, revisando novamente o projeto Godot.
Use analisadores regularmente
Preparando-me para falar na conferência de desenvolvedores de jogos, decidi obter novos exemplos de erros interessantes que a ferramenta
PVS-Studio pode detectar. Para esse fim, vários mecanismos de jogo foram testados, um dos quais foi Godot. Não encontrei nenhum erro particularmente interessante para o relatório, mas queria escrever um artigo sobre erros comuns. Esses erros demonstram muito bem a relevância do uso regular de ferramentas de análise de código estático.
Deve-se notar que já
verificamos este projeto em 2015, e o autor trabalhou com os erros que escrevemos.
Aqui você pode ver o commit correspondente.
3 anos se passaram. O projeto mudou. O analisador PVS-Studio também mudou, e novos diagnósticos apareceram nele. Portanto, não é de surpreender que eu tenha conseguido escrever com facilidade e rapidez exemplos suficientes de erros para escrever este artigo.
No entanto, algo mais é importante. Ao desenvolver Godot ou qualquer outro projeto, novos erros aparecem constantemente e são corrigidos. Erros não detectados "se estabelecem" no código por um longo tempo e, em seguida, muitos deles podem ser detectados ao iniciar uma análise de código estática. Por esse motivo, às vezes há uma falsa sensação de que os analisadores estáticos encontram apenas alguns erros desinteressantes em seções de código raramente usadas. Obviamente, esse é o caso se você usar o analisador incorretamente e executá-lo apenas de tempos em tempos, por exemplo, pouco antes do lançamento do release.
Sim, ao escrever artigos, nós mesmos realizamos verificações únicas de projetos abertos. Mas temos um objetivo diferente. Queremos demonstrar os recursos de um analisador de código para detectar defeitos. Essa tarefa tem pouco a ver com melhorar a qualidade do código do projeto como um todo e reduzir os custos associados à correção de erros.
Mais uma vez sobre o principal. O objetivo da análise de código estático não é encontrar erros antigos. Esses erros antigos geralmente são insignificantes; caso contrário, eles teriam interferido nos usuários e já teriam sido encontrados e corrigidos. O objetivo da análise estática é corrigir rapidamente os erros no código recém-escrito ou modificado. Isso reduz o tempo de depuração, o número de reclamações dos usuários e, finalmente, reduz o orçamento do projeto desenvolvido.
Agora vamos aos bugs que os leitores de nossas publicações amam tanto.
Erros de copiar e colar
Vamos ver o que notei enquanto estudava o relatório do PVS-Studio. Começarei com meu diagnóstico V501 favorito, que encontra
erros em quase todos os projetos que verificamos :).
Erro N1virtual bool can_export(....) { .... if (!exists_export_template("uwp_" + platform_infix + "_debug.zip", &err) || !exists_export_template("uwp_" + platform_infix + "_debug.zip", &err)) { valid = false; r_missing_templates = true; } .... }
Aviso do PVS-Studio: V501 CWE-570 Existem sub-expressões idênticas '! Exists_export_template ("uwp_" + platform_infix + "_debug.zip", & err)' à esquerda e à direita do '||' operador. export.cpp 1135
Erro clássico de copiar e colar. A chamada de função foi copiada, mas não editada. O nome do segundo arquivo a ser processado deve terminar com "_release.zip".
Erros N2, N3 static String dump_node_code(SL::Node *p_node, int p_level) { .... if (bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW || bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW) { code += scode;
PVS-Studio Warning: V501 CWE-570 Existem sub-expressões idênticas 'bnode-> declarações [i] -> type == SL :: Node :: TYPE_CONTROL_FLOW' à esquerda e à direita da '||' operador. test_shader_lang.cpp 183
void EditorSpinSlider::_notification(int p_what) { if (p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT || p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT) { if (grabbing_spinner) { Input::get_singleton()->set_mouse_mode(Input::MOUSE_MODE_VISIBLE); grabbing_spinner = false; grabbing_spinner_attempt = false; } } .... }
PVS-Studio Warning: V501 CWE-570 Existem sub-expressões idênticas 'p_what == MainLoop :: NOTIFICATION_WM_FOCUS_OUT' à esquerda e à direita da '||' operador. editor_spin_slider.cpp 157
Eu acho que os erros são claramente visíveis e não exigem nenhuma explicação. Exatamente o mesmo Copy-Paste clássico, como no primeiro caso.
Erro N4 String SoftBody::get_configuration_warning() const { .... Transform t = get_transform(); if ((ABS(t.basis.get_axis(0).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(1).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(0).length() - 1.0) > 0.05)) { if (!warning.empty()) .... }
PVS-Studio Warning: V501 CWE-570 Existem subexpressões idênticas à esquerda e à direita do '||' operador. soft_body.cpp 399
Aqui a primeira linha foi copiada duas vezes. Mas o número do eixo de coordenadas foi alterado apenas na segunda linha. E eles se esqueceram de editar a terceira linha. Isso não passa de "
Last Line Effect ".
Nota No momento, além do “Efeito de última linha”, fiz as seguintes observações interessantes: “
A função mais perigosa do mundo C / C ++ ”, “O
mal vive em funções de comparação ”. E agora farei o anúncio de um novo artigo, cuja redação pretendo fazer no futuro próximo. O título do trabalho é "0, 1, 2". Deve ser interessante e instrutivo. Convido você a se inscrever em um dos canais para não perder:
twitter ,
vk.com ,
Instagram ,
telegrama e
rss "old school".
Erro N5 void ScrollContainer::_notification(int p_what) { .... if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= h_scroll->get_minimum_size().x; .... }
Aviso do PVS-Studio: V778 CWE-682 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'v_scroll' deva ser usada em vez de 'h_scroll'. scroll_container.cpp 249
Em relação a esse trecho de código, não tenho certeza absoluta de que há um erro. No entanto, concordo com o analisador de que o segundo bloco parece muito suspeito. Provavelmente, o código foi escrito usando Copy-Paste e, no segundo bloco de texto, eles esqueceram de substituir
h_scroll por
v_scroll .
O código provavelmente deve ser assim:
if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= v_scroll->get_minimum_size().x;
Erro N6Outro caso em que um fragmento de código grande o suficiente foi copiado e alterado sem êxito. A linha de erro é marcada pelo meu comentário "// <=".
void ShaderGLES2::bind_uniforms() { .... const Map<uint32_t, Variant>::Element *E = uniform_defaults.front(); while (E) { int idx = E->key(); int location = version->uniform_location[idx]; if (location < 0) { E = E->next(); continue; } Variant v; v = E->value(); _set_uniform_variant(location, v); E = E->next(); } const Map<uint32_t, CameraMatrix>::Element *C = uniform_cameras.front(); while (C) { int idx = E->key();
PVS-Studio Warning: V522 CWE-476 A desreferenciação do ponteiro nulo 'E' pode ocorrer. shader_gles2.cpp 102
O erro foi detectado indiretamente. Graças à análise
do fluxo de dados, o PVS-Studio revelou que o ponteiro
E pode ser zero no momento da desreferenciação.
O erro é que, no fragmento de código copiado, eles esqueceram de substituir
E em
C em um só lugar. Devido a esse erro, a função funciona de uma maneira muito estranha e faz coisas estranhas.
Erros de digitação
Erro N7É difícil para programadores que escrevem em linguagens diferentes de C ou C ++ imaginar que um erro de digitação pode ser feito escrevendo uma vírgula em vez de um asterisco *, e o código será compilado. No entanto, é assim.
LRESULT OS_Windows::WndProc(....) { .... BITMAPINFO bmi; ZeroMemory(&bmi, sizeof(BITMAPINFO)); bmi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER); bmi.bmiHeader.biWidth = dib_size.x; bmi.bmiHeader.biHeight = dib_size.y; bmi.bmiHeader.biPlanes = 1; bmi.bmiHeader.biBitCount = 32; bmi.bmiHeader.biCompression = BI_RGB; bmi.bmiHeader.biSizeImage = dib_size.x, dib_size.y * 4; .... }
PVS-Studio Warning: V521 CWE-480 Tais expressões usando o operador ',' são perigosas. Verifique se a expressão está correta. os_windows.cpp 776
A variável
bmi.bmiHeader.biSizeImage recebe o valor da variável
dib_size.x . Em seguida, o operador de vírgula ',' é executado, cuja
prioridade é menor que a do operador '='. O resultado da expressão
dib_size.y * 4 não é utilizado de forma alguma.
Em vez de uma vírgula na expressão, o operador de multiplicação '*' deve ser usado. Em primeiro lugar, essa expressão faria sentido. Em segundo lugar, abaixo há uma opção semelhante, mas já correta, para inicializar a mesma variável:
bmi.bmiHeader.biSizeImage = dib_size.x * dib_size.y * 4;
Erros N8, N9 void Variant::set(....) { .... int idx = p_index; if (idx < 0) idx += 4; if (idx >= 0 || idx < 4) { Color *v = reinterpret_cast<Color *>(_data._mem); (*v)[idx] = p_value; valid = true; } .... }
Aviso do PVS-Studio: V547 Expressão CWE-571 'idx> = 0 || idx <4 'é sempre verdadeiro. variant_op.cpp 2152
Qualquer índice será considerado correto. Para corrigir o erro, você precisa substituir o
|| em
&& :
if (idx >= 0 && idx < 4) {
Esse erro lógico provavelmente surgiu devido ao descuido, portanto, estou inclinado a atribuí-lo a erros de digitação.
Exatamente o mesmo erro pode ser observado no mesmo arquivo abaixo. A falha do erro de criação, aparentemente, é copiar e colar.
Erro múltiplo: V547 CWE-571 Expressão 'idx> = 0 || idx <4 'é sempre verdadeiro. variant_op.cpp 2527
Erro N10Um exemplo de erro do qual se deseja excluir: WTF ?!
void AnimationNodeBlendSpace1D::add_blend_point( const Ref<AnimationRootNode> &p_node, float p_position, int p_at_index) { ERR_FAIL_COND(blend_points_used >= MAX_BLEND_POINTS); ERR_FAIL_COND(p_node.is_null()); ERR_FAIL_COND(p_at_index < -1 || p_at_index > blend_points_used); if (p_at_index == -1 || p_at_index == blend_points_used) { p_at_index = blend_points_used; } else { for (int i = blend_points_used - 1; i > p_at_index; i++) { blend_points[i] = blend_points[i - 1]; } } .... }
Aviso do PVS-Studio: V621 CWE-835 Considere inspecionar o operador 'for'. É possível que o loop seja executado incorretamente ou não seja executado. animation_blend_space_1d.cpp 113
Observe a condição de parada do loop:
i> p_at_index . É sempre verdade, pois a variável
i é inicializada com o valor
blend_points_used - 1 . Além disso, a partir de duas verificações anteriores, segue-se que
blend_points_used> p_at_index .
A condição pode se tornar falsa somente quando ocorre um estouro da variável de sinal
i , que é um comportamento indefinido. Além disso, ele não alcançará um estouro, pois a matriz ultrapassará os limites muito antes.
Antes de nós, na minha opinião, há um erro de digitação bonito quando eles escrevem '>' em vez de '<'. Sim, tenho uma visão pervertida da beleza dos erros :).
O ciclo correto:
for (int i = blend_points_used - 1; i < p_at_index; i++) {
Erro N11Outro caso igualmente brilhante de erro de digitação na condição do ciclo.
void AnimationNodeStateMachineEditor::_state_machine_pos_draw() { .... int idx = -1; for (int i = 0; node_rects.size(); i++) { if (node_rects[i].node_name == playback->get_current_node()) { idx = i; break; } } .... }
Aviso do PVS-Studio: V693 CWE-835 Considere inspecionar a expressão condicional do loop. É possível que 'i <X.size ()' seja usado em vez de 'X.size ()'. animation_state_machine_editor.cpp 852
Pode ocorrer um estouro da matriz, pois o valor de
i aumenta incontrolavelmente. Código de segurança:
for (int i = 0; i < node_rects.size(); i++) {
Erro N12 GDScriptDataType GDScriptCompiler::_gdtype_from_datatype( const GDScriptParser::DataType &p_datatype) const { .... switch (p_datatype.kind) { .... case GDScriptParser::DataType::NATIVE: { result.kind = GDScriptDataType::NATIVE; result.native_type = p_datatype.native_type; } break; case GDScriptParser::DataType::SCRIPT: { result.kind = GDScriptDataType::SCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } case GDScriptParser::DataType::GDSCRIPT: { result.kind = GDScriptDataType::GDSCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } break; .... }
Aviso do PVS-Studio: V796 CWE-484 É possível que a instrução 'break' esteja ausente na instrução switch. gdscript_compiler.cpp 135
Esqueceu-se acidentalmente de escrever uma declaração de
intervalo . Portanto, quando entrar no
caso GDScriptParser :: DataType :: SCRIPT, as variáveis receberão valores, como se fosse o
caso GDScriptParser :: DataType :: GDSCRIPT .
Erro N13O seguinte erro pode ser classificado como Copiar e Colar. No entanto, não tenho certeza se uma linha curta foi copiada. Portanto, consideraremos isso um erro de digitação simples ao digitar.
void CPUParticles::_particles_process(float p_delta) { .... if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.velocity.z = 0.0; } .... }
Aviso do PVS-Studio: V519 CWE-563 A variável 'p.velocity.z' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 664, 665. cpu_particles.cpp 665
Duas vezes a atribuição da mesma variável. Abaixo você pode ver o seguinte fragmento de código:
if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.transform.origin.z = 0.0; }
Muito provavelmente, no primeiro caso, deveria ter sido escrito da mesma maneira.
Erro N14 bool AtlasTexture::is_pixel_opaque(int p_x, int p_y) const { if (atlas.is_valid()) { return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_x + region.position.y + margin.position.y ); } return true; }
Aviso do PVS-Studio: O parâmetro V751 'p_y' não é usado dentro do corpo da função. texture.cpp 1085
Fragmento da descrição do diagnóstico
V751 :
O analisador detectou uma função suspeita, um dos parâmetros que nunca é usado. Ao mesmo tempo, seu outro parâmetro é usado várias vezes, o que, possivelmente, indica a presença de um erro.Como você pode ver, isso é verdade e é muito suspeito. A variável
p_x é usada duas vezes e
p_y não
é usado. Provavelmente, deve ser escrito:
return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_y + region.position.y + margin.position.y );
A propósito, no código fonte a chamada de função é escrita em uma linha. Por esse motivo, é mais difícil perceber o erro. Se o autor do código escrevesse os argumentos reais em uma coluna, como escrevi no artigo, o erro imediatamente chamaria minha atenção. Lembre-se de que a formatação da tabela é muito útil e evita muitos erros de digitação. Consulte o capítulo "Alinhe o mesmo tipo de código com uma" tabela "no artigo"
A questão principal da programação, refatoração e tudo mais ".
Erro N15 bool SpriteFramesEditor::can_drop_data_fw(....) const { .... Vector<String> files = d["files"]; if (files.size() == 0) return false; for (int i = 0; i < files.size(); i++) { String file = files[0]; String ftype = EditorFileSystem::get_singleton()->get_file_type(file); if (!ClassDB::is_parent_class(ftype, "Texture")) { return false; } } .... }
PVS-Studio Warning: V767 Acesso suspeito ao elemento da matriz 'files' por um índice constante dentro de um loop. sprite_frames_editor_plugin.cpp 602
O loop processa o mesmo arquivo em todas as iterações do loop. Erro de digitação aqui:
String file = files[0];
Deve ser:
String file = files[i];
Outros erros
Erro N16 CSGBrush *CSGBox::_build_brush() { .... for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); else face_points[3 - j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); .... } .... }
O analisador PVS-Studio produz imediatamente duas respostas para este código:
- V547 CWE-570 A expressão 'i> = 3' é sempre falsa. csg_shape.cpp 939
- V547 CWE-571 A expressão 'i> = 3' é sempre verdadeira. csg_shape.cpp 941
De fato, esse operador ternário em ambas as expressões parece muito estranho:
i >= 3 ? -1 : 1
Em um caso, a condição é sempre verdadeira e, no outro, é falsa. Difícil dizer como esse código deve parecer correto. Talvez seja apenas redundante e possa ser escrito assim:
for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k]; else face_points[3 - j][(i + k) % 3] = -v[k]; .... }
Posso estar errado, e o código precisa ser corrigido de uma maneira completamente diferente.
Erro N17Quase não houve erros como o V595, embora eles geralmente sejam
encontrados em abundância em qualquer projeto . Aparentemente, esses erros foram corrigidos após uma verificação anterior e os erros desse tipo quase não apareceram. Vi apenas alguns falsos positivos e um erro.
bool CanvasItemEditor::_get_bone_shape(....) { .... Node2D *from_node = Object::cast_to<Node2D>( ObjectDB::get_instance(bone->key().from)); .... if (!from_node->is_inside_tree()) return false;
Aviso do PVS-Studio: V595 CWE-476 O ponteiro 'from_node' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 565, 567. canvas_item_editor_plugin.cpp 565
O ponteiro
from_node é primeiro desreferenciado para chamar a função
is_inside_tree e somente então é verificado se há igualdade de
nullptr . Os cheques devem ser trocados:
if (!from_node) return false; if (!from_node->is_inside_tree()) return false;
Erro N18 enum JoystickList { .... JOY_AXIS_MAX = 10, .... }; static const char *_axes[] = { "Left Stick X", "Left Stick Y", "Right Stick X", "Right Stick Y", "", "", "L2", "R2" }; int InputDefault::get_joy_axis_index_from_string(String p_axis) { for (int i = 0; i < JOY_AXIS_MAX; i++) { if (p_axis == _axes[i]) { return i; } } ERR_FAIL_V(-1); }
Aviso do PVS-Studio: É possível saturação da matriz V557 CWE-125. O valor do índice 'i' pode chegar a 9. input_default.cpp 1119
A matriz
_axes consiste em oito elementos. Nesse caso, a constante
JOY_AXIS_MAX , que define o número de iterações do loop, é 10. Acontece que o loop vai além do limite do array.
Erro N19E a última função muito estranha, que, aparentemente, foi projetada para testar alguma coisa. A função é longa, então eu darei como uma imagem (clique na imagem para ampliar).
Aviso do PVS-Studio: V779 CWE-561 Código inacessível detectado. É possível que haja um erro. test_math.cpp 457
Existem várias
instruções de retorno incondicional em uma função. Na foto, marquei-os com ovais vermelhos. Parece que vários testes de unidade diferentes foram coletados para esta função, mas esquecemos de remover os
NULLs de retorno extra. Como resultado, a função não verifica o que deve verificar. Quase todo o corpo de uma função consiste em código inacessível.
Talvez, é claro, esse seja algum tipo de idéia astuta. Mas parece-me que isso aconteceu por acaso e o código deve ser corrigido.
Vamos terminar com isso. Certamente, se você olhar atentamente para o relatório do analisador, poderá encontrar outros erros. No entanto, mesmo escrito mais do que suficiente para escrever um artigo. Então se tornará chato para mim e para o leitor :).
Conclusão
O artigo descreve erros que não existiriam se o código fosse analisado regularmente usando o PVS-Studio. No entanto, o mais importante é que, usando análises regulares, é possível encontrar e eliminar imediatamente muitos outros erros. Meu colega descreveu essa idéia com mais detalhes em sua nota: “
Filosofia da análise estática de código: temos 100 programadores, o analisador encontrou poucos erros, é inútil? ". Eu recomendo gastar 10 minutos lendo este artigo curto, mas muito importante.
Obrigado pela atenção. Convido todos a fazer o download e experimentar a análise estática do PVS-Studio para testar seus próprios projetos.
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Andrey Karpov.
Godot: Sobre o uso regular de analisadores estáticos .