Funkcja nieumyślnie unieważnia parametr referencyjny - co poszło nie tak?

Dzisiaj odkryliśmy przyczynę paskudnego błędu, który zdarzał się sporadycznie tylko na niektórych platformach. Nasz kod wyglądał następująco:

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

Problem może wydawać się oczywisty w tym uproszczonym przypadku: Bprzekazuje odwołanie do klucza A, który usuwa wpis mapy przed próbą wydrukowania. (W naszym przypadku nie został wydrukowany, ale użyto go w bardziej skomplikowany sposób) Jest to oczywiście niezdefiniowane zachowanie, ponieważ keyjest to wiszące odniesienie po wywołaniu erase.

Naprawienie tego było trywialne - właśnie zmieniliśmy typ parametru z const string&na string. Pytanie brzmi: w jaki sposób moglibyśmy uniknąć tego błędu? Wygląda na to, że obie funkcje działały poprawnie:

• Anie ma sposobu, aby wiedzieć, że keyodnosi się to do rzeczy, którą zamierza zniszczyć.
• Bmógł zrobić kopię przed jej przekazaniem A, ale czy zadaniem odbiorcy nie jest podjęcie decyzji o przyjęciu parametrów według wartości, czy przez odniesienie?

Czy jest jakaś zasada, której nie przestrzegaliśmy?

Anie ma sposobu, aby wiedzieć, że keyodnosi się to do rzeczy, którą zamierza zniszczyć.

Chociaż jest to prawda, Awie o następujących rzeczach:

1. Jego celem jest zniszczenie czegoś .

2. Pobiera parametr, który jest dokładnie tego samego rodzaju rzeczy, którą zniszczy.

Biorąc pod uwagę te fakty, to jest możliwe do Azniszczyć własny parametr jeśli trwa parametr jako wskaźnik / odniesienia. Nie jest to jedyne miejsce w C ++, w którym należy zająć się takimi kwestiami.

Ta sytuacja jest podobna do tego, jak natura operator=operatora przypisania oznacza, że ​​możesz potrzebować obawy o samodzielne przypisanie. Jest to możliwe, ponieważ typ thisi typ parametru referencyjnego są takie same.

Należy zauważyć, że jest to tylko problematyczne, ponieważ Apóźniej zamierza użyć keyparametru po usunięciu wpisu. Jeśli nie, to będzie dobrze. Oczywiście wtedy łatwo jest mieć wszystko idealnie działające, a potem ktoś zmienia Aużycie keypo potencjalnym zniszczeniu.

To byłoby dobre miejsce na komentarz.

Czy jest jakaś zasada, której nie przestrzegaliśmy?

W C ++ nie możesz działać przy założeniu, że jeśli będziesz ślepo przestrzegać zestawu reguł, twój kod będzie w 100% bezpieczny. Nie możemy mieć zasad dotyczących wszystkiego .

Rozważ punkt 2 powyżej. Amógł wziąć jakiś parametr innego typu niż klucz, ale sam obiekt może być podobiektem klucza na mapie. W C ++ 14 findmoże przyjmować typ inny niż typ klucza, o ile istnieje prawidłowe porównanie między nimi. Jeśli tak m.erase(m.find(key)), możesz zniszczyć parametr, nawet jeśli typ parametru nie jest typem klucza.

Zatem zasada typu „jeśli typ parametru i typ klucza są takie same, weź je według wartości” nie uratuje cię. Potrzebujesz więcej informacji niż tylko to.

Ostatecznie musisz zwrócić uwagę na konkretne przypadki użycia i dokonać oceny, na podstawie doświadczenia.

Powiedziałbym, że tak, złamałeś dość prostą zasadę, która by cię uratowała: zasada pojedynczej odpowiedzialności.

W tej chwili Aprzekazywany jest parametr, którego używa, aby zarówno usunąć element z mapy, jak i wykonać inne przetworzone (drukowanie jak pokazano powyżej, najwyraźniej coś innego w prawdziwym kodzie). Łączenie tych obowiązków wydaje mi się źródłem źródła problemu.

Jeśli mamy jedną funkcję, która po prostu usuwa wartość z mapy, a drugą, która po prostu przetwarza wartość z mapy, musielibyśmy wywołać każdą z kodu wyższego poziomu, więc otrzymalibyśmy coś takiego :

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

To prawda, że ​​nazwy, których użyłem, niewątpliwie sprawiają, że problem jest bardziej oczywisty niż prawdziwe nazwiska, ale jeśli nazwy są w ogóle znaczące, są prawie pewne, że wyjaśnią, że staramy się nadal używać odwołania po jego został unieważniony. Prosta zmiana kontekstu sprawia, że ​​problem jest o wiele bardziej oczywisty.

Czy jest jakaś zasada, której nie przestrzegaliśmy?

Tak, nie udało się udokumentować funkcji .

Bez opisu umowy dotyczącej przekazywania parametrów (w szczególności części dotyczącej ważności parametru - czy jest to na początku wywołania funkcji, czy w całym tekście) nie można stwierdzić, czy błąd występuje we wdrożeniu (jeśli umowa wywołania jest to, że parametr jest ważny w momencie rozpoczęcia połączenia, funkcja musi wykonać kopię przed wykonaniem jakiejkolwiek czynności, która może unieważnić parametr) lub w abonencie wywołującym (jeśli w umowie dotyczącej połączenia parametr musi pozostać ważny przez cały czas trwania połączenia, osoba dzwoniąca nie może przekazać odwołanie do danych w modyfikowanej kolekcji).

Na przykład sam standard C ++ określa, że:

Jeśli argument funkcji ma niepoprawną wartość (na przykład wartość spoza dziedziny funkcji lub wskaźnik niepoprawny do zamierzonego użycia), zachowanie jest niezdefiniowane.

ale nie określa, czy dotyczy to tylko momentu wywołania, czy też wykonania funkcji. Jednak w wielu przypadkach jest oczywiste, że tylko ta ostatnia jest nawet możliwa - mianowicie wtedy, gdy nie można utrzymać ważności argumentu poprzez wykonanie kopii.

Istnieje wiele rzeczywistych przypadków, w których to rozróżnienie ma zastosowanie. Na przykład dołączenie std::vector<T>do siebie

Czy jest jakaś zasada, której nie przestrzegaliśmy?

Tak, nie udało się go poprawnie przetestować. Nie jesteś sam i jesteś we właściwym miejscu do nauki :)

C ++ ma wiele niezdefiniowanych zachowań, niezdefiniowane zachowanie przejawia się w subtelny i irytujący sposób.

Prawdopodobnie nigdy nie będziesz w stanie napisać w 100% bezpiecznego kodu C ++, ale z pewnością możesz zmniejszyć prawdopodobieństwo przypadkowego wprowadzenia niezdefiniowanego zachowania w swojej bazie kodu, stosując szereg narzędzi.

1. Ostrzeżenia kompilatora
2. Analiza statyczna (rozszerzona wersja ostrzeżeń)
3. Instrumentalne pliki binarne
4. Utwardzone pliki binarne

W twoim przypadku wątpię (1) i (2), że bardzo by pomogły, choć ogólnie radzę ich używać. Na razie skupmy się na dwóch pozostałych.

Zarówno gcc, jak i Clang mają -fsanitizeflagę, która instrumentuje kompilowane programy w celu sprawdzenia różnych problemów. -fsanitize=undefinedna przykład wyłapie niedopełnienie / przepełnienie liczby całkowitej, przesunięcie o zbyt dużą liczbę itp. W twoim konkretnym przypadku -fsanitize=addressi -fsanitize=memoryprawdopodobnie byłby problem z wykryciem ... pod warunkiem, że masz test wywołujący tę funkcję. Dla kompletności -fsanitize=threadwarto skorzystać, jeśli masz wielowątkową bazę kodową. Jeśli nie możesz zaimplementować pliku binarnego (na przykład, masz biblioteki innych firm bez ich źródła), możesz także użyć valgrindtego, choć ogólnie jest wolniejszy.

Najnowsze kompilatory oferują również możliwości umacniania bogactwa . Główną różnicą w stosunku do oprzyrządowanych plików binarnych jest to, że kontrole hartowania mają mały wpływ na wydajność (<1%), co czyni je odpowiednimi do kodu produkcyjnego. Najbardziej znane są kontrole CFI (Control Flow Integrity), które zostały zaprojektowane w celu udaremnienia ataków polegających na niszczeniu stosów i hi-jackingu wirtualnego wskaźnika między innymi sposobami obalenia kontroli.

Zarówno w (3), jak i (4) chodzi o przekształcenie przerywanej awarii w pewną awarię : obie działają zgodnie z zasadą szybkiego niepowodzenia . To znaczy że:

• zawsze zawiedzie, gdy nadepniesz na minę
• natychmiast kończy się niepowodzeniem , wskazując na błąd, a nie na przypadkowe uszkodzenie pamięci itp.

Połączenie (3) z dobrym zasięgiem testowym powinno wychwycić większość problemów, zanim trafią one do produkcji. Użycie (4) w produkcji może być różnicą między irytującym błędem a exploitem.

@ Uwaga: ten post po prostu dodaje więcej argumentów nad odpowiedzią Bena Voigta .

Pytanie brzmi: w jaki sposób moglibyśmy uniknąć tego błędu? Wygląda na to, że obie funkcje działały poprawnie:

• Nie ma sposobu, aby wiedzieć, że klucz odnosi się do rzeczy, którą zamierza zniszczyć.
• B mógł wykonać kopię przed przekazaniem jej do A, ale czy zadaniem odbiorcy nie jest podjęcie decyzji o przyjęciu parametrów według wartości, czy przez odniesienie?

Obie funkcje działały poprawnie.

Problem leży w kodzie klienta, który nie wziął pod uwagę skutków ubocznych wywołania A.

C ++ nie ma bezpośredniego sposobu na określenie efektów ubocznych w języku.

Oznacza to, że do Ciebie (i Twojego zespołu) należy dopilnowanie, aby rzeczy takie jak skutki uboczne były widoczne w kodzie (jako dokumentacja) i utrzymywane przy pomocy kodu (prawdopodobnie powinieneś rozważyć udokumentowanie warunków wstępnych, warunków dodatkowych i niezmienników) również ze względu na widoczność).

Zmiana kodu:

class Foo {
  map<string,string> m;

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

Od tego momentu masz coś na wierzchu interfejsu API, który mówi ci, że powinieneś mieć test jednostkowy na to; Mówi także, jak korzystać z interfejsu API (a nie używać).

jak mogliśmy w pierwszej kolejności uniknąć tego błędu?

Jest tylko jeden sposób na uniknięcie błędów: przestań pisać kod. Wszystko inne zawiodło w jakiś sposób.

Jednak testowanie kodu na różnych poziomach (testy jednostkowe, testy funkcjonalne, testy integracyjne, testy akceptacyjne itp.) Nie tylko poprawią jakość kodu, ale także zmniejszą liczbę błędów.