La función invalida inadvertidamente el parámetro de referencia. ¿Qué salió mal?

  

A no tiene forma de saber que key se refiere a lo que está a punto de destruir.

Si bien esto es cierto, A sabe lo siguiente:

1. Su propósito es destruir algo .

2. Toma un parámetro que es del mismo tipo de la cosa que destruirá.

Dados estos hechos, es posible que A destruya su propio parámetro si toma el parámetro como un puntero / referencia. Este no es el único lugar en C ++ donde es necesario abordar tales consideraciones.

Esta situación es similar a la forma en que la naturaleza de un operador de asignación operator= significa que es posible que deba preocuparse por la autoasignación. Esa es una posibilidad porque el tipo de this y el tipo del parámetro de referencia son los mismos.

Debe tenerse en cuenta que esto solo es problemático porque A luego intenta usar el parámetro key después de eliminar la entrada. Si no fuera así, entonces estaría bien. Por supuesto, entonces es fácil hacer que todo funcione perfectamente, entonces alguien cambia A para usar key después de que potencialmente haya sido destruido.

Ese sería un buen lugar para un comentario.

  

¿Hay alguna regla que no hayamos seguido?

En C ++, no puede operar bajo el supuesto de que si sigue ciegamente un conjunto de reglas, su código estará 100% seguro. No podemos tener reglas para todo .

Considera el punto # 2 arriba. A podría haber tomado algún parámetro de un tipo diferente al de la clave, pero el objeto en sí podría ser un subobjeto de una clave en el mapa. En C ++ 14, find puede tomar un tipo diferente del tipo de clave, siempre que haya una comparación válida entre ellos. Entonces, si lo hace m.erase(m.find(key)) , puede destruir el parámetro aunque el tipo de parámetro no sea el tipo de clave.

Por lo tanto, una regla como "si el tipo de parámetro y el tipo de clave son iguales, tómelos por valor" no lo salvará. Necesitaría más información que solo eso.

En última instancia, debe prestar atención a sus casos de uso específicos y ejercer un juicio, informado por la experiencia.

Yo diría que sí, hay una regla bastante simple que rompiste que te habría salvado: el principio de responsabilidad única.

En este momento, A recibe un parámetro que usa para eliminar un elemento de un mapa, y procesan otros (imprimiendo como se muestra arriba, aparentemente algo más en el código real) ). La combinación de esas responsabilidades me parece que es gran parte de la fuente del problema.

Si tenemos una función que solo elimina el valor del mapa, y otra que solo procesa un valor del mapa, tendríamos que llame a cada uno desde un código de nivel superior, así que terminaríamos con algo como esto:

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

Por supuesto, los nombres que he usado indudablemente hacen que el problema sea más obvio que los nombres reales, pero si los nombres son significativos, es casi seguro que dejarán en claro que estamos tratando de seguir usando el Referencia después de que ha sido invalidado. El simple cambio de contexto hace que el problema sea mucho más obvio.

  

¿Hay alguna regla que no hayamos seguido?

Sí, no pudo documentar la función .

Sin una descripción del contrato de paso de parámetros (específicamente la parte relacionada con la validez del parámetro, ya sea al comienzo de la llamada de la función o en toda su extensión), es imposible saber si el error está en la implementación (si el contrato de llamada es que el parámetro es válido cuando se inicia la llamada, la función debe hacer una copia antes de realizar cualquier acción que pueda invalidar el parámetro) o en la persona que llama (si el contrato de llamada es que el parámetro debe permanecer válido durante toda la llamada, la persona que llama no puede pasar una referencia a los datos dentro de la colección que se está modificando).

Por ejemplo, el propio estándar de C ++ especifica que:

  

Si un argumento de una función tiene un valor no válido (como un valor fuera del dominio de la función o un puntero no válido para su uso previsto), el comportamiento no está definido.

pero no especifica si esto se aplica solo en el momento en que se realiza la llamada, o durante la ejecución de la función. Sin embargo, en muchos casos está claro que solo esto último es posible, especialmente cuando el argumento no se puede mantener válido haciendo una copia.

Hay bastantes casos reales en los que esta distinción entra en juego. Por ejemplo, agregar un std::vector<T> a sí mismo

  

¿Hay alguna regla que no hayamos seguido?

Sí, no lo probaste correctamente. No estás solo, y estás en el lugar correcto para aprender :)

C ++ tiene una gran cantidad de comportamientos indefinidos, comportamientos indefinidos de formas sutiles y molestas.

Es probable que nunca pueda escribir un código C ++ 100% seguro, pero ciertamente puede disminuir la probabilidad de introducir accidentalmente un comportamiento indefinido en su base de código empleando una serie de herramientas.

1. Advertencias del compilador
2. Análisis estático (versión extendida de las advertencias)
3. Binarios de prueba instrumentados
4. Binarios de producción endurecidos

En tu caso, dudo que (1) y (2) hubieran ayudado mucho, aunque en general aconsejo usarlos. Por ahora concentrémonos en los otros dos.

Tanto gcc como Clang cuentan con un indicador -fsanitize que instrumenta los programas que compila para verificar una variedad de problemas. -fsanitize=undefined , por ejemplo, detectará un desbordamiento / desbordamiento de enteros con signo, desplazándose en una cantidad demasiado alta, etc. En su caso específico, -fsanitize=address y -fsanitize=memory habrían resuelto el problema ... proporcionando Tienes una prueba llamando a la función. Para completar, vale la pena utilizar -fsanitize=thread si tiene un código base de subprocesos múltiples. Si no puede implementar el binario (por ejemplo, tiene bibliotecas de terceros sin su fuente), también puede usar valgrind , aunque en general es más lento.

Los compiladores recientes también cuentan con un rico posibilidades de endurecimiento . La principal diferencia con los binarios instrumentados es que los controles de endurecimiento están diseñados para tener un bajo impacto en el rendimiento (< 1%), lo que los hace adecuados para el código de producción en general. Las más conocidas son las verificaciones de CFI (Control Flow Integrity), que están diseñadas para impedir ataques de aplastamiento de apilamientos y para el secuestro de punteros virtuales, entre otras formas de subvertir el flujo de control.

El punto de ambos (3) y (4) es transformar un fallo intermitente en un cierto fallo : ambos siguen el error rápido principio. Esto significa que:

• siempre falla cuando pisas la mina terrestre
• falla inmediatamente , indicándole el error en lugar de corromper la memoria al azar, etc ...

Combinar (3) con una buena cobertura de prueba debería detectar la mayoría de los problemas antes de que lleguen a la producción. Usar (4) en producción puede ser la diferencia entre un error molesto y un exploit.

@note: esta publicación solo agrega más argumentos sobre La respuesta de Ben Voigt .

  

La pregunta es: ¿cómo podríamos haber evitado este error en primer lugar? Parece que ambas funciones hicieron lo correcto:

     

• A no tiene forma de saber que la clave se refiere a lo que está a punto de destruir.
  
• B podría haber hecho una copia antes de pasársela a A, pero ¿no es el trabajo del que llama decidir si tomar parámetros por valor o por referencia?
  

Ambas funciones hicieron lo correcto.

El problema está dentro del código del cliente, que no tuvo en cuenta los efectos secundarios de llamar a A.

C ++ no tiene una forma directa de especificar efectos secundarios en el idioma.

Esto significa que depende de usted (y su equipo) asegurarse de que cosas como los efectos secundarios sean visibles en el código (como documentación) y se mantengan con el código (probablemente debería considerar documentar las condiciones previas, condiciones e invariantes también, por razones de visibilidad también).

Cambio de código:

class Foo {
  map<string,string> m;

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

A partir de este momento, tienes algo en la parte superior de la API que te dice que debes hacerte una prueba de unidad; También le indica cómo usar (y no usar) la API.

  

¿Cómo podríamos haber evitado este error en primer lugar?

Solo hay una forma de evitar errores: dejar de escribir código. Todo lo demás falló de alguna manera.

Sin embargo, las pruebas de código en varios niveles (pruebas unitarias, pruebas funcionales, pruebas de integración, pruebas de aceptación, etc.) no solo mejorarán la calidad del código, sino que también reducirán la cantidad de errores.