Функция случайно делает недействительным ссылочный параметр - что пошло не так?

Сегодня мы узнали причину неприятной ошибки, которая периодически возникала только на определенных платформах. Сложив наш код выглядел так:

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

Проблема может показаться очевидной в этом упрощенном случае: Bпередается ссылка на ключ A, который удаляет запись карты перед попыткой ее распечатать. (В нашем случае это не было напечатано, но использовалось более сложным способом) Это, конечно, неопределенное поведение, так keyкак это ссылка висячая после вызова erase.

Исправить это было тривиально - мы просто изменили тип параметра с const string&на string. Вопрос в том, как мы могли бы избежать этой ошибки. Кажется, обе функции сделали правильно:

• Aне может знать, что keyотносится к тому, что он собирается уничтожить.
• Bмог бы сделать копию, прежде чем передать ее A, но разве не вызывающей стороне решать, принимать ли параметры по значению или по ссылке?

Есть ли какое-то правило, которому мы не смогли следовать?

Aне может знать, что keyотносится к тому, что он собирается уничтожить.

Хотя это действительно так, Aзнает следующие вещи:

1. Его цель - уничтожить что-то .

2. Он принимает параметр, который точно такого же типа, что и объект, который он уничтожит.

Учитывая эти факты, то возможно для Aуничтожить свой собственный параметр , если он принимает параметр как указатель / ссылка. Это не единственное место в C ++, где необходимо учитывать такие соображения.

Эта ситуация похожа на то, как характер operator=оператора присваивания означает, что вам может потребоваться заботиться о самопредставлении. Это возможно, потому что тип thisи тип ссылочного параметра одинаковы.

Следует отметить, что это только проблематично, потому что Aпозже намеревается использовать keyпараметр после удаления записи. Если этого не произойдет, то это будет хорошо. Конечно, тогда становится легко заставить все работать идеально, а затем кто-то переходит Aна использование keyпосле того, как оно может быть уничтожено.

Это было бы хорошим местом для комментариев.

Есть ли какое-то правило, которому мы не смогли следовать?

В C ++ вы не можете действовать при условии, что если вы будете слепо следовать набору правил, ваш код будет на 100% безопасным. У нас не может быть правил для всего .

Рассмотрим пункт № 2 выше. Aмог бы принять некоторый параметр типа, отличного от ключа, но сам объект мог бы быть подобъектом ключа на карте. В C ++ 14 findможет принимать тип, отличный от типа ключа, при условии, что между ними есть правильное сравнение. Поэтому, если вы это сделаете m.erase(m.find(key)), вы можете уничтожить параметр, даже если тип параметра не является типом ключа.

Таким образом, правило типа «если тип параметра и тип ключа совпадают, примите их по значению» не спасет вас. Вам нужно больше информации, чем просто.

В конечном счете, вам необходимо уделять внимание конкретным случаям использования и выносить суждения, основываясь на своем опыте.

Я бы сказал, да, есть довольно простое правило, которое вы нарушили, которое спасло бы вас: принцип единой ответственности.

Прямо сейчас Aпередается параметр, который он использует как для удаления элемента с карты, так и для другой обработки (печать, как показано выше, очевидно, что-то еще в реальном коде). Сочетание этих обязанностей выглядит для меня, как большая часть источника проблемы.

Если у нас есть одна функция, которая просто удаляет значение из карты, и другая, которая просто выполняет обработку значения из карты, нам пришлось бы вызывать каждую из кода более высокого уровня, поэтому мы получили бы что-то вроде этого :

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

Конечно, имена, которые я использовал, несомненно, делают проблему более очевидной, чем реальные имена, но если имена вообще значимы, они почти наверняка прояснят, что мы пытаемся продолжать использовать ссылку после был признан недействительным. Простое изменение контекста делает проблему намного более очевидной.

Есть ли какое-то правило, которому мы не смогли следовать?

Да, вам не удалось задокументировать функцию .

Без описания контракта на передачу параметров (в частности, части, относящейся к действительности параметра - в начале вызова функции или во всем), невозможно определить, есть ли ошибка в реализации (если контракт вызова является то, что параметр является действительным, когда начинается вызов, функция должна сделать копию перед выполнением любого действия, которое может сделать недействительным параметр) или в вызывающей стороне (если контракт вызова заключается в том, что параметр должен оставаться действительным в течение всего вызова, вызывающая сторона не может передать ссылку на данные внутри изменяемой коллекции).

Например, сам стандарт C ++ определяет, что:

Если аргумент функции имеет недопустимое значение (например, значение вне домена функции или указатель, недопустимый для ее предполагаемого использования), поведение не определено.

но в нем не указывается, применяется ли это только к моменту вызова или во время выполнения функции. Однако во многих случаях ясно, что возможна только последняя, ​​а именно, когда аргумент нельзя сохранить действительным, сделав копию.

Есть немало реальных случаев, когда это различие вступает в игру. Например, добавление std::vector<T>к себе

Есть ли какое-то правило, которому мы не смогли следовать?

Да, вы не смогли проверить это правильно. Ты не один, и ты в правильном месте, чтобы учиться :)

C ++ имеет много неопределенного поведения, неопределенное поведение проявляется тонкими и раздражающими способами.

Вы, вероятно, никогда не сможете написать 100% безопасный код C ++, но вы, безусловно, можете уменьшить вероятность случайного введения Undefined Behavior в вашу кодовую базу, используя ряд инструментов.

1. Предупреждения компилятора
2. Статический анализ (расширенная версия предупреждений)
3. Инструментальные тестовые двоичные файлы
4. Закаленные производственные двоичные файлы

В вашем случае я сомневаюсь, что (1) и (2) очень помогли бы, хотя в целом я советую использовать их. А пока давайте сосредоточимся на двух других.

И gcc, и Clang имеют -fsanitizeфлаг, который используется инструментами, которые вы компилируете, для проверки множества проблем. -fsanitize=undefinedнапример, будет перехватывать целое число переполнения / переполнения, сдвига на слишком большое количество и т. д. ... В вашем конкретном случае, -fsanitize=addressи -fsanitize=memory, скорее всего , возникнет проблема ... при условии, что у вас есть тест, вызывающий функцию. Для полноты, -fsanitize=threadстоит использовать, если у вас есть многопоточная кодовая база. Если вы не можете реализовать двоичный файл (например, у вас есть сторонние библиотеки без их источника), то вы также можете использовать valgrindего, хотя в целом он медленнее.

Последние компиляторы также показывают возможности повышения благосостояния . Основное отличие от инструментальных бинарных файлов заключается в том, что проверки на закалку предназначены для того, чтобы оказывать низкое влияние на производительность (<1%), что делает их пригодными для производственного кода в целом. Самыми известными являются проверки CFI (целостность потока управления), которые предназначены для предотвращения атак с разбивкой стека и хай-джеккинга виртуальных указателей, в том числе для подрыва потока управления.

Цель обоих (3) и (4) - преобразовать прерывистый отказ в определенный отказ : оба они следуют принципу быстрого отказа . Это значит, что:

• это всегда терпит неудачу, когда вы наступаете на мины
• он немедленно завершается неудачно , указывая на ошибку, а не на случайное повреждение памяти и т. д.

Объединение (3) с хорошим тестовым покрытием должно выявить большинство проблем, прежде чем они попадут в производство. Использование (4) в производстве может быть различием между надоедливой ошибкой и эксплойтом.

@ примечание: этот пост просто добавляет больше аргументов поверх ответа Бена Фойгта .

Вопрос в том, как мы могли бы избежать этой ошибки. Кажется, обе функции сделали правильно:

• У А нет способа узнать, что ключ относится к тому, что он собирается уничтожить.
• B мог бы сделать копию, прежде чем передать ее A, но разве вызывающий не должен решать, принимать ли параметры по значению или по ссылке?

Обе функции сделали правильную вещь.

Проблема в клиентском коде, который не учитывает побочные эффекты вызова A.

C ++ не имеет прямого способа указать побочные эффекты в языке.

Это означает, что вы (и ваша команда) должны убедиться, что такие вещи, как побочные эффекты, видны в коде (как документация) и поддерживаются в коде (вам, вероятно, следует рассмотреть возможность документирования предварительных условий, постусловий и инвариантов а также, из соображений наглядности).

Изменение кода:

class Foo {
  map<string,string> m;

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

С этого момента у вас есть что-то поверх API, которое говорит вам, что у вас должен быть модульный тест для него; Он также говорит вам, как использовать (а не использовать) API.

как мы могли бы избежать этой ошибки в первую очередь?

Есть только один способ избежать ошибок: прекратить писать код. Все остальное не получилось каким-то образом

Однако тестирование кода на различных уровнях (модульные тесты, функциональные тесты, интеграционные тесты, приемочные тесты и т. Д.) Не только улучшит качество кода, но и уменьшит количество ошибок.