A função inadvertidamente invalida 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 certas plataformas. Abaixo, 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-lo. (No nosso caso, não foi impresso, mas usado de uma maneira mais complicada) Isso é um comportamento indefinido, já que key é uma referência pendente após a chamada para erase .

Corrigir isso foi trivial - apenas mudamos 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 que key 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 candidato decidir se vai tomar parâmetros por valor ou por referência?

Existe alguma regra que não seguimos?

    
por Nikolai 08.12.2016 / 16:01
fonte

6 respostas

35

A has no way of knowing that key refers to the thing it's about to destroy.

Embora isso seja verdade, A sabe o seguinte:

  1. Seu objetivo é destruir algo .

  2. É preciso um parâmetro que é exatamente do mesmo tipo da coisa que ele irá destruir.

Dado estes fatos, é possível que A destrua seu próprio parâmetro se tomar o parâmetro como um ponteiro / referência. Este não é o único lugar em C ++ onde tais considerações precisam ser abordadas.

Essa situação é semelhante a como a natureza de um operador% de atribuição de operator= significa que você precisa se preocupar com atribuição automática. Essa é uma possibilidade porque o tipo de this e o tipo do parâmetro de referência são os mesmos.

Deve-se notar que isso é apenas problemático porque A depois pretende usar o parâmetro key após remover a entrada. Se isso não acontecesse, estaria tudo bem. Claro, então fica fácil ter tudo funcionando perfeitamente, então alguém altera A para usar key depois de ter sido potencialmente destruído.

Esse seria um bom lugar para um comentário.

Is there some rule we failed to follow?

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

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

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

Em última análise, você precisa prestar atenção aos seus casos de uso específicos e exercer um julgamento informado pela experiência.

    
por 08.12.2016 / 16:34
fonte
23

Eu diria que sim, há uma regra bastante simples que você quebrou e que salvou você: o princípio da responsabilidade única.

Agora, A é passado um parâmetro que ele usa para remover um item de um mapa, e fazer algum outro processado (impressão como mostrado acima, aparentemente algo mais no código real ). Combinar essas responsabilidades parece-me 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 um de um código de nível mais alto, então acabaríamos com algo assim:

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

Com certeza, os nomes que usei, sem dúvida, tornam o problema mais óbvio do que os nomes reais, mas se os nomes forem significativos, é quase certo que eles deixarão claro que estamos tentando continuar usando o nome. referência depois de ter sido invalidada. A simples mudança de contexto torna o problema muito mais óbvio.

    
por 08.12.2016 / 19:16
fonte
10

Is there some rule we failed to follow?

Sim, você não documentou a função .

Sem uma descrição do contrato de passagem de parâmetros (especificamente a parte relativa à validade do parâmetro - é no início da chamada de função ou ao longo) é impossível dizer se o erro está na implementação (se o contrato de chamada é 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 for que o parâmetro deve permanecer válido durante a chamada, o chamador não pode passar uma referência aos dados dentro da coleção sendo modificada).

Por exemplo, o próprio padrão C ++ especifica que:

If an argument to a function has an invalid value (such as a value outside the domain of the function or a pointer invalid for its intended use), the behavior is undefined.

mas não especifica 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 é mesmo possível - ou seja, quando o argumento não pode ser mantido válido ao fazer uma cópia.

Existem alguns casos do mundo real em que essa distinção entra em cena. Por exemplo, anexando um std::vector<T> a si mesmo

    
por 08.12.2016 / 18:17
fonte
2

Is there some rule we failed to follow?

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, o comportamento indefinido se manifesta de maneiras sutis e irritantes.

Você provavelmente não pode escrever código C ++ 100% seguro, mas certamente pode diminuir a probabilidade de introduzir acidentalmente o Undefined Behavior 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 protegidos

No seu caso, duvido que (1) e (2) tenham ajudado muito, embora em geral eu recomende usá-los. Por enquanto, vamos nos concentrar nos outros dois.

O gcc e o Clang apresentam um -fsanitize flag que instrumenta os programas que você compila para verificar uma variedade de problemas. -fsanitize=undefined , por exemplo, irá capturar underflow / overflow de número inteiro assinado, alterando por uma quantidade muito alta, etc ... No seu caso específico, -fsanitize=address e -fsanitize=memory provavelmente teriam captado o problema ... fornecendo você tem um teste chamando a função. Para completar, -fsanitize=thread vale a pena usar se você tiver uma base de código multi-threaded. Se você não puder implementar o binário (por exemplo, você tem bibliotecas de terceiros sem sua origem), então você também pode usar valgrind , embora seja mais lento em geral.

Compiladores recentes também apresentam uma riqueza possibilidades de endurecimento . A principal diferença com binários instrumentados é que as verificações de endurecimento 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 evitar ataques de esmagamento de pilha e o apontamento de ponteiro virtual, entre outras maneiras de subverter o fluxo de controle.

O ponto de ambos (3) e (4) é transformar uma falha intermitente em uma determinada falha : ambos seguem a falha rápida

  • sempre falha quando você pisa na mina terrestre
  • falha imediatamente , apontando você para o erro em vez de corromper aleatoriamente a memória, etc ...

A combinação de (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 um exploit.

    
por 09.12.2016 / 13:23
fonte
0

@note: esta postagem apenas adiciona mais argumentos à resposta de Ben Voigt .

The question is: how could we have avoided this bug in the first place? It seems both functions did the right thing:

  • A has no way of knowing that key refers to the thing it's about to destroy.
  • B could have made a copy before passing it to A, but isn't it the callee's job to decide whether to take parameters by value or by reference?

Ambas as funções fizeram o correto.

O problema está dentro do código do cliente, que não levou em consideração os efeitos colaterais da chamada A

.

O C ++ não tem um meio direto de especificar efeitos colaterais no idioma.

Isso significa que cabe a 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 (provavelmente você deve considerar a documentação de pré-condições, condições e invariantes, bem como, 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
  }
  ...

Deste ponto em diante, você tem algo sobre a API que diz que você deve ter um teste de unidade para ele; Ele também informa como usar (e não usar) a API.

    
por 27.12.2016 / 09:31
fonte
-4

how could we have avoided this bug in the first place?

Existe apenas uma maneira de evitar erros: parar de escrever código. Tudo o resto falhou de alguma forma.

No entanto, o teste de 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 erros.

    
por 08.12.2016 / 16:44
fonte

Tags