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: B
przekazuje 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ż key
jest 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:
A
nie ma sposobu, aby wiedzieć, żekey
odnosi się to do rzeczy, którą zamierza zniszczyć.B
mógł zrobić kopię przed jej przekazaniemA
, 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?
Powiedziałbym, że tak, złamałeś dość prostą zasadę, która by cię uratowała: zasada pojedynczej odpowiedzialności.
W tej chwili
A
przekazywany 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 :
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.
źródło
A
faktycznie szukałem nakey
dwóch różnych mapach i, jeśli został znaleziony, usunął wpisy plus dodatkowe czyszczenie. Nie jest więc jasne, czy naszA
naruszony SRP. Zastanawiam się, czy powinienem zaktualizować pytanie w tym momencie.m.erase(key)
ma pierwszą odpowiedzialność icout << "Erased: " << key
drugą 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.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:
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źródło
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.
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ą
-fsanitize
flagę, która instrumentuje kompilowane programy w celu sprawdzenia różnych problemów.-fsanitize=undefined
na przykład wyłapie niedopełnienie / przepełnienie liczby całkowitej, przesunięcie o zbyt dużą liczbę itp. W twoim konkretnym przypadku-fsanitize=address
i-fsanitize=memory
prawdopodobnie byłby problem z wykryciem ... pod warunkiem, że masz test wywołujący tę funkcję. Dla kompletności-fsanitize=thread
warto 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ćvalgrind
tego, 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:
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.
źródło
@ Uwaga: ten post po prostu dodaje więcej argumentów nad odpowiedzią Bena Voigta .
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:
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ć).
źródło
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.
źródło