Cirka hver sjette måned skriver nogen til os fra Yandex-virksomhedens ansatte, spørger om licensering af PVS-Studio, downloader prøven og forsvinder. Det er normalt, vi vænnede os til en langsommelig proces med at sælge vores analysator til store virksomheder. Men når jeg først har en mulighed, ville det ikke være en ekstra ting at sige hej til Yandex-udviklere og minde om PVS-Studio-værktøjet.
Helt ærligt viste artiklen sig at være tilfældig i mange henseender. Vi er allerede blevet tilbudt at tjekke ClickHouse, men på en eller anden måde blev denne idé glemt. Forleden dag, mens jeg surfede på internettet, mødte jeg igen omtalen af ClickHouse og blev interesseret i projektet. Denne gang besluttede jeg ikke at udsætte og tjekke dette projekt ud.
ClickHouse
ClickHouse er en kolonnedatabase til OLAP (online analytical request processing). ClickHouse blev designet i Yandex for at imødekomme udfordringerne fra Yandex.Metrica. ClickHouse giver dig mulighed for at udføre analytiske anmodninger på opdaterede data i realtid. Det lineært skalerbare system er i stand til at arbejde både med billioner af poster og petabytes af data. I juni 2016 blev ClickHouse udgivet i open source under Apache-licensen 2.0.
- Websted:clickhouse.yandex.
- Side i Wikipedia:ClickHouse.
- Repository på GitHub.com-webstedet:yandex/ClickHouse.
Analyse af projekt ved hjælp af PVS-Studio
Jeg tjekkede ClickHouse-kildekoden taget fra repository af 14. august 2017. For at teste brugte jeg betaversionen af PVS-Studio v6.17. På det tidspunkt, vi publicerede artiklen, er denne version allerede blevet frigivet.
Følgende mapper blev udelukket fra kontrollen:
- Klikhus/bidrag
- ClickHouse/libs
- ClickHouse/build
- forskellige tests blev også udelukket, for eksempel ClickHouse/dbms/src/Common/tests
Størrelsen på resten af kildekoden i C++ er 213 KLOC. Samtidig er 7,9 % af linjer kommentarer. Det viser sig, at størrelsen på selve koden, der er blevet kontrolleret, ikke er så stor:omkring 196 KLOC.
Som du kan se, har ClickHouse-projektet en lille størrelse. Udover det er kvaliteten af koden enestående høj, og jeg vil ikke være i stand til at skrive en chokerende artikel. I alt udstedte analysatoren 130 advarsler (generel analyse, høje og mellemstore advarsler).
Jeg er ikke sikker på antallet af falske positiver. Der er mange advarsler, som formelt ikke kan betegnes som falske, men samtidig er der ingen praktisk brug i dem. Den nemmeste måde at forklare det på er at give et eksempel.
int format_version;
....
if (format_version < 1 || format_version > 4)
throw Exception("Bad checksums format version: " + ....);
if (format_version == 1) return false;
if (format_version == 2) return read_v2(in);
if (format_version == 3) return read_v3(in);
if (format_version == 4) return read_v4(in);
return false;Analyzer gør opmærksom på, at hvis udtrykket (format_version ==4) begynder at evaluere, så vil det altid være sandt. Som du kan se, er der et tjek ovenfor, at hvis en værdi format_version går ud over [1..4], så kastes der en undtagelse. Operatøren returnerer falsk vil aldrig blive henrettet.
Formelt har analysatoren ret, og det er ikke klart, hvordan man beviser, at det er en falsk positiv. På den anden side er det indlysende, at denne kode er korrekt og blot er skrevet med en "sikkerhedsmargen".
I sådanne tilfælde kan en programmør undertrykke analysatoradvarslerne på forskellige måder eller omskrive koden. Du kan f.eks. skrive som følger:
switch(format_version)
{
case 1: return false;
case 2: return read_v2(in);
case 3: return read_v3(in);
case 4: return read_v4(in);
default:
throw Exception("Bad checksums format version: " + ....);
}Der er nogle advarsler om, at jeg bare ikke kan sige, om de påpeger en fejl eller ej. Jeg er ikke bekendt med projektet og har ingen idé om, hvordan nogle kodefragmenter skal køres. Lad os overveje en sådan sag.
Der er en vis rækkevidde med 3 funktioner:
namespace CurrentMemoryTracker
{
void alloc(Int64 size);
void realloc(Int64 old_size, Int64 new_size);
void free(Int64 size);
}Navnene på formelle argumenter for funktioner antyder, at nogle størrelser overføres til funktionerne. Nogle tilfælde er mistænkelige for analysatoren. For eksempel, når størrelsen af en markør, men ikke størrelsen af en struktur, overføres til alloc funktion.
using Large = HyperLogLogCounter<K, Hash, UInt32, DenominatorType>;
Large * large = nullptr;
....
CurrentMemoryTracker::alloc(sizeof(large));Analysatoren ved ikke, om det er en fejl eller ej. Jeg ved det heller ikke, men efter min mening er denne kode mistænkelig.
Nå, jeg vil ikke skrive om sådanne sager. Hvis ClickHouse-udviklere er interesserede, kan de selv tjekke projektet og udforske listen over advarsler mere detaljeret. Jeg vil i artiklen kun gennemgå de kodefragmenter, der forekom mig de mest interessante.
Interessante kodefragmenter
1. CWE-476:NULL Pointer Dereference (3 fejl)
bool executeForNullThenElse(....)
{
....
const ColumnUInt8 * cond_col =
typeid_cast<const ColumnUInt8 *>(arg_cond.column.get());
....
if (cond_col)
{
....
}
else if (cond_const_col)
{
....
}
else
throw Exception(
"Illegal column " + cond_col->getName() + // <=
" of first argument of function " + getName() +
". Must be ColumnUInt8 or ColumnConstUInt8.",
ErrorCodes::ILLEGAL_COLUMN);
....
}PVS-Studio advarsel:V522 Dereference af nul-markøren 'cond_col' kan finde sted. FunctionsConditional.h 765
Her håndteres situationen forkert, når der opstår en fejl. I stedet for at kaste en undtagelse, vil der forekomme nul pointer dereference.
For at oprette en fejlmeddelelse sker funktionskaldet:cond_col->getName() . Du kan ikke gøre dette, fordi cond_col markøren vil være nul.
En lignende fejl findes her:V522 Dereference af nul-markøren 'cond_col' kan finde sted. FunctionsConditional.h 1061
Lad os overveje en anden variant af spørgsmålet om at bruge en null-pointer:
void processHigherOrderFunction(....)
{
....
const DataTypeExpression * lambda_type =
typeid_cast<const DataTypeExpression *>(types[i].get());
const DataTypes & lambda_argument_types =
lambda_type->getArgumentTypes();
if (!lambda_type)
throw Exception("Logical error: .....",
ErrorCodes::LOGICAL_ERROR);
....
}PVS-Studio advarsel:V595 'lambda_type' pointeren blev brugt før den blev verificeret mod nullptr. Tjek linjer:359, 361. TypeAndConstantInference.cpp 359
I begyndelsen lambda_type pointeren dereferences, og først derefter tjekker den. For at rette koden skal du flytte markøren højere op:
if (!lambda_type)
throw Exception("Logical error: .....",
ErrorCodes::LOGICAL_ERROR);
const DataTypes & lambda_argument_types =
lambda_type->getArgumentTypes();2. CWE-665:Ukorrekt initialisering (1 fejl)
struct TryResult
{
....
explicit TryResult(Entry entry_)
: entry(std::move(entry)) // <=
, is_usable(true)
, is_up_to_date(true)
{
}
....
Entry entry;
....
}V546 Medlem af en klasse initialiseres af sig selv:'entry(entry)'. PoolWithFailoverBase.h 74
På grund af tastefejl, indtastning medlem initialiserer sig selv, og som et resultat forbliver det faktisk uinitialiseret. For at rette koden skal du tilføje understregningssymbolet korrekt:
: entry(std::move(entry_))3. CWE-672:Operation på en ressource efter udløb eller frigivelse (1 fejl)
using Strings = std::vector<std::string>;
....
int mainEntryClickhousePerformanceTest(int argc, char ** argv)
{
....
Strings input_files;
....
for (const String filename : input_files) // <=
{
FS::path file(filename);
if (!FS::exists(file))
throw DB::Exception(....);
if (FS::is_directory(file))
{
input_files.erase( // <=
std::remove(input_files.begin(), // <=
input_files.end(), // <=
filename) , // <=
input_files.end() ); // <=
getFilesFromDir(file, input_files, recursive);
}
else
{
if (file.extension().string() != ".xml")
throw DB::Exception(....);
}
}
....
}PVS-Studio-advarsel:V789 Iteratorer for 'input_files'-beholderen, der bruges i den områdebaserede for loop, bliver ugyldige ved kald af 'slette'-funktionen. PerformanceTest.cpp 1471
Input_filer container bruges i rækkevidde-baseret for loop. På samme tid, inde i løkken, kan beholderen variere på grund af fjernelse af nogle elementer. Hvis det ikke er meget klart for en læser, hvorfor du ikke kan gøre det, foreslår jeg at læse beskrivelsen af diagnostik V789.
4. CWE-563:Tildeling til variabel uden brug ('Uused Variable') (1 fejl)
struct StringRange
{
const char * first;
const char * second;
....
StringRange(TokenIterator token_begin, TokenIterator token_end)
{
if (token_begin == token_end)
{
first = token_begin->begin; // <=
second = token_begin->begin; // <=
}
TokenIterator token_last = token_end;
--token_last;
first = token_begin->begin; // <=
second = token_last->end; // <=
}
};Analysatoren udsender to advarsler:
- V519 Den 'første' variabel tildeles værdier to gange i træk. Måske er dette en fejl. Tjek linjer:26, 33. StringRange.h 33
- V519 Den 'anden' variabel tildeles værdier to gange i træk. Måske er dette en fejl. Tjek linjer:27, 34. StringRange.h 34
Når en bestemt tilstand i begyndelsen først og sekund variabler tildeles til token_begin->begin værdi. Længere fremme er værdien af disse variable under alle omstændigheder ved at ændre sig igen. Mest sandsynligt indeholder denne kode en logisk fejl, eller noget mangler. For eksempel retur operatør kan blive glemt:
if (token_begin == token_end)
{
first = token_begin->begin;
second = token_begin->begin;
return;
}5. CWE-570:Udtrykket er altid falsk (2 fejl)
DataTypePtr
getReturnTypeImpl(const DataTypes & arguments) const override
{
....
if (!((.....))
|| ((left_is_string || left_is_fixed_string) && (.....))
|| (left_is_date && right_is_date)
|| (left_is_date && right_is_string)
|| (left_is_string && right_is_date)
|| (left_is_date_time && right_is_date_time) // 1
|| (left_is_date_time && right_is_string) // 1
|| (left_is_string && right_is_date_time) // 1
|| (left_is_date_time && right_is_date_time) // 2
|| (left_is_date_time && right_is_string) // 2
|| (left_is_string && right_is_date_time) // 2
|| (left_is_uuid && right_is_uuid)
|| (left_is_uuid && right_is_string)
|| (left_is_string && right_is_uuid)
|| (left_is_enum && right_is_enum && .....)
|| (left_is_enum && right_is_string)
|| (left_is_string && right_is_enum)
|| (left_tuple && right_tuple && .....)
|| (arguments[0]->equals(*arguments[1]))))
throw Exception(....);
....
}I denne tilstand gentages tre underudtryk to gange. PVS-Studio advarsler:
- V501 Instantiate FunctionComparison
- V501 Instantiate Function Comparison
- V501 Instantiate Function Comparison
Der er to muligheder. For det første er der ingen fejl, betingelsen er simpelthen overflødig og kan forenkles. Den anden - der er en fejl her, og nogle betingelser er ikke kontrolleret. Under alle omstændigheder bør forfatterne kontrollere dette kodefragment.
Lad os se på et andet tilfælde, hvor en betingelse altid er falsk.
static void ipv6_scan(const char * src, unsigned char * dst)
{
....
uint16_t val{};
unsigned char * colonp = nullptr;
while (const auto ch = *src++)
{
const auto num = unhex(ch);
if (num != -1)
{
val <<= 4;
val |= num;
if (val > 0xffffu) // <=
return clear_dst();
saw_xdigit = 1;
continue;
}
....
}PVS-Studio advarsel:V547 Udtryk 'val> 0xffffu' er altid falsk. Værdiområdet for kort type uden fortegn:[0, 65535]. FunctionsCoding.h 339
Når du analyserer en streng, der indeholder en IPv6-adresse, vil nogle ugyldige IPv6-adresser blive taget som korrekte. Det forventes, at der kan optages tal mellem separatorerne i hexadecimalt format, med en værdi på mindre end FFFF. Hvis tallet er større, må adressen anses for forkert. For at identificere denne situation i kode er der en test "if (val> 0xffffu) ". Men det virker ikke. Val variabel er af uint16_t type, hvilket betyder, at den ikke kan være større end 0xFFFF. Som følge heraf vil funktionen "sluge" den forkerte adresse. Som en almindelig del af adressen vil 4 sidste hexadecimale tal før separatoren blive repræsenteret.
6. CWE-571. Udtrykket er altid sandt (1 fejl)
static void formatIP(UInt32 ip, char *& out)
{
char * begin = out;
for (auto i = 0; i < 3; ++i)
*(out++) = 'x';
for (size_t offset = 8; offset <= 24; offset += 8)
{
if (offset > 0) // <=
*(out++) = '.';
/// Get the next byte.
UInt32 value = (ip >> offset) & static_cast<UInt32>(255);
/// Faster than sprintf.
if (value == 0)
{
*(out++) = '0';
}
else
{
while (value > 0)
{
*(out++) = '0' + value % 10;
value /= 10;
}
}
}
/// And reverse.
std::reverse(begin, out);
*(out++) = '\0';
}PVS-Studio advarsel:V547 Udtryk 'offset> 0' er altid sandt. FunctionsCoding.h 649
"offset > 0 " betingelse udføres altid, derfor tilføjes punktet altid. Det forekommer mig, at der ikke er nogen fejl, og en kontrol er bare overflødig. Selvom jeg selvfølgelig ikke er sikker. Hvis det ikke var en fejl, skulle en kontrol slettes, så det ikke ville forvirre andre programmører og statiske kodeanalysatorer.
Konklusion
Måske vil projektudviklere også være i stand til at finde en række fejl ved at kigge gennem analysatoradvarslerne, som blev afspejlet i artiklen. Jeg vil gerne afslutte en historiefortælling, især da jeg havde nok af materiale til at "give hilsen".
Generelt vil jeg gerne bemærke den høje kvalitet af koden for ClickHouse-projektudviklere. Men selv højt dygtige udviklere er ikke immune over for fejl, og denne artikel beviser det igen. PVS-Studio statisk kodeanalysator hjælper med at forhindre mange fejl. Den største effekt fra statisk analyse får udviklere, når de skriver ny kode. Det giver ingen mening at bruge tid på at fejlfinde fejl, der kan opdages af analysatoren umiddelbart efter kontrol af ny kode.
Jeg inviterer jer alle til at downloade og prøve PVS-Studio.