Überprüfung des Telegramm-Open-Network-Codes durch den PVS-Studio-Analysator

Telegram Open Network (TON) ist eine Plattform der Entwickler des Telegram Messenger, die neben der Blockchain eine Vielzahl von Diensten enthält. Entwickler haben kürzlich in C ++ geschriebenen Plattformcode veröffentlicht und auf GitHub veröffentlicht. Wir wollten das Projekt vor seinem offiziellen Start überprüfen.

Einführung

Telegram Open Network ist eine Reihe verschiedener Dienste. Das Projekt verfügt beispielsweise über ein eigenes Zahlungssystem, das auf der Gram-Kryptowährung basiert. Außerdem gibt es eine virtuelle TON VM-Maschine, an der intelligente Verträge arbeiten. Es gibt einen Nachrichtendienst - TON-Nachrichten. Das Projekt selbst zielt darauf ab, die Internet-Zensur zu bekämpfen.

Das Projekt verwendet das CMake-Montagesystem, sodass bei der Montage und Überprüfung keine besonderen Probleme auftraten. Der Code ist in C ++ 14 geschrieben und hat 210.000 Zeilen:

Das Projekt ist klein und von hoher Qualität, daher gab es nicht so viele Fehler, aber sie verdienen Aufmerksamkeit und Korrektur.

Rückkehrcode

static int process_workchain_shard_hashes(....) { .... if (f == 1) { if ((shard.shard & 1) || cs.size_ext() != 0x20000) { return false; // <= } .... int r = process_workchain_shard_hashes(....); if (r < 0) { return r; } .... return cb.store_bool_bool(true) && cb.store_ref_bool(std::move(left)) && cb.store_ref_bool(std::move(right)) && cb.finalize_to(branch) ? r : -1; .... } 

PVS-Studio Warnung: V601 Der Wert 'false' wird implizit in den Integer-Typ umgewandelt. mc-config.cpp 884

Es scheint, dass die Entwickler zu diesem Zeitpunkt den zurückgegebenen Fehlerstatus durcheinander gebracht haben. Anscheinend sollte die Funktion im Fehlerfall einen negativen Wert zurückgeben und nicht wahr / falsch. Zumindest ist es die Rückgabe von -1, die im folgenden Code beobachtet werden kann.

Eine Variable mit sich selbst vergleichen

 class LastBlock : public td::actor::Actor { .... ton::ZeroStateIdExt zero_state_id_; .... }; void LastBlock::update_zero_state(ton::ZeroStateIdExt zero_state_id) { .... if (zero_state_id_ == zero_state_id_) { return; } LOG(FATAL) << ....; } 

PVS-Studio Warnung: V501 Links und rechts vom Operator '==' befinden sich identische Unterausdrücke: zero_state_id_ == zero_state_id_ LastBlock.cpp 66

TON-Code wird unter Verwendung eines Codierungsstandards geschrieben, in dem Klassenmitglieder mit einem Unterstrich am Ende benannt werden. Ein solches Schreiben kann jedoch wie in diesem Fall unbemerkt bleiben und zu einem Fehler führen. Der Parameter, der in diese Funktion eingeht, hat einen ähnlichen Namen wie ein Mitglied der Klasse, sodass sie leicht zu verwechseln sind. Höchstwahrscheinlich war er es, der am Vergleich teilnehmen sollte.

Unsicheres Makro

 namespace td { namespace detail { [[noreturn]] void process_check_error(const char *message, const char *file, int line); } // namespace detail } #define CHECK(condition) \ if (!(condition)) { \ ::td::detail::process_check_error(#condition, __FILE__, __LINE__); \ } void BlockDb::get_block_handle(BlockIdExt id, ....) { if (!id.is_valid()) { promise.set_error(....); return; } CHECK(id.is_valid()); // <= .... } 

PVS-Studio Warnung: V581 Die bedingten Ausdrücke der nebeneinander angeordneten ' if' -Anweisungen sind identisch. Überprüfen Sie die Zeilen: 80, 84. blockdb.cpp 84

Die Bedingung innerhalb des CHECK- Makros wird niemals erfüllt, weil seine Prüfung war bereits in der vorherigen if .

Es gibt noch einen weiteren Fehler: Das CHECK- Makro ist unsicher, weil Die darin enthaltene Bedingung wird nicht in ein do {...} while (0) -Konstrukt eingeschlossen. Dies dient dazu, Kollisionen mit anderen Bedingungen in anderen zu vermeiden. Mit anderen Worten, dieser Code funktioniert nicht wie erwartet:

 if (X) CHECK(condition) else foo(); 

Validierung der Vorzeichenvariablen

 class Slice { .... char operator[](size_t i) const; .... }; td::Result<int> CellSerializationInfo::get_bits(td::Slice cell) const { .... int last = cell[data_offset + data_len - 1]; if (!last || last == 0x80) { // <= return td::Status::Error("overlong encoding"); } .... } 

PVS-Studio- Warnung : V560 Ein Teil des bedingten Ausdrucks ist immer falsch: last == 0x80. boc.cpp 78

Der zweite Teil der Bedingung wird niemals erfüllt sein, weil Typ char hat in diesem Fall ein Vorzeichen. Wenn Sie einer Variablen vom Typ int einen Wert zuweisen, wird das Vorzeichen erweitert, sodass der Wertebereich der Variablen weiterhin im Grenzwert [-128, 127] und nicht in [0, 256] bleibt.

Es ist zu beachten, dass der char- Typ nicht immer signiert wird - dieses Verhalten hängt von der Plattform und dem Compiler ab. Diese Bedingung kann also theoretisch beim Zusammenbau auf einer anderen Plattform noch erfüllt werden.

Negative Bit Bitverschiebung

 template <class Tr> bool AnyIntView<Tr>::export_bits_any(....) const { .... int mask = (-0x100 >> offs) & 0xff; .... } 

PVS-Studio Warnung: V610 Nicht angegebenes Verhalten. Überprüfen Sie den Schaltoperator '>>'. Der linke Operand '-0x100' ist negativ. bigint.hpp 1925

Die bitweise Verschiebung einer negativen Zahl nach rechts ist ein nicht spezifiziertes Verhalten: Es ist nicht bekannt, wie sich das Vorzeichen in diesem Fall verhält - wird es erweitert oder mit Nullen gefüllt?

Nach neu auf null prüfen

 CellBuilder* CellBuilder::make_copy() const { CellBuilder* c = new CellBuilder(); if (!c) { // <= throw CellWriteError(); } .... } 

PVS-Studio Warnung: V668 Es macht keinen Sinn, den Zeiger 'c' gegen null zu testen, da der Speicher mit dem Operator 'new' zugewiesen wurde. Die Ausnahme wird bei einem Speicherzuordnungsfehler generiert. CellBuilder.cpp 531

Aus der Warnung des Analysators geht alles hervor. Bei nicht erfolgreicher Speicherzuweisung wird eine Ausnahme ausgelöst und kein Nullzeiger zurückgegeben. Überprüfung macht keinen Sinn.

Erneut prüfen

 int main(int argc, char* const argv[]) { .... if (!no_env) { const char* path = std::getenv("FIFTPATH"); if (path) { parse_include_path_set(path ? path : "/usr/lib/fift", source_include_path); } } .... } 

PVS-Studio Warnung: V547 Ausdruck 'Pfad' ist immer wahr. fift-main.cpp 136

Dieser Code wurde in einem der internen Dienstprogramme übernommen. Der ternäre Operator ist in diesem Fall überflüssig: Die darin eingecheckte Bedingung existiert bereits im vorherigen if . Höchstwahrscheinlich haben sie vergessen, den ternären Operator zu entfernen, als sie die Standardpfade verlassen wollten (zumindest wurde in der Hilfemeldung nichts über sie gesagt).

Nicht verwendete Variable

 bool Op::set_var_info_except(const VarDescrList& new_var_info, const std::vector<var_idx_t>& var_list) { if (!var_list.size()) { return set_var_info(new_var_info); } VarDescrList tmp_info{new_var_info}; tmp_info -= var_list; return set_var_info(new_var_info); // <= } 

PVS-Studio Warnung: V1001 Die Variable 'tmp_info' wird zugewiesen, aber am Ende der Funktion nicht verwendet. analyzer.cpp 140

Anscheinend war in der letzten Zeile dieser Funktion geplant, die Variable tmp_info zu verwenden. Hier ist der Code für dieselbe Funktion, jedoch mit anderen Parameterspezifizierern:

 bool Op::set_var_info_except(VarDescrList&& new_var_info, const std::vector<var_idx_t>& var_list) { if (var_list.size()) { new_var_info -= var_list; // <= } return set_var_info(std::move(new_var_info)); } 

Mehr oder weniger?

 int compute_compare(const VarDescr& x, const VarDescr& y, int mode) { switch (mode) { case 1: // > return x.always_greater(y) ? 1 : (x.always_leq(y) ? 2 : 3); case 2: // = return x.always_equal(y) ? 1 : (x.always_neq(y) ? 2 : 3); case 3: // >= return x.always_geq(y) ? 1 : (x.always_less(y) ? 2 : 3); case 4: // < return x.always_less(y) ? 1 : (x.always_geq(y) ? 2 : 3); case 5: // <> return x.always_neq(y) ? 1 : (x.always_equal(y) ? 2 : 3); case 6: // >= return x.always_geq(y) ? 1 : (x.always_less(y) ? 2 : 3); case 7: // <=> return x.always_less(y) ? 1 : (x.always_equal(y) ? 2 : (x.always_greater(y) ? 4 : (x.always_leq(y) ? 3 : (x.always_geq(y) ? 6 : (x.always_neq(y) ? 5 : 7))))); default: return 7; } } 

PVS-Studio Warnung: V1037 Zwei oder mehr Fallzweige führen dieselben Aktionen aus. Überprüfen Sie die Zeilen: 639, 645 builtins.cpp 639

Aufmerksame Leser haben möglicherweise bemerkt, dass die Operation <= in diesem Code fehlt. Und tatsächlich sollte es Nummer 6 sein. Dies kann an zwei Stellen gefunden werden. Das erste ist die Initialisierung:

 AsmOp compile_cmp_int(std::vector<VarDescr>& res, std::vector<VarDescr>& args, int mode) { .... if (x.is_int_const() && y.is_int_const()) { r.set_const(compute_compare(x.int_const, y.int_const, mode)); x.unused(); y.unused(); return push_const(r.int_const); } int v = compute_compare(x, y, mode); .... } void define_builtins() { .... define_builtin_func("_==_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 2)); define_builtin_func("_!=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 5)); define_builtin_func("_<_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 4)); define_builtin_func("_>_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 1)); define_builtin_func("_<=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 6)); define_builtin_func("_>=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 3)); define_builtin_func("_<=>_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 7)); .... } 

In der Funktion define_builtins sehen Sie den Aufruf compile_cmp_int für <= mit dem Modusparameter gleich 6.

Nun, die zweite ist dieselbe Funktion compile_cmp_int , die die Namen der Operationen enthält:

 AsmOp compile_cmp_int(std::vector<VarDescr>& res, std::vector<VarDescr>& args, int mode) { .... static const char* cmp_names[] = {"", "GREATER", "EQUAL", "GEQ", "LESS", "NEQ", "LEQ", "CMP"}; .... return exec_op(cmp_names[mode], 2); } 

Unter dem Index 6 steht das Wort LEQ , was weniger oder gleich bedeutet.

Ein weiterer schöner Fehler im Zusammenhang mit der Fehlerklasse in Vergleichsfunktionen .

Andere

 #define VM_LOG_IMPL(st, mask) \ LOG_IMPL_FULL(get_log_interface(st), ...., VERBOSITY_NAME(DEBUG), \ (get_log_mask(st) & mask) != 0, "") // <= 

PVS-Studio Warnung: V1003 Das Makro 'VM_LOG_IMPL' ist ein gefährlicher Ausdruck. Der Parameter 'Maske' muss in Klammern stehen. log.h 23

Das Makro VM_LOG_IMPL ist unsicher. Der zweite Parameter ist nicht in Klammern gesetzt. Dies kann zu Nebenwirkungen führen, wenn ein komplexer Ausdruck an die Bedingung übergeben wird. Wenn mask nur eine Konstante ist, gibt es keine Probleme mit diesem Code, aber nichts hindert uns daran, etwas anderes an das Makro zu übergeben.

Abschließend

Der TON-Code stellte sich als nicht so groß heraus und es gibt nicht viele Fehler darin. Dafür sollten Sie natürlich den Programmierern des Telegrammteams Tribut zollen. Aber auch sie sind nicht immun gegen Fehler. Der Code-Analysator ist ein leistungsstarkes Tool, das in der Lage ist, gefährliche Stellen in einem frühen Stadium zu finden, selbst in hochwertigen Codebasen. Daher sollte seine Verwendung nicht vernachlässigt werden. Die statische Analyse ist kein Werkzeug für einmalige Überprüfungen, sondern Teil des Entwicklungsprozesses: " Betten Sie die statische Analyse in den Prozess ein und suchen Sie nicht nach Fehlern ."

Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Sergey Larin. Überprüfen des offenen Telegrammnetzwerks mit PVS-Studio .