A função invalida inadvertidamente o parâmetro de referência - o que deu errado?

54

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: Bpassa 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:

  • Anão tem como saber que keyse refere à coisa que está prestes a destruir.
  • Bpoderia ter feito uma cópia antes de passá-la A, mas não é tarefa do receptor decidir se aceita parâmetros por valor ou por referência?

Existe alguma regra que não seguimos?

Nikolai
fonte

Respostas:

35

Anão tem como saber que keyse refere à coisa que está prestes a destruir.

Embora isso seja verdade, Aele sabe o seguinte:

  1. Seu objetivo é destruir algo .

  2. É preciso um parâmetro que seja exatamente do mesmo tipo que ele destruirá.

Perante estes factos, é possível para Adestruir seu próprio parâmetro se ele leva o parâmetro como um ponteiro / referência. Este não é o único local em C ++ em que essas considerações precisam ser abordadas.

Essa situação é semelhante à forma como a natureza de um operator=operador de atribuição significa que você pode precisar se preocupar com a auto-atribuição. Essa é uma possibilidade, porque o tipo thise o tipo do parâmetro de referência são os mesmos.

Note-se que isso é apenas problemático, porque Amais tarde pretende usar o keyparâmetro após remover a entrada. Se não, então tudo bem. Obviamente, então fica fácil ter tudo funcionando perfeitamente, então alguém muda Apara usar keydepois que potencialmente foi destruído.

Esse seria um bom lugar para comentar.

Existe alguma regra que não seguimos?

No C ++, você não pode operar com o pressuposto de que, se você seguir cegamente um conjunto de regras, seu código será 100% seguro. Não podemos ter regras para tudo .

Considere o ponto 2 acima. Apoderia ter tomado algum parâmetro de um tipo diferente da chave, mas o próprio objeto poderia ser um subobjeto de uma chave no mapa. No C ++ 14, findpode assumir um tipo diferente do tipo de chave, desde que haja uma comparação válida entre eles. Portanto, se você fizer isso m.erase(m.find(key)), poderá destruir o parâmetro, mesmo que o tipo do parâmetro não seja o tipo de chave.

Portanto, uma regra como "se o tipo de parâmetro e o tipo de chave forem iguais, aceite-os por valor" não o salvará. Você precisaria de mais informações do que apenas isso.

Por fim, você precisa prestar atenção em seus casos de uso específicos e exercer julgamento, informado pela experiência.

Nicol Bolas
fonte
10
Bem, você poderia ter a regra de "nunca compartilhe estado mutável" ou é dual "Nunca mutação estado compartilhado", mas depois que você luta para escrever c identificável ++
Caleth
7
@ Caleth Se você deseja usar essas regras, C ++ provavelmente não é o idioma para você.
user253751
3
@Caleth Você está descrevendo Rust?
Malcolm
11
"Não podemos ter regras para tudo." Sim, nós podemos. cstheory.stackexchange.com/q/4052
Ouroborus
23

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 :

std::string &key = get_value_from_map();
destroy(key);
continue_to_use(key);

É 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.

Jerry Coffin
fonte
3
Bem, isso é uma observação válida, se aplica apenas de maneira muito restrita a este caso. Há muitos exemplos em que o SRP é respeitado e ainda há problemas com a função potencialmente invalidando seu próprio parâmetro.
Ben Voigt
5
@ BenVoigt: Apenas invalidar seu parâmetro não causa um problema. Ele continua a usar o parâmetro depois de invalidado, o que leva a problemas. Mas, em última análise, sim, você está certo: embora isso o tenha salvado nesse caso, há indubitavelmente casos em que é insuficiente.
Jerry Coffin
3
Ao escrever um exemplo simplificado, você deve omitir alguns detalhes, e às vezes acontece que um desses detalhes era importante. No nosso caso, Aprocuramos keyem dois mapas diferentes e, se encontrado, removemos as entradas, além de uma limpeza extra. Portanto, não está claro que nosso ASRP violado. Gostaria de saber se devo atualizar a pergunta neste momento.
Nikolai
2
Para expandir o argumento de @BenVoigt: no exemplo de Nicolai, m.erase(key)tem a primeira responsabilidade e cout << "Erased: " << keya 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.
sdenham
10

Existe alguma regra que não seguimos?

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:

Se um argumento para uma função tiver um valor inválido (como um valor fora do domínio da função ou um ponteiro inválido para o uso pretendido), o comportamento será indefinido.

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 mesmo

Ben Voigt
fonte
"falha ao especificar se isso se aplica apenas ao instante em que a chamada é feita ou durante a execução da função." Na prática, os compiladores fazem praticamente tudo o que desejam em toda a função, uma vez que o UB é chamado. Isso pode levar a um comportamento realmente estranho se o programador não pegar o UB.
@ boneco de neve, apesar de interessante, a reordenação do UB não tem nenhuma relação com o que discuto nesta resposta, que é a responsabilidade de garantir a validade (para que o UB nunca aconteça).
Ben Voigt
qual é exatamente o meu ponto: a pessoa que escreve o código precisa ser responsável por evitar o UB para evitar uma toca de coelho cheia de problemas.
@ Snowman: Não há "uma pessoa" que grave todo o código em um projeto. Essa é uma das razões pelas quais a documentação da interface é tão importante. Outra é que interfaces bem definidas reduzem a quantidade de código que precisa ser fundamentada ao mesmo tempo - para qualquer projeto não trivial, simplesmente não é possível que alguém seja "responsável" por pensar na correção de cada declaração.
Ben Voigt
Eu nunca disse que uma pessoa escreve todo o código. Em um determinado momento, um programador pode estar olhando para uma função ou escrevendo código. Tudo o que estou tentando dizer é que quem está olhando para o código precisa ter cuidado, porque, na prática, o UB é infeccioso e se espalha de uma linha de código em escopos mais amplos quando o compilador está envolvido. Isso remonta ao seu argumento sobre a violação do contrato de uma função: estou de acordo com você, mas afirmando que ela pode se tornar um problema ainda maior.
2

Existe alguma regra que não seguimos?

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.

  1. Avisos do compilador
  2. Análise estática (versão estendida dos avisos)
  3. Binários de Teste Instrumentados
  4. Binários de produção reforçados

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 -fsanitizesinalizador que instrui os programas que você compila para verificar uma variedade de problemas. -fsanitize=undefinedpor exemplo, vai pegar inteiro assinado underflow / estouro, passando por uma quantidade muito alta, etc ... No seu caso específico, -fsanitize=addresse -fsanitize=memoryteria sido propensos a pegar na questão ... desde que tenha um teste de chamar a função. Para completar, -fsanitize=threadvale 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, valgrindembora 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:

  • sempre falha quando você pisa na mina terrestre
  • falha imediatamente , apontando o erro ao invés de corromper aleatoriamente a memória, etc ...

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.

Matthieu M.
fonte
0

Nota: este post adiciona mais argumentos em cima da resposta de Ben Voigt .

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 que a chave se refere à coisa que está prestes a destruir.
  • B poderia ter feito uma cópia antes de passá-la para A, mas não é tarefa do receptor decidir se aceita parâmetros por valor ou por referência?

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:

class Foo {
  map<string,string> m;

  /// \sideeffect invalidates iterators
  void A(const string& key) {
    m.erase(key);
    cout << "Erased: " << key; // oops
  }
  ...

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.

utnapistim
fonte
-4

como poderíamos ter evitado esse bug em primeiro lugar?

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.

BЈовић
fonte
11
Isso é um absurdo completo. Há não somente uma maneira de evitar erros. Embora seja trivialmente verdade que a única maneira de evitar completamente a existência de bugs é nunca escrever código, também é verdade (e muito mais útil) que existem vários procedimentos de engenharia de software que você pode seguir, tanto ao escrever inicialmente um código quanto ao testá-lo, isso pode reduzir significativamente a presença de bugs. Todo mundo sabe sobre a fase de teste, mas o maior impacto geralmente pode ser obtido com o menor custo, seguindo práticas e expressões de design responsáveis ​​e escrevendo o código em primeiro lugar.
Cody Gray