La funzione invalida inavvertitamente il parametro di riferimento - cosa è andato storto?

Question

La funzione invalida inavvertitamente il parametro di riferimento - cosa è andato storto?

#1 da (35 voti)
#2 da (23 voti)
#3 da (10 voti)
#4 da (2 voti)
#5 da (0 voti)
#6 da (-4 voti)

54

Oggi abbiamo scoperto la causa di un brutto bug che si è verificato solo saltuariamente su determinate piattaforme. Bollito, il nostro codice assomigliava a questo:

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);
    }
  }
}

Il problema potrebbe sembrare ovvio in questo caso semplificato: B passa un riferimento alla chiave a A , che rimuove la voce della mappa prima di tentare di stamparla. (Nel nostro caso, non è stato stampato, ma usato in un modo più complicato) Questo è ovviamente un comportamento non definito, poiché key è un riferimento ciondolante dopo la chiamata a erase .

Risolvere ciò è stato banale - abbiamo appena cambiato il tipo di parametro da const string& a string . La domanda è: come abbiamo potuto evitare questo bug in primo luogo? Sembra che entrambe le funzioni abbiano fatto la cosa giusta:

A non ha modo di sapere che key si riferisce alla cosa che sta per distruggere.
B potrebbe aver fatto una copia prima di passarla a A , ma non è compito del callee decidere se prendere parametri per valore o per riferimento?

C'è qualche regola che non siamo riusciti a seguire?

c++

posta Nikolai 08.12.2016 - 15:01

fonte

6 risposte

Leggi altre domande sui tag c++

Chi ha creato l'idea (o le idee) dei primi costrutti del ciclo? È una cattiva pratica non eliminare i file ridondanti subito da VCS, ma contrassegnarli come "Da eliminare" con i commenti prima?

score 35 · Answer 1

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

Mentre questo è vero, A conosce le seguenti cose:

Il suo scopo è quello di distruggere qualcosa .
Ci vuole un parametro che è esattamente lo stesso tipo della cosa che distruggerà.

Dati questi fatti, è possibile per A a distruggere il proprio parametro se prende il parametro come puntatore / riferimento. Questo non è l'unico posto in C ++ in cui tali considerazioni devono essere affrontate.

Questa situazione è simile a come la natura di un operatore di assegnazione operator= significa che potrebbe essere necessario preoccuparsi dell'assegnazione di sé. Questa è una possibilità perché il tipo di this e il tipo del parametro di riferimento sono gli stessi.

Va notato che questo è solo problematico perché A più tardi intende utilizzare il parametro key dopo aver rimosso la voce. Se così non fosse, allora andrebbe bene. Naturalmente, diventa facile avere tutto perfettamente funzionante, quindi qualcuno cambia A per usare key dopo che è stato potenzialmente distrutto.

Sarebbe un buon posto per un commento.

Is there some rule we failed to follow?

In C ++, non puoi operare partendo dal presupposto che se segui ciecamente un insieme di regole, il tuo codice sarà sicuro al 100%. Non possiamo avere regole per tutto .

Considerare il punto 2 sopra. A potrebbe aver preso alcuni parametri di un tipo diverso dalla chiave, ma l'oggetto stesso potrebbe essere un sottoprogetto di una chiave nella mappa. In C ++ 14, find può assumere un tipo diverso dal tipo di chiave, purché vi sia un confronto valido tra di essi. Quindi, se fai m.erase(m.find(key)) , puoi distruggere il parametro anche se il tipo del parametro non è il tipo di chiave.

Quindi una regola come "se il tipo di parametro e il tipo di chiave sono gli stessi, prendili per valore" non ti salverà. Avresti bisogno di più informazioni di quelle.

In definitiva, è necessario prestare attenzione ai casi d'uso specifici e all'esercizio del giudizio, informati dall'esperienza.

score 23 · Answer 2

Direi di sì, c'è una regola abbastanza semplice che hai violato che ti avrebbe salvato: il principio della responsabilità unica.

In questo momento, A è passato ad un parametro che usa sia per rimuovere un elemento da una mappa, e fare altri elaborati (stampare come mostrato sopra, apparentemente qualcos'altro nel codice reale ). La combinazione di queste responsabilità mi sembra molto all'origine del problema.

Se abbiamo una funzione che solo cancella il valore dalla mappa, e un'altra che solo esegue l'elaborazione di un valore dalla mappa, dovremmo chiama ciascuno dal codice di livello superiore, quindi ci ritroveremmo con qualcosa del genere:

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

Certo, i nomi che ho usato indubbiamente rendono il problema più ovvio di quanto non lo sarebbero i nomi reali, ma se i nomi hanno un significato, sono quasi certi di mettere in chiaro che stiamo cercando di continuare a usare il riferimento dopo che è stato invalidato. Il semplice cambiamento di contesto rende il problema molto più ovvio.

score 10 · Answer 3

Is there some rule we failed to follow?

Sì, non hai documentato la funzione .

Senza una descrizione del contratto che passa i parametri (in particolare la parte relativa alla validità del parametro - si trova all'inizio della chiamata di funzione o in tutto) è impossibile dire se l'errore è nell'implementazione (se il contratto di chiamata è che il parametro è valido all'avvio della chiamata, la funzione deve eseguire una copia prima di eseguire qualsiasi azione che possa invalidare il parametro) o nel chiamante (se il contratto di chiamata è che il parametro deve rimanere valido per tutta la chiamata, il chiamante non può passare un riferimento ai dati all'interno della collezione in fase di modifica).

Ad esempio, lo stesso standard C ++ specifica che:

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.

ma non riesce a specificare se questo si applica solo all'istante in cui viene effettuata la chiamata o durante l'esecuzione della funzione. Tuttavia, in molti casi è chiaro che solo il secondo è addirittura possibile, vale a dire quando l'argomento non può essere mantenuto valido facendo una copia.

Ci sono alcuni casi reali in cui questa distinzione entra in gioco. Ad esempio, aggiungendo un std::vector<T> a se stesso

score 2 · Answer 4

Is there some rule we failed to follow?

Sì, non è stato possibile testarlo correttamente. Non sei solo, e sei nel posto giusto per imparare:)

C ++ ha un sacco di comportamento indefinito, il comportamento indefinito si manifesta in modi sottili e fastidiosi.

Probabilmente non puoi mai scrivere codice C ++ sicuro al 100%, ma puoi certamente ridurre la probabilità di introdurre accidentalmente il comportamento non definito nella tua base di codice impiegando un numero di strumenti.

Avvisi del compilatore
Analisi statica (versione estesa degli avvisi)
Binari dei test strumentati
Binari di produzione induriti

Nel tuo caso dubito che (1) e (2) possano essere di grande aiuto, anche se in generale consiglio di utilizzarli. Per ora concentriamoci sugli altri due.

Sia gcc che Clang presentano un flag -fsanitize che identifica i programmi che compilate per verificare una serie di problemi. -fsanitize=undefined , ad esempio, catturerà l'intero underflow / overflow con segno, lo spostamento di una quantità troppo alta, ecc ... Nel tuo caso specifico, -fsanitize=address e -fsanitize=memory sarebbero stati in grado di captare il problema ... fornendo hai un test che chiama la funzione. Per completezza, vale la pena utilizzare -fsanitize=thread se si dispone di una base di codice multi-thread. Se non riesci a implementare il binario (ad esempio, hai librerie di terze parti senza la loro origine), puoi anche utilizzare valgrind anche se in generale è più lento.

I compilatori recenti presentano anche una ricchezza possibilità di indurimento . La principale differenza con i binari strumentati è che i controlli di tempra sono progettati per avere un basso impatto sulle prestazioni (< 1%), rendendoli adatti al codice di produzione in generale. I più noti sono i controlli CFI (Control Flow Integrity) che sono progettati per sventare attacchi di distruzione dello stack e hi-jack del puntatore virtuale tra altri modi per sovvertire il flusso di controllo.

Il punto di entrambi (3) e (4) è trasformare un errore intermittente in un errore certo : entrambi seguono il fail fast principio. Ciò significa che:

fallisce sempre quando passi sulla mina
fallisce immediatamente , indicandoti l'errore piuttosto che corrompendo la memoria in modo casuale, ecc ...

Combinare (3) con una buona copertura di prova dovrebbe catturare la maggior parte dei problemi prima che colpiscano la produzione. Usare (4) in produzione può essere la differenza tra un bug fastidioso e un exploit.

score 0 · Answer 5

@note: questo post aggiunge solo altri argomenti in cima alla risposta di 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?

Entrambe le funzioni hanno fatto la cosa giusta.

Il problema è all'interno del codice client, che non teneva conto degli effetti collaterali della chiamata A.

C ++ non ha un modo diretto per specificare gli effetti collaterali nella lingua.

Questo significa che spetta a te (e al tuo team) assicurarsi che cose come gli effetti collaterali siano visibili nel codice (come documentazione) e mantenute con il codice (dovresti probabilmente considerare la documentazione delle pre-condizioni, post- condizioni e invarianti, anche per ragioni di visibilità).

Cambia codice:

class Foo {
  map<string,string> m;

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

Da questo momento in poi hai qualcosa in cima all'API che ti dice che dovresti avere un test unitario per questo; Ti dice anche come usare (e non usare) l'API.

score -4 · Answer 6

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

C'è un solo modo per evitare i bug: smettere di scrivere codice. Tutto il resto è fallito in qualche modo.

Tuttavia, testare il codice a vari livelli (test unitari, test funzionali, test di integrazione, test di accettazione, ecc.) non solo migliorerà la qualità del codice, ma ridurrà anche il numero di bug.