Introdução
Procuramos repetidamente por bugs em projetos que usam aprendizado de máquina, e DeepSpeech não foi exceção para nós. Não é surpreendente, porque este projeto é bastante popular: no momento em que este livro foi escrito, ele já tinha mais de 15 mil estrelas no GitHub.
Como de costume, a busca de erros que citarei neste artigo foi realizada utilizando o analisador de código estático PVS-Studio.
Para seu trabalho, o DeepSpeech usa a biblioteca TensorFlow. Desativei a análise do código desta biblioteca, pois já escrevemos um artigo separado sobre ela., entretanto, não desativei a análise do restante das bibliotecas utilizadas. Qual é a razão para isto? Erros em qualquer biblioteca incluída em seu projeto também se tornam erros em seu projeto. Portanto, é útil analisar não apenas o seu, mas também qualquer código de terceiros que você usar. Você pode ler uma opinião detalhada sobre isso em nosso artigo recente .
Isso conclui a breve introdução - é hora de passar para a análise de erros. A propósito, se você veio aqui para descobrir a resposta à pergunta que fiz no título do artigo (por que você não deve escrever no namespace std), pode imediatamente olhar para o final do artigo. Um exemplo particularmente interessante espera por você lá!
Visão geral de 10 avisos interessantes emitidos pelo analisador
Aviso 1
V773 A função foi encerrada sem liberar o ponteiro de 'dados'. Um vazamento de memória é possível. 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 trecho de código contém um exemplo clássico de vazamento de memória: a função Read chama ' return nullptr ' sem liberar a memória alocada com a expressão ' new EditFstData '. Com essa saída da função (sem chamar delete data ), apenas o próprio ponteiro será excluído, e o destruidor do objeto para o qual ele aponta não será chamado. Assim, o objeto continuará armazenado na memória, não sendo mais possível apagá-lo ou utilizá-lo.
Além de um erro, esse código também contém outra prática não muito boa: o código de uma função usa ponteiros inteligentes e comuns simultaneamente. Por exemplo, se dadostambém fosse um ponteiro inteligente, então tal erro não ocorreria: se necessário, ao sair do escopo, os ponteiros inteligentes chamam automaticamente o destruidor do objeto armazenado.
Aviso 2
V1062 A classe 'DfsState' define um operador 'novo' personalizado. O operador 'delete' também deve ser definido. 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 não pára e continua a adicionar novos diagnósticos. Este trecho de código é um ótimo exemplo para mostrar o trabalho do último diagnóstico numerado V1062 .
A regra de que este diagnóstico monitora é simples: se você definir seu próprio 'novo' operador, também deverá definir seu próprio operador 'excluir'. O oposto funciona da mesma maneira: se você definir seu próprio 'deletar', então o seu próprio 'novo' também deve ser definido.
No exemplo acima, esta regra é violada: o objeto será criado usando o 'novo' definido por nós, e excluído - usando o padrão 'excluir'. Vamos ver o que a função Allocate da classe MemoryPool faz ,que nosso próprio 'novo' chama:
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 função cria um item e o adiciona à lista vinculada. É lógico que tal alocação deveria ter sido escrita em seu próprio 'novo'.
Mas espere um minuto! Apenas algumas linhas abaixo contêm a seguinte função:
void Free(void *ptr) {
if (ptr) {
auto *link = static_cast<Link *>(ptr);
link->next = free_list_;
free_list_ = link;
}
}Isso significa que já temos funções prontas para alocação e liberação. Muito provavelmente, o programador teve que escrever seu próprio operador 'delete', usando a função Free () para liberá-lo .
O analisador detectou pelo menos mais três desses erros:
- V1062 A classe 'VectorState' define um operador 'novo' personalizado. O operador 'delete' também deve ser definido. vector-fst.h 31
- V1062 A classe 'CacheState' define um operador 'novo' personalizado. O operador 'delete' também deve ser definido. cache.h 65
Aviso 3
V703 É estranho que o campo 'first_path' na classe derivada 'ShortestPathOptions' substitua o campo na classe base 'ShortestDistanceOptions'. Verifique as linhas: shortest-path.h: 35, shortest-distance.h: 34. caminho mais curto.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) {}
};Concordo, não é tão fácil encontrar um erro potencial, certo?
O problema é que tanto a classe base quanto a classe derivada contêm campos com o mesmo nome: first_path . Isso fará com que a classe derivada tenha seu próprio campo diferente, que substitui o campo da classe base com seu nome. Esses erros podem causar sérias confusões.
Para entender melhor o que quero dizer, proponho considerar um pequeno exemplo sintético de nossa documentação. Digamos que temos o seguinte código:
class U {
public:
int x;
};
class V : public U {
public:
int x; // <= V703 here
int z;
};Aqui, o nome x é substituído dentro da classe derivada. Agora a questão é: qual valor o código a seguir imprimirá?
int main() {
V vClass;
vClass.x = 1;
U *uClassPtr = &vClass;
std::cout << uClassPtr->x << std::endl;
....
}Se você acha que um valor indefinido será gerado, você está certo. Neste exemplo, a unidade será gravada no campo da classe derivada, mas a leitura será feita a partir do campo da classe base, que no momento da saída ainda está indefinida.
A sobreposição de nomes na hierarquia de classes é um erro potencial que deve ser evitado :)
Aviso 4
V1004 O ponteiro 'aiter' foi usado sem segurança após ter sido verificado em relação a nullptr. Verifique as linhas: 107, 119. visite.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(); // <=
....
}O ponteiro aiter é usado após a verificação de nullptr . O analisador faz uma suposição: se um ponteiro é verificado para nullptr , então durante a verificação ele pode ter esse valor.
Nesse caso, vamos ver o que acontece com aiter se realmente for igual a zero. Primeiro, este ponteiro será verificado na instrução ' if ((aiter && aiter-> Done ()) ||! Visit) '. Essa condição será falsa e não entraremos no ramo desse if . E então, de acordo com todos os cânones dos erros clássicos, um ponteiro nulo será desreferenciado: ' aiter-> Value ();'. Este desreferenciamento resulta em comportamento indefinido.
Aviso 5
O exemplo a seguir contém dois erros de uma vez:
- V595 O ponteiro 'istrm' foi utilizado antes de ser verificado em relação a nullptr. Verifique as linhas: 60, 61. mapped-file.cc 60
- V595 O ponteiro 'istrm' foi utilizado antes de ser verificado em relação a nullptr. Verifique as linhas: 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();
}
....
}O erro encontrado aqui é mais claro do que o erro do exemplo anterior. O ponteiro istrm é primeiro desreferenciado (duas vezes), e somente depois disso é feita uma verificação de zero e um registro de erros. Isso indica claramente: se um ponteiro nulo vier para essa função como istrm , um comportamento indefinido (ou, mais provavelmente, uma falha do programa) ocorrerá sem nenhum registro. Desordem ... bugs como este não devem ser esquecidos.
Aviso 6
V730 Nem todos os membros de uma classe são inicializados dentro do construtor. Considere inspecionar: stones_written_. ersatz_progress.cc 14
ErsatzProgress::ErsatzProgress()
: current_(0)
, next_(std::numeric_limits<uint64_t>::max())
, complete_(next_)
, out_(NULL)
{}O analisador nos avisa que o construtor não inicializa todos os campos da estrutura ErzatzProgress . Vamos comparar este construtor com a lista de campos nesta estrutura:
class ErsatzProgress {
....
private:
void Milestone();
uint64_t current_, next_, complete_;
unsigned char stones_written_;
std::ostream *out_;
};Na verdade, você pode ver que o construtor inicializa todos os campos, exceto stones_written_ .
Nota : Este exemplo pode não ser um erro. O erro real só acontecerá quando o valor de um campo não inicializado for usado .
No entanto, o diagnóstico do V730 ajuda a depurar esses casos de uso com antecedência. Afinal, surge uma questão natural: se o programador decidiu inicializar especificamente todos os campos da classe, então por que deveria ter um motivo para deixar um campo sem valor?
Meu palpite de que o campo stones_written_ não foi inicializado por engano foi confirmado quando algumas linhas abaixo eu vi outro construtor:
ErsatzProgress::ErsatzProgress(uint64_t complete,
std::ostream *to,
const std::string &message)
: current_(0)
, next_(complete / kWidth)
, complete_(complete)
, stones_written_(0)
, out_(to)
{
....
}Aqui todos os campos da classe são inicializados, o que confirma: o programador realmente planejou inicializar todos os campos, mas acidentalmente esqueceu de uma coisa.
Aviso 7
V780 O objeto '& params' de um tipo não passivo (não PDS) não pode ser inicializado usando a função 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 entender esse aviso, sugiro que você primeiro entenda o que é um tipo de PDS. PDS significa Passive Data Structure, uma estrutura de dados simples. Às vezes, em vez de "PDS", eles dizem "POD" - "Plain Old Data". Em termos simples (cito a Wikipedia russa ), o tipo PDS é um tipo de dados que tem uma localização de campos rigidamente definida na memória, que não requer restrições de acesso e controle automático. Simplificando, é um tipo de dados que contém apenas tipos internos.
Uma característica distinta dos tipos POD é que as variáveis desses tipos podem ser alteradas e processadas usando funções primitivas de gerenciamento de memória (memset, memcpy e assim por diante). No entanto, isso não pode ser dito sobre os tipos "não-PDS": esse tratamento de baixo nível de seus valores pode levar a erros graves. Por exemplo, vazamentos de memória, liberação dupla do mesmo recurso ou comportamento indefinido.
PVS-Studio emite um aviso para o código acima: você não pode manipular uma estrutura do tipo Parâmetros desta forma. Se você olhar a definição desta estrutura, você pode ver que seu segundo membro é do tipo std :: vector... Esse tipo usa ativamente o gerenciamento automático de memória e, além dos dados de conteúdo, armazena variáveis de serviço adicionais. Zerar tal campo usando memset pode quebrar a lógica da classe e é um erro sério.
Aviso 8
V575 O ponteiro nulo em potencial é passado para a função 'memcpy'. Inspecione o primeiro argumento. Verifique as linhas: 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;
}O próximo aviso nos diz que um ponteiro nulo está sendo passado para a função memcpy . Sim, de fato, se a função malloc falhar em alocar memória, ela retornará NULL . Neste caso, este ponteiro será passado para a função memset , onde será desreferenciado - e, conseqüentemente, um travamento de programa encantador.
No entanto, alguns de nossos leitores podem ficar indignados: se a memória está sobrecarregada / fragmentada a ponto de malloc não conseguir alocar memória, realmente importa o que acontece a seguir? O programa irá travar de qualquer maneira, porque devido à falta de memória, ele não poderá funcionar normalmente.
Repetidamente nos deparamos com essa opinião e acreditamos que ela é incorreta. Eu diria em detalhes por que isso é realmente assim, mas este tópico merece um artigo separado. Muito merecedora de termos escrito há alguns anos :) Se você está se perguntando por que sempre deve verificar um ponteiro retornado pelas funções malloc , convido você a ler: Por que é importante verificar o que malloc retornou .
Aviso 9
O seguinte aviso é causado pelos mesmos motivos que o anterior. É verdade que aponta para um erro ligeiramente diferente.
V769O ponteiro 'middle_begin_' na expressão 'middle_begin_ + (counts.size () - 2)' pode ser nullptr. Nesse caso, o valor resultante não fará sentido e não deve ser usado. Verifique as linhas: 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);
....
}Assim como no exemplo anterior, a memória é alocada aqui usando a função malloc . O ponteiro retornado, sem nenhuma verificação de nullptr, é usado na expressão aritmética. Infelizmente, o resultado de tal expressão não fará nenhum sentido, e um valor completamente inútil será armazenado no campo middle_end_ .
Aviso 10
Bem, finalmente, o exemplo mais interessante na minha opinião foi encontrado na biblioteca kenlm incluída no DeepSpeech:
V1061 Estender o namespace 'std' pode resultar em um comportamento 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 stdO truque chamado "truque sujo" no comentário é muito sujo. A questão é que essa expansão do namespace std pode levar a um comportamento indefinido.
Por quê? Porque o conteúdo do namespace std é determinado exclusivamente pelo comitê de padrões. É por isso que o padrão internacional da linguagem C ++ proíbe explicitamente a expansão de std dessa forma.
O último padrão compatível com g ++ 4.6 é C ++ 03. Aqui está uma citação traduzida do esboço de trabalho final do C ++ 03(ver item 17.6.4.2.1): "O comportamento de um programa C ++ é indefinido se adicionar declarações ou definições ao namespace std ou ao namespace aninhado em std, a menos que especificado de outra forma." Esta citação se aplica a todos os padrões subsequentes (C ++ 11, C ++ 14, C ++ 17 e C ++ 20).
Proponho considerar como você poderia corrigir o código problemático do nosso exemplo. A primeira pergunta lógica: quais são esses "casos para os quais o oposto é indicado"? Existem várias situações em que a expansão padrão não leva a um comportamento indefinido. Você pode ler mais sobre todas essas situações na página de documentação para diagnósticos do V1061 , mas agora é importante para nós que um desses casos é a adição de uma especialização do modelo de função.
Porquestd já tem uma função chamada iter_swap (nota: uma função de template), é lógico supor que o programador queria expandir suas capacidades para que pudesse trabalhar com o tipo util :: SizedIterator . Mas aqui está o azar: em vez de adicionar especialização ao modelo de função , o programador simplesmente escreveu uma sobrecarga comum . Deveria ter sido escrito assim:
namespace std {
template <>
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdNo entanto, esse código também não é tão simples. A questão é que esse código só será válido até o padrão C ++ 20. Sim, ele também observou as especializações dos modelos de função como levando a um comportamento indefinido (consulte o esboço de trabalho final do C ++ 20 , seção 16.5.4.2.1). E como esse código pertence a uma biblioteca, é provável que mais cedo ou mais tarde ele seja compilado com o sinalizador -std = C ++ 20 . A propósito, o PVS-Studio distingue qual versão do padrão é usada no código e, dependendo disso, emite ou não emite um aviso. Veja você mesmo: exemplo para C ++ 17 , exemplo para C ++ 20 .
Na verdade, você pode fazer muito mais fácil. Para corrigir o erro, você só precisa transferir sua própria definição de iter_swappara o mesmo namespace que define a classe SizedIterator . Neste caso, nos locais onde iter_swap é chamado , você precisa adicionar "using std :: iter_swap;". Acontece assim (a definição da classe SizedIterator e a função util :: swap () foram alteradas para simplificar):
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;
}Agora, o compilador selecionará independentemente a sobrecarga necessária da função iter_swap com base em uma pesquisa de argumento (ADL). Para a classe SizedIterator , a versão do namespace util será chamada , e para outros tipos, a versão do namespace std será chamada . A evidência está no link . Além disso, não há necessidade de adicionar qualquer "uso" dentro das funções da biblioteca: como seu código já está dentro de std , o compilador ainda escolherá a sobrecarga correta.
E então - voila - a função iter_swap personalizada funcionará como deveria, sem quaisquer "truques sujos" e outra bruxaria :)
Conclusão
Isso conclui meu artigo. Espero que os erros que encontrei tenham sido interessantes para você e que tenha aprendido algo novo e útil para si mesmo. Se você leu até este ponto, desejo sinceramente um código limpo e organizado, sem erros. Deixe os bugs contornar seus projetos!
PS Achamos que é uma má prática escrever seu próprio código no namespace std. O que você acha? Aguardo suas respostas nos comentários.
Se você está desenvolvendo em C, C ++, C # ou Java e, como eu, está interessado no tópico de análise estática, então sugiro que experimente o PVS-Studio. Você pode baixá-lo no link .
Se você quiser compartilhar este artigo com um público que fala inglês, use o link de tradução: George Gribkov. Verificando o código de DeepSpeech ou por que você não deve escrever no namespace std .