Überprüfen des offenen Telegrammnetzwerks mit PVS-Studio

Telegram Open Network (TON) ist eine Plattform des gleichen Teams, das den Telegram Messenger entwickelt hat. Neben der Blockchain bietet TON eine Vielzahl von Diensten. Die Entwickler haben kürzlich den in C ++ geschriebenen Code der Plattform öffentlich zugänglich gemacht und auf GitHub hochgeladen. Wir haben beschlossen, das Projekt vor seiner offiziellen Veröffentlichung zu überprüfen.

Einführung

Telegram Open Network ist eine Reihe verschiedener Dienste. Unter anderem bietet es ein eigenes Zahlungssystem, das auf der Gram-Kryptowährung basiert, und eine virtuelle Maschine namens TON VM, die intelligente Verträge ausführt. Es bietet auch einen Nachrichtendienst, TON Messages. Das gesamte Projekt wird als Gegenmaßnahme zur Internet-Zensur angesehen.

Das Projekt wurde mit CMake erstellt, daher hatte ich keine Schwierigkeiten, es zu erstellen und zu überprüfen. Der Quellcode ist in C ++ 14 geschrieben und läuft auf 210.000 LOC:

Da es sich um ein kleines und qualitativ hochwertiges Projekt handelt, gibt es nicht viele Fehler, aber sie sollten trotzdem behoben werden.

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-Diagnosemeldung: V601 Der Wert 'false' wird implizit in den Integer-Typ umgewandelt. mc-config.cpp 884

Es sieht so aus, als ob die Funktion hier den falschen Fehlerstatus zurückgibt. Die Funktion sollte anscheinend eher einen negativen Wert für Fehler als wahr / falsch zurückgeben. Das ist zumindest das, was es im Code weiter macht, wo es -1 zurückgibt.

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-Diagnosemeldung: V501 Links und rechts vom Operator '==' befinden sich identische Unterausdrücke: zero_state_id_ == zero_state_id_ LastBlock.cpp 66

TON folgt einem Codierungsstandard, der vorschreibt, dass die Namen der Klassenmitglieder mit einem Unterstrich enden sollen. In solchen Fällen kann diese Notation jedoch zu einem Fehler führen, da Sie Gefahr laufen, den Unterstrich zu übersehen. Der Name des an diese Funktion übergebenen Arguments ähnelt dem des Klassenmitglieds, wodurch es einfach ist, sie zu verwechseln. Dieses Argument sollte höchstwahrscheinlich am Vergleich teilnehmen.

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-Diagnosemeldung: V581 Die bedingten Ausdrücke der nebeneinander angeordneten if-Anweisungen sind identisch. Überprüfen Sie die Zeilen: 80, 84. blockdb.cpp 84

Die Bedingung im CHECK- Makro wird niemals ausgeführt, da sie bereits von der vorherigen if- Anweisung überprüft wurde.

Hier liegt noch ein weiterer Fehler vor: Das CHECK- Makro ist unsicher, da die darin enthaltene Bedingung nicht in ein do {...} while (0) -Konstrukt eingeschlossen ist. Eine solche Umhüllung ist erforderlich, um Kollisionen mit anderen Bedingungen im Zweig else zu vermeiden. Mit anderen Worten, der folgende Code würde nicht wie erwartet funktionieren:

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

Überprüfen einer vorzeichenbehafteten Variablen

 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-Diagnosemeldung: V560 Ein Teil des bedingten Ausdrucks ist immer falsch: last == 0x80. boc.cpp 78

Der zweite Teil der Bedingung wird niemals ausgeführt, da in diesem Fall der Typ char signiert ist. Wenn Sie einer Variablen vom Typ int einen Wert zuweisen, tritt eine Vorzeichenerweiterung auf, sodass ihre Werte weiterhin im Bereich [-128, 127] und nicht im Bereich [0, 256] liegen.

Es sollte beachtet werden, dass char nicht immer signiert ist: sein Verhalten ist plattform- und compilerabhängig. Theoretisch könnte die betreffende Bedingung also weiterhin ausgeführt werden, wenn auf einer anderen Plattform aufgebaut wird.

Bitweise Verschiebung einer negativen Zahl

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

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

Das Ausführen einer bitweisen Rechtsverschiebungsoperation für eine negative Zahl ist ein nicht spezifiziertes Verhalten: Es ist unmöglich, im Voraus zu wissen, ob das Vorzeichen erweitert oder mit Nullen aufgefüllt wird.

Nullprüfung nach neu

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

PVS-Studio-Diagnosemeldung: 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

Die Nachricht sagt alles: Wenn die Speicherzuordnung fehlschlägt, löst das Programm eine Ausnahme aus, anstatt einen Nullzeiger zurückzugeben. Dies bedeutet, dass die Überprüfung sinnlos ist.

Redundanter Scheck

 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-Diagnosemeldung: V547 Ausdruck 'Pfad' ist immer wahr. fift-main.cpp 136

Dieses Snippet stammt aus einem der internen Dienstprogramme des Projekts. Der ternäre Operator ist in diesem Fall redundant: Die Bedingung, die er überprüft, wird bereits von der vorherigen if- Anweisung überprüft. Es sieht so aus, als hätten die Entwickler vergessen, diesen ternären Operator zu entfernen, als sie beschlossen, die Verwendung von Standardpfaden zu verwerfen (diese werden zumindest in der Hilfemeldung nicht erwähnt).

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-Diagnosemeldung: V1001 Die Variable 'tmp_info' wird zugewiesen, aber am Ende der Funktion nicht verwendet. analyzer.cpp 140

Die Entwickler wollten anscheinend eine Variable namens tmp_info in der letzten Zeile dieser Funktion verwenden. Hier ist der Code derselben 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)); } 

Größer oder kleiner als?

 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-Diagnosemeldung: V1037 Zwei oder mehr Fallzweige führen dieselben Aktionen aus. Überprüfen Sie die Zeilen: 639, 645 builtins.cpp 639

Wenn Sie sorgfältig lesen, haben Sie festgestellt, dass diesem Code eine <= -Operation fehlt. In der Tat sollte sich Fall 6 mit dieser Operation befassen. Wir können daraus schließen, indem wir uns zwei Stellen ansehen. Der erste ist der Initialisierungscode:

 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)); .... } 

Wie Sie sehen, enthält die Funktion define_builtins einen Aufruf compile_cmp_int für den Operator <= , wobei der Parameter mode auf 6 gesetzt ist.

Der zweite Punkt ist die Funktion compile_cmp_int selbst, die die Namen der Operationen auflistet:

 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); } 

Index 6 entspricht dem LEQ- Wort, was "weniger oder gleich" bedeutet.

Es ist ein weiterer netter Fehler der Klasse von Fehlern, die in Vergleichsfunktionen gefunden werden .

Verschiedenes

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

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

Das VM_LOG_IMPL- Makro ist unsicher. Sein zweiter Parameter ist nicht in Klammern eingeschlossen, was möglicherweise unerwünschte Nebenwirkungen verursachen kann, wenn ein komplexer Ausdruck an die Bedingung übergeben wird. Wenn die Maske jedoch nur eine Konstante ist, wird dieser Code ohne Probleme ausgeführt. Nichts hindert Sie jedoch daran, etwas anderes an das Makro zu übergeben.

Fazit

TON erwies sich als ziemlich klein, so dass es dort nur wenige Fehler gibt, für die das Telegramm-Entwicklerteam auf jeden Fall Anerkennung finden sollte. Aber jeder macht ab und zu Fehler, auch diese Leute. Codeanalysatoren sind leistungsstarke Tools, mit denen gefährliche Stellen im Quellcode bereits in der frühen Entwicklungsphase erkannt werden können. Vernachlässigen Sie sie daher nicht. Die statische Analyse soll nicht von Zeit zu Zeit ausgeführt werden, sondern sollte Teil des Entwicklungsprozesses sein: " Statische Analyse in den Prozess einführen, nicht nur nach Fehlern suchen ".