Introducción
En repetidas ocasiones hemos buscado errores en proyectos que utilizan aprendizaje automático, y DeepSpeech no fue una excepción para nosotros. No es sorprendente, porque este proyecto es bastante popular: al momento de escribir este artículo, ya tiene más de 15k estrellas en GitHub.
Como es habitual, la búsqueda de errores que citaré en este artículo se realizó utilizando el analizador de código estático PVS-Studio.
Para su trabajo, DeepSpeech usa la biblioteca TensorFlow. Apagué el análisis del código de esta biblioteca, porque ya hemos escrito un artículo aparte al respecto.Sin embargo, no desactivé el análisis del resto de bibliotecas utilizadas. ¿Cuál es la razón para esto? Los errores dentro de cualquier biblioteca que incluya en su proyecto también se convierten en errores dentro de su proyecto. Por lo tanto, es útil analizar no solo su, sino también cualquier código de terceros que utilice. Puede leer una opinión detallada sobre esto en nuestro artículo reciente .
Con esto concluye la breve introducción: es hora de pasar al análisis de errores. Por cierto, si vino aquí para averiguar la respuesta a la pregunta que hice en el título del artículo (por qué no debería escribir en el espacio de nombres estándar), puede mirar inmediatamente al final del artículo. ¡Un ejemplo particularmente interesante le espera allí!
Resumen de 10 advertencias interesantes emitidas por el analizador
Advertencia 1
V773 Se salió de la función sin soltar el puntero de 'datos'. Es posible una pérdida de memoria. edit-fst.h 311
// EditFstData method implementations: just the Read method.
template <typename A, typename WrappedFstT, typename MutableFstT>
EditFstData<A, WrappedFstT, MutableFstT> *
EditFstData<A, WrappedFstT, MutableFstT>::Read(std::istream &strm,
const FstReadOptions &opts)
{
auto *data = new EditFstData<A, WrappedFstT, MutableFstT>();
// next read in MutabelFstT machine that stores edits
FstReadOptions edits_opts(opts);
....
std::unique_ptr<MutableFstT> edits(MutableFstT::Read(strm, edits_opts));
if (!edits) return nullptr; // <=
....
}Este fragmento de código contiene un ejemplo clásico de pérdida de memoria: la función de lectura llama a ' return nullptr ' sin liberar la memoria asignada con la expresión ' new EditFstData '. Con tal salida de la función (sin llamar a delete data ), solo se eliminará el puntero y no se llamará al destructor del objeto al que apunta. Por tanto, el objeto seguirá almacenado en la memoria y ya no será posible borrarlo ni utilizarlo.
Además de un error, este código también contiene otra práctica no muy buena: el código de una función utiliza simultáneamente punteros inteligentes y ordinarios. Por ejemplo, si los datostambién era un puntero inteligente, entonces ese error no ocurriría: si es necesario, cuando se sale del alcance, los punteros inteligentes llaman automáticamente al destructor del objeto almacenado.
Advertencia 2
V1062 La clase 'DfsState' define un operador 'nuevo' personalizado. También debe definirse el operador 'eliminar'. dfs-visit.h 62
// An FST state's DFS stack state.
template <class FST>
struct DfsState {
public:
....
void *operator new(size_t size,
MemoryPool<DfsState<FST>> *pool) {
return pool->Allocate();
}
....
}PVS-Studio no se detiene y continúa agregando nuevos diagnósticos. Este fragmento de código es un gran ejemplo para mostrar el trabajo de los últimos diagnósticos numerados V1062 .
La regla que supervisa este diagnóstico es simple: si define su propio operador 'nuevo', también debe definir su propio operador 'eliminar'. Lo contrario funciona de la misma manera: si define su propio 'eliminar', entonces también debe definir su propio 'nuevo'.
En el ejemplo anterior, se infringe esta regla: el objeto se creará usando el 'nuevo' definido por nosotros y se eliminará, usando el estándar 'eliminar'. Veamos qué hace la función Allocate de la clase MemoryPool ,que nuestro propio 'nuevo' llama:
void *Allocate() {
if (free_list_ == nullptr) {
auto *link = static_cast<Link *>(mem_arena_.Allocate(1));
link->next = nullptr;
return link;
} else {
auto *link = free_list_;
free_list_ = link->next;
return link;
}
}Esta función crea un elemento y lo agrega a la lista vinculada. Es lógico que tal asignación se haya escrito en su propio "nuevo".
¡Pero espere un minuto! Solo se incluyen unas pocas líneas debajo de esta función:
void Free(void *ptr) {
if (ptr) {
auto *link = static_cast<Link *>(ptr);
link->next = free_list_;
free_list_ = link;
}
}Esto significa que ya tenemos funciones listas para usar tanto para la asignación como para la liberación. Lo más probable es que el programador haya tenido que escribir su propio operador de 'eliminación', utilizando la función Free () para liberarlo .
El analizador detectó al menos tres errores más:
- V1062 La clase 'VectorState' define un operador 'nuevo' personalizado. También debe definirse el operador 'eliminar'. vector-fst.h 31
- V1062 La clase 'CacheState' define un operador 'nuevo' personalizado. También debe definirse el operador 'eliminar'. cache.h 65
Advertencia 3
V703 Es extraño que el campo 'first_path' en la clase derivada 'ShortestPathOptions' sobrescriba el campo en la clase base 'ShortestDistanceOptions'. Verifique las líneas: ruta más corta. H: 35, distancia más corta. H: 34. camino más corto h 35
// Base class
template <class Arc, class Queue, class ArcFilter>
struct ShortestDistanceOptions {
Queue *state_queue; // Queue discipline used; owned by caller.
ArcFilter arc_filter; // Arc filter (e.g., limit to only epsilon graph).
StateId source; // If kNoStateId, use the FST's initial state.
float delta; // Determines the degree of convergence required
bool first_path; // For a semiring with the path property (o.w.
// undefined), compute the shortest-distances along
// along the first path to a final state found
// by the algorithm. That path is the shortest-path
// only if the FST has a unique final state (or all
// the final states have the same final weight), the
// queue discipline is shortest-first and all the
// weights in the FST are between One() and Zero()
// according to NaturalLess.
ShortestDistanceOptions(Queue *state_queue, ArcFilter arc_filter,
StateId source = kNoStateId,
float delta = kShortestDelta)
: state_queue(state_queue),
arc_filter(arc_filter),
source(source),
delta(delta),
first_path(false) {}
};
// Derived class
template <class Arc, class Queue, class ArcFilter>
struct ShortestPathOptions
: public ShortestDistanceOptions<Arc, Queue, ArcFilter> {
using StateId = typename Arc::StateId;
using Weight = typename Arc::Weight;
int32 nshortest; // Returns n-shortest paths.
bool unique; // Only returns paths with distinct input strings.
bool has_distance; // Distance vector already contains the
// shortest distance from the initial state.
bool first_path; // Single shortest path stops after finding the first
// path to a final state; that path is the shortest path
// only when:
// (1) using the ShortestFirstQueue with all the weights
// in the FST being between One() and Zero() according to
// NaturalLess or when
// (2) using the NaturalAStarQueue with an admissible
// and consistent estimate.
Weight weight_threshold; // Pruning weight threshold.
StateId state_threshold; // Pruning state threshold.
ShortestPathOptions(Queue *queue, ArcFilter filter, int32 nshortest = 1,
bool unique = false, bool has_distance = false,
float delta = kShortestDelta, bool first_path = false,
Weight weight_threshold = Weight::Zero(),
StateId state_threshold = kNoStateId)
: ShortestDistanceOptions<Arc, Queue, ArcFilter>(queue, filter,
kNoStateId, delta),
nshortest(nshortest),
unique(unique),
has_distance(has_distance),
first_path(first_path),
weight_threshold(std::move(weight_threshold)),
state_threshold(state_threshold) {}
};De acuerdo, no es tan fácil encontrar un error potencial, ¿verdad?
El problema radica en el hecho de que tanto la clase base como la derivada contienen campos con el mismo nombre: first_path . Esto hará que la clase derivada tenga su propio campo diferente, que anula el campo de la clase base con su nombre. Tales errores pueden generar una gran confusión.
Para comprender mejor lo que quiero decir, propongo considerar un breve ejemplo sintético de nuestra documentación. Digamos que tenemos el siguiente código:
class U {
public:
int x;
};
class V : public U {
public:
int x; // <= V703 here
int z;
};Aquí, el nombre x se reemplaza dentro de la clase derivada. Ahora la pregunta es: ¿qué valor imprimirá el siguiente código?
int main() {
V vClass;
vClass.x = 1;
U *uClassPtr = &vClass;
std::cout << uClassPtr->x << std::endl;
....
}Si cree que se generará un valor indefinido, entonces tiene razón. En este ejemplo, la unidad se escribirá en el campo de la clase derivada, pero la lectura se realizará desde el campo de la clase base, que en el momento de la salida aún no está definido.
La superposición de nombres en la jerarquía de clases es un error potencial que debe evitarse :)
Advertencia 4
V1004 El puntero 'aiter' se usó de manera insegura después de que se verificó con nullptr. Verifique las líneas: 107, 119. visit.h 119
template <....>
void Visit(....)
{
....
// Deletes arc iterator if done.
auto *aiter = arc_iterator[state];
if ((aiter && aiter->Done()) || !visit) {
Destroy(aiter, &aiter_pool);
arc_iterator[state] = nullptr;
state_status[state] |= kArcIterDone;
}
// Dequeues state and marks black if done.
if (state_status[state] & kArcIterDone) {
queue->Dequeue();
visitor->FinishState(state);
state_status[state] = kBlackState;
continue;
}
const auto &arc = aiter->Value(); // <=
....
}El puntero aiter se usa después de que se haya verificado si hay nullptr . El analizador hace una suposición: si se comprueba un puntero en busca de nullptr , durante la comprobación puede tener ese valor.
En ese caso, veamos qué le sucede a aiter si realmente es igual a cero. Primero, este puntero se verificará en la declaración ' if ((aiter && aiter-> Done ()) ||! Visit) '. Esta condición será igual a falso , y no entraremos en la rama then de este if . Y luego, de acuerdo con todos los cánones de errores clásicos, un puntero nulo será desreferenciado: ' aiter-> Value ();'. Esta desreferenciación da como resultado un comportamiento indefinido.
Advertencia 5
El siguiente ejemplo contiene dos errores a la vez:
- V595 El puntero 'istrm' se utilizó antes de que se verificara con nullptr. Compruebe las líneas: 60, 61. mapped-file.cc 60
- V595 El puntero 'istrm' se utilizó antes de que se verificara con nullptr. Verifique las líneas: 39, 61. mapped-file.cc 39
MappedFile *MappedFile::Map(std::istream *istrm, bool memorymap,
const string &source, size_t size) {
const auto spos = istrm->tellg(); // <=
....
istrm->seekg(pos + size, std::ios::beg); // <=
if (istrm) { // <=
VLOG(1) << "mmap'ed region of " << size
<< " at offset " << pos
<< " from " << source
<< " to addr " << map;
return mmf.release();
}
....
}El error encontrado aquí es más claro que el error del ejemplo anterior. El puntero istrm se desreferencia primero (dos veces), y solo después de eso sigue una verificación de cero y un registro de errores. Esto indica claramente: si un puntero nulo llega a esta función como istrm , entonces se producirá un comportamiento indefinido (o, más probablemente, un bloqueo del programa) sin ningún registro. Trastorno ... errores como este no deben pasarse por alto.
Advertencia 6
V730 No todos los miembros de una clase se inicializan dentro del constructor. Considere inspeccionar: stones_written_. ersatz_progress.cc 14
ErsatzProgress::ErsatzProgress()
: current_(0)
, next_(std::numeric_limits<uint64_t>::max())
, complete_(next_)
, out_(NULL)
{}El analizador nos advierte que el constructor no inicializa todos los campos de la estructura ErzatzProgress . Comparemos este constructor con la lista de campos en esta estructura:
class ErsatzProgress {
....
private:
void Milestone();
uint64_t current_, next_, complete_;
unsigned char stones_written_;
std::ostream *out_;
};De hecho, puede ver que el constructor inicializa todos los campos excepto stones_written_ .
Nota : este ejemplo puede no ser un error. El error real solo ocurrirá cuando se use el valor de un campo no inicializado .
Sin embargo, los diagnósticos del V730 le ayudan a depurar dichos casos de uso por adelantado. Después de todo, surge una pregunta natural: si el programador decidió inicializar específicamente todos los campos de la clase, entonces ¿por qué debería tener una razón para dejar un campo sin valor?
Mi suposición de que el campo stones_written_ no se inicializó por error se confirmó cuando vi otro constructor unas líneas a continuación:
ErsatzProgress::ErsatzProgress(uint64_t complete,
std::ostream *to,
const std::string &message)
: current_(0)
, next_(complete / kWidth)
, complete_(complete)
, stones_written_(0)
, out_(to)
{
....
}Aquí se inicializan todos los campos de la clase, lo que confirma: el programador realmente planeó inicializar todos los campos, pero accidentalmente se olvidó de una cosa.
Advertencia 7
V780 El objeto '& params' de tipo no pasivo (no PDS) no se puede inicializar mediante la función memset. binary_format.cc 261
/* Not the best numbering system,
but it grew this way for historical reasons
* and I want to preserve existing binary files. */
typedef enum
{
PROBING=0,
REST_PROBING=1,
TRIE=2,
QUANT_TRIE=3,
ARRAY_TRIE=4,
QUANT_ARRAY_TRIE=5
}
ModelType;
....
struct FixedWidthParameters {
unsigned char order;
float probing_multiplier;
// What type of model is this?
ModelType model_type;
// Does the end of the file
// have the actual strings in the vocabulary?
bool has_vocabulary;
unsigned int search_version;
};
....
// Parameters stored in the header of a binary file.
struct Parameters {
FixedWidthParameters fixed;
std::vector<uint64_t> counts;
};
....
void BinaryFormat::FinishFile(....)
{
....
// header and vocab share the same mmap.
Parameters params = Parameters();
memset(¶ms, 0, sizeof(Parameters)); // <=
....
}Para comprender esta advertencia, sugiero que primero comprenda qué es un tipo de PDS. PDS son las siglas de Passive Data Structure, una estructura de datos simple. A veces, en lugar de "PDS", dicen "POD" - "Datos antiguos sin formato". En términos simples (estoy citando de la Wikipedia rusa ), el tipo PDS es un tipo de datos que tiene una disposición rígidamente definida de campos en la memoria que no requiere restricciones de acceso ni control automático. En pocas palabras, es un tipo de datos que consta solo de tipos integrados.
Una característica distintiva de los tipos de POD es que las variables de estos tipos se pueden cambiar y procesar utilizando funciones primitivas de administración de memoria (memset, memcpy, etc.). Sin embargo, esto no puede decirse de los tipos "no PDS": un manejo tan bajo de sus valores puede conducir a errores graves. Por ejemplo, pérdidas de memoria, doble vaciado del mismo recurso o comportamiento indefinido.
PVS-Studio emite una advertencia al código anterior: no puede manejar una estructura del tipo Parámetros de esta manera. Si observa la definición de esta estructura, puede ver que su segundo miembro es de tipo std :: vector... Este tipo utiliza activamente la gestión automática de la memoria y, además de los datos de contenido, almacena variables de servicio adicionales. Poner a cero un campo de este tipo con memset puede romper la lógica de la clase y es un error grave.
Advertencia 8
V575 El puntero nulo potencial se pasa a la función 'memcpy'. Inspeccione el primer argumento. Verifique las líneas: 73, 68.modelstate.cc 73
Metadata*
ModelState::decode_metadata(const DecoderState& state,
size_t num_results)
{
....
Metadata* ret = (Metadata*)malloc(sizeof(Metadata));
....
memcpy(ret, &metadata, sizeof(Metadata));
return ret;
}La siguiente advertencia nos dice que se está pasando un puntero nulo a la función memcpy . Sí, de hecho, si la función malloc no puede asignar memoria, devolverá NULL . En este caso, este puntero se pasará a la función memset , donde se eliminará la referencia y, en consecuencia, se bloqueará el programa encantador.
Sin embargo, algunos de nuestros lectores pueden estar indignados: si la memoria se desborda / fragmenta tanto que malloc no puede asignar memoria, ¿realmente importa lo que suceda después? El programa se bloqueará de todos modos, porque debido a la falta de memoria no podrá funcionar normalmente.
En repetidas ocasiones nos hemos encontrado con esta opinión y creemos que es incorrecta. Les diría en detalle por qué esto es realmente así, pero este tema merece un artículo aparte. Tan merecido que lo escribimos hace unos años :) Si se pregunta por qué siempre debería comprobar un puntero devuelto por funciones malloc , le invito a leer: ¿Por qué es importante comprobar qué devolvió malloc ?
Advertencia 9
La siguiente advertencia se debe a las mismas razones que la anterior. Es cierto que apunta a un error ligeramente diferente.
V769El puntero 'middle_begin_' en la expresión 'middle_begin_ + (count.size () - 2)' podría ser nullptr. En tal caso, el valor resultante no tendrá sentido y no debería utilizarse. Verifique las líneas: 553, 552. search_trie.cc 553
template <class Quant, class Bhiksha> class TrieSearch {
....
private:
....
Middle *middle_begin_, *middle_end_;
....
};
template <class Quant, class Bhiksha>
uint8_t *TrieSearch<Quant, Bhiksha>::SetupMemory(....)
{
....
middle_begin_
= static_cast<Middle*>(malloc(sizeof(Middle) * (counts.size() - 2)));
middle_end_ = middle_begin_ + (counts.size() - 2);
....
}Al igual que en el ejemplo anterior, la memoria se asigna aquí mediante la función malloc . El puntero devuelto, sin ninguna comprobación de nullptr, se utiliza en la expresión aritmética. Por desgracia, el resultado de tal expresión no tendrá ningún sentido y se almacenará un valor completamente inútil en el campo middle_end_ .
Advertencia 10
Y finalmente, el ejemplo más interesante en mi opinión se encontró en la biblioteca kenlm incluida en DeepSpeech:
V1061 La extensión del espacio de nombres 'std' puede resultar en un comportamiento indefinido. size_iterator.hh 210
// Dirty hack because g++ 4.6 at least wants
// to do a bunch of copy operations.
namespace std {
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdEl truco llamado "truco sucio" en el comentario es realmente sucio. El punto es que tal expansión del espacio de nombres estándar puede conducir a un comportamiento indefinido.
¿Por qué? Porque el contenido del espacio de nombres estándar lo determina exclusivamente el comité de estándares. Es por eso que el estándar internacional del lenguaje C ++ prohíbe explícitamente expandir std de esta manera.
El último estándar soportado en g ++ 4.6 es C ++ 03. Aquí hay una cita traducida del borrador de trabajo final de C ++ 03(vea el ítem 17.6.4.2.1): "El comportamiento de un programa C ++ no está definido si agrega declaraciones o definiciones al espacio de nombres estándar o al espacio de nombres anidado estándar, a menos que se especifique lo contrario". Esta cita se aplica a todos los estándares posteriores (C ++ 11, C ++ 14, C ++ 17 y C ++ 20).
Propongo considerar cómo podría solucionar el código problemático de nuestro ejemplo. La primera pregunta lógica: ¿cuáles son estos "casos para los que se indica lo contrario"? Hay varias situaciones en las que la expansión estándar no conduce a un comportamiento indefinido. Puede leer más sobre todas estas situaciones en la página de documentación para los diagnósticos V1061 , pero ahora es importante para nosotros que uno de estos casos sea la adición de una especialización de la plantilla de función.
Porquestd ya tiene una función llamada iter_swap (nota: una función de plantilla), es lógico suponer que el programador quería expandir sus capacidades para poder trabajar con el tipo util :: SizedIterator . Pero aquí está la mala suerte: en lugar de agregar especialización a la plantilla de función , el programador simplemente escribió una sobrecarga ordinaria . Debería haber sido escrito así:
namespace std {
template <>
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdSin embargo, este código tampoco es tan simple. El caso es que este código solo será válido hasta el estándar C ++ 20. Sí, también señaló que las especializaciones de las plantillas de funciones conducen a un comportamiento indefinido (consulte el borrador de trabajo final de C ++ 20 , sección 16.5.4.2.1). Y dado que este código pertenece a una biblioteca, es probable que tarde o temprano se compile con el indicador -std = C ++ 20 . Por cierto, PVS-Studio distingue qué versión del estándar se usa en el código y, dependiendo de esto, emite o no una advertencia. Véalo usted mismo: ejemplo para C ++ 17 , ejemplo para C ++ 20 .
De hecho, puede hacerlo mucho más fácilmente. Para corregir el error, solo necesita transferir su propia definición de iter_swapen el mismo espacio de nombres que define la clase SizedIterator . En este caso, en los lugares donde se llama a iter_swap , debe agregar "using std :: iter_swap;". Resulta así (la definición de la clase SizedIterator y la función util :: swap () se han cambiado por simplicidad):
namespace util
{
class SizedIterator
{
public:
SizedIterator(int i) : m_data(i) {}
int& operator*()
{
return m_data;
}
private:
int m_data;
};
....
inline void iter_swap(SizedIterator first,
SizedIterator second)
{
std::cout << "we are inside util::iter_swap" << std::endl;
swap(*first, *second);
}
}
int main()
{
double d1 = 1.1, d2 = 2.2;
double *pd1 = &d1, *pd2 = &d2;
util::SizedIterator si1(42), si2(43);
using std::iter_swap;
iter_swap(pd1, pd2);
iter_swap(si1, si2); // "we are inside util::iter_swap"
return 0;
}Ahora el compilador seleccionará de forma independiente la sobrecarga requerida de la función iter_swap basándose en una búsqueda de argumentos (ADL). Para la clase SizedIterator , se llamará a la versión del espacio de nombres util , y para otros tipos, se llamará a la versión del espacio de nombres std . La evidencia está en el enlace . Además, no es necesario agregar ningún "uso" dentro de las funciones de la biblioteca: dado que su código ya está dentro de std , el compilador seguirá eligiendo la sobrecarga correcta.
Y luego, listo, la función iter_swap personalizada funcionará como debería sin ningún "truco sucio" y otra brujería :)
Conclusión
Con esto concluye mi artículo. Espero que los errores que encontré te hayan resultado interesantes y que hayas aprendido algo nuevo y útil para ti. Si ha leído hasta este punto, le deseo sinceramente un código limpio y ordenado sin errores. ¡Deje que los errores pasen por alto sus proyectos!
PD : Creemos que es una mala práctica escribir su propio código en el espacio de nombres std. ¿Qué piensas? Espero sus respuestas en los comentarios.
Si desarrolla en C, C ++, C # o Java y, como yo, está interesado en el tema del análisis estático, le sugiero que pruebe PVS-Studio usted mismo. Puedes descargarlo en el enlace .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: George Gribkov. Verificación del código de DeepSpeech o por qué no debería escribir en el espacio de nombres std .