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

54

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?

Mikołaj
źródło

Odpowiedzi:

35

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.

Nicol Bolas
źródło
10
Cóż, możesz mieć zasadę „nigdy nie udostępniaj stanu zmiennego” lub jego podwójny „nigdy nie mutuj stanu wspólnego”, ale wtedy miałbyś
problem z napisaniem możliwego
7
@Caleth Jeśli chcesz korzystać z tych reguł, C ++ prawdopodobnie nie jest dla Ciebie językiem.
user253751
3
@Caleth Opisujesz Rust?
Malcolm,
1
„Nie możemy mieć zasad dotyczących wszystkiego”. Tak możemy. cstheory.stackexchange.com/q/4052
Ouroborus
23

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.

Jerry Coffin
źródło
3
Cóż, jest to ważna obserwacja, dotyczy to bardzo wąsko w tym przypadku. Istnieje wiele przykładów poszanowania SRP i nadal występują problemy z funkcją potencjalnie unieważniającą jej własny parametr.
Ben Voigt,
5
@BenVoigt: Samo unieważnienie jego parametru nie powoduje jednak problemu. Nadal używa parametru po jego unieważnieniu, co prowadzi do problemów. Ale ostatecznie tak, masz rację: choć w tym przypadku uratowałoby go to, istnieją niewątpliwie przypadki, w których jest to niewystarczające.
Jerry Coffin
3
Pisząc uproszczony przykład, musisz pominąć niektóre szczegóły, a czasem okazuje się, że jeden z tych szczegółów był ważny. W naszym przypadku Afaktycznie szukałem na keydwóch różnych mapach i, jeśli został znaleziony, usunął wpisy plus dodatkowe czyszczenie. Nie jest więc jasne, czy nasz Anaruszony SRP. Zastanawiam się, czy powinienem zaktualizować pytanie w tym momencie.
Mikołaj
2
Aby rozwinąć kwestię @BenVoigt: w przykładzie Nicolai m.erase(key)ma pierwszą odpowiedzialność i cout << "Erased: " << keydrugą odpowiedzialność, więc struktura kodu pokazana w tej odpowiedzi w rzeczywistości nie różni się od struktury kodu w przykładzie, ale w w prawdziwym świecie problem został przeoczony. Zasada pojedynczej odpowiedzialności nie robi nic, aby zapewnić, a nawet zwiększyć prawdopodobieństwo, że sprzeczne sekwencje pojedynczych działań pojawią się blisko siebie w kodzie świata rzeczywistego.
sdenham
10

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

Ben Voigt
źródło
„nie określa, czy dotyczy to tylko momentu wywołania, czy też wykonania funkcji”. W praktyce, po wywołaniu UB kompilatory robią właściwie wszystko, co chcą. Może to prowadzić do naprawdę dziwnych zachowań, jeśli programista nie złapie UB.
@snowman, chociaż interesujące, zmiana kolejności UB jest całkowicie niezwiązana z tym, co omawiam w tej odpowiedzi, która jest odpowiedzialna za zapewnienie ważności (aby UB nigdy się nie zdarzyło).
Ben Voigt,
o to właśnie mi chodzi: osoba pisząca kod musi być odpowiedzialna za unikanie UB, aby uniknąć całej dziury królika pełnej problemów.
@Snowman: Nie ma „jednej osoby”, która napisałaby cały kod w projekcie. To jeden z powodów, dla których dokumentacja interfejsu jest tak ważna. Innym jest to, że dobrze zdefiniowane interfejsy zmniejszają ilość kodu, o którym należy jednocześnie wnioskować - w przypadku każdego nietrywialnego projektu po prostu niemożliwe jest, aby ktoś był „odpowiedzialny” za myślenie o poprawności każdego stwierdzenia.
Ben Voigt,
Nigdy nie mówiłem, że jedna osoba pisze cały kod. W pewnym momencie programista może patrzeć na funkcję lub pisać kod. Wszystko, co próbuję powiedzieć, to to, że ktokolwiek patrzy na kod, musi uważać, ponieważ w praktyce UB jest zakaźny i rozprzestrzenia się z jednego wiersza kodu na szersze zakresy, gdy tylko kompilator się zaangażuje. To wraca do twojego punktu o naruszeniu kontraktu funkcji: zgadzam się z tobą, ale stwierdzam, że może on stać się jeszcze większym problemem.
2

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.

Matthieu M.
źródło
0

@ 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ć).

utnapistim
źródło
-4

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.

BЈовић
źródło
1
To kompletny nonsens. Istnieje nie tylko jeden sposób uniknięcia błędów. Chociaż jest to trywialnie prawdą, że jedynym sposobem na całkowite uniknięcie błędów jest nigdy nie pisanie kodu, prawdą jest również (i znacznie bardziej przydatne), że istnieją różne procedury inżynierii oprogramowania, których można przestrzegać, zarówno przy początkowym pisaniu kodu, jak i podczas testowania może to znacznie zmniejszyć obecność błędów. Wszyscy wiedzą o fazie testowania, ale największy wpływ można często osiągnąć przy najniższych kosztach, przestrzegając odpowiedzialnych praktyk projektowych i idiomów podczas pisania kodu.
Cody Gray,