Hoje descobrimos a causa de um bug desagradável que só acontecia intermitentemente em determinadas plataformas. Resumindo, nosso código ficou assim:
class Foo {
map<string,string> m;
void A(const string& key) {
m.erase(key);
cout << "Erased: " << key; // oops
}
void B() {
while (!m.empty()) {
auto toDelete = m.begin();
A(toDelete->first);
}
}
}
O problema pode parecer óbvio neste caso simplificado: B
passa uma referência à chave para A
, que remove a entrada do mapa antes de tentar imprimi-la. (No nosso caso, não foi impresso, mas usado de uma maneira mais complicada) Esse comportamento é obviamente indefinido, pois key
é uma referência pendente após a chamada para erase
.
A correção foi trivial - apenas alteramos o tipo de parâmetro de const string&
para string
. A questão é: como poderíamos ter evitado esse bug em primeiro lugar? Parece que ambas as funções fizeram a coisa certa:
A
não tem como saber quekey
se refere à coisa que está prestes a destruir.B
poderia ter feito uma cópia antes de passá-laA
, mas não é tarefa do receptor decidir se aceita parâmetros por valor ou por referência?
Existe alguma regra que não seguimos?
Eu diria que sim, há uma regra bastante simples que você violou que o salvaria: o princípio da responsabilidade única.
No momento,
A
é passado um parâmetro que ele usa para remover um item de um mapa e fazer outros processados (impressão como mostrado acima, aparentemente algo mais no código real). Combinar essas responsabilidades me parece muito da fonte do problema.Se tivermos uma função que apenas exclui o valor do mapa e outra que apenas processa um valor do mapa, teríamos que chamar cada uma delas a partir de código de nível superior, para terminar com algo assim :
É verdade que os nomes que eu usei, sem dúvida, tornam o problema mais óbvio do que os nomes reais, mas se os nomes tiverem algum significado, é quase certo que deixemos claro que estamos tentando continuar usando a referência depois que ela for concluída. foi invalidado. A simples mudança de contexto torna o problema muito mais óbvio.
fonte
A
procuramoskey
em dois mapas diferentes e, se encontrado, removemos as entradas, além de uma limpeza extra. Portanto, não está claro que nossoA
SRP violado. Gostaria de saber se devo atualizar a pergunta neste momento.m.erase(key)
tem a primeira responsabilidade ecout << "Erased: " << key
a segunda, portanto, a estrutura do código mostrada nesta resposta não é realmente diferente da estrutura do código no exemplo, ainda em No mundo real, o problema foi esquecido. O princípio da responsabilidade única não faz nada para garantir, ou mesmo torná-lo mais provável, que seqüências contraditórias de ações únicas aparecerão próximas do código do mundo real.Sim, você não documentou a função .
Sem uma descrição do contrato de passagem de parâmetro (especificamente a parte relativa à validade do parâmetro - é no início da chamada de função ou durante todo), é impossível saber se o erro está na implementação (se a chamada do contrato é que o parâmetro é válido quando a chamada é iniciada, a função deve fazer uma cópia antes de executar qualquer ação que possa invalidar o parâmetro) ou no chamador (se o contrato de chamada é que o parâmetro deve permanecer válido durante toda a chamada, o chamador não poderá passar uma referência aos dados dentro da coleção que está sendo modificada).
Por exemplo, o próprio padrão C ++ especifica que:
mas falha ao especificar se isso se aplica apenas ao instante em que a chamada é feita ou durante a execução da função. No entanto, em muitos casos, é claro que apenas o último é possível - ou seja, quando o argumento não pode ser mantido válido fazendo uma cópia.
Existem alguns casos do mundo real em que essa distinção entra em jogo. Por exemplo, anexando a
std::vector<T>
a si mesmofonte
Sim, você não conseguiu testá-lo corretamente. Você não está sozinho e está no lugar certo para aprender :)
O C ++ tem muitos comportamentos indefinidos, comportamento indefinido se manifesta de maneiras sutis e irritantes.
Você provavelmente nunca pode escrever código C ++ 100% seguro, mas certamente pode diminuir a probabilidade de introduzir acidentalmente o comportamento indefinido em sua base de código, empregando várias ferramentas.
No seu caso, duvido que (1) e (2) ajudariam muito, embora em geral eu recomendo usá-los. Por enquanto, vamos nos concentrar nos outros dois.
O gcc e o Clang apresentam um
-fsanitize
sinalizador que instrui os programas que você compila para verificar uma variedade de problemas.-fsanitize=undefined
por exemplo, vai pegar inteiro assinado underflow / estouro, passando por uma quantidade muito alta, etc ... No seu caso específico,-fsanitize=address
e-fsanitize=memory
teria sido propensos a pegar na questão ... desde que tenha um teste de chamar a função. Para completar,-fsanitize=thread
vale a pena usar se você tiver uma base de código multithread. Se você não conseguir implementar o binário (por exemplo, você possui bibliotecas de terceiros sem a fonte), também poderá usar,valgrind
embora seja mais lento em geral.Compiladores recentes também apresentam inúmeras possibilidades de proteção . A principal diferença com os binários instrumentados é que as verificações de proteção são projetadas para ter um baixo impacto no desempenho (<1%), tornando-as adequadas para o código de produção em geral. As mais conhecidas são as verificações de CFI (Control Flow Integrity), que são projetadas para impedir ataques de esmagamento de pilha e ataques de ponteiros virtuais entre outras maneiras de subverter o fluxo de controle.
O objetivo de (3) e (4) é transformar uma falha intermitente em uma certa falha : ambos seguem o princípio de falha rápida . Isso significa que:
Combinar (3) com uma boa cobertura de teste deve capturar a maioria dos problemas antes que eles atinjam a produção. Usar (4) na produção pode ser a diferença entre um bug irritante e uma exploração.
fonte
Nota: este post adiciona mais argumentos em cima da resposta de Ben Voigt .
Ambas as funções fizeram a coisa correta.
O problema está no código do cliente, que não levou em consideração os efeitos colaterais da chamada A.
O C ++ não possui uma maneira direta de especificar efeitos colaterais no idioma.
Isso significa que depende de você (e sua equipe) garantir que coisas como efeitos colaterais sejam visíveis no código (como documentação) e mantidas com o código (você deve considerar a possibilidade de documentar pré-condições, pós-condições e invariantes também, por razões de visibilidade).
Alteração de código:
A partir deste ponto, você tem algo no topo da API que diz que você deve fazer um teste de unidade para isso; Também mostra como usar (e não usar) a API.
fonte
Há apenas uma maneira de evitar erros: pare de escrever código. Tudo o resto falhou de alguma maneira.
No entanto, testar o código em vários níveis (testes de unidade, testes funcionais, testes de integração, testes de aceitação etc.) não apenas melhorará a qualidade do código, mas também reduzirá o número de bugs.
fonte