Es wurde uns angeboten, den PVS-Studio-Analysator zu verwenden, um die Sammlung offener PMDK-Bibliotheken zu überprüfen, die zum Entwickeln und Debuggen von Anwendungen mit Unterstützung für nichtflüchtigen Speicher bestimmt sind. Warum eigentlich nicht? Darüber hinaus ist dies ein kleines Projekt in C und C ++ mit einer Gesamtgröße der Codebasis von ca. 170 KLOC ohne Kommentare. Dies bedeutet, dass die Überprüfung der Analyseergebnisse nicht viel Zeit und Mühe kostet. Lass uns gehen.
Zur Analyse des Quellcodes wird das PVS-Studio-Tool Version 7.08 verwendet. Die Leser unseres Blogs sind natürlich schon lange mit unserem Tool vertraut, und ich werde nicht weiter darauf eingehen. Für diejenigen, die zum ersten Mal zu uns gekommen sind, empfehle ich den Artikel " Wie kann man schnell interessante Warnungen anzeigen, die der PVS-Studio-Analysator für C- und C ++ - Code ausgibt? " Und eine kostenlose Testversion des Analysators zu testen.
Dieses Mal werde ich einen Blick in das PMDK-Projekt werfen und Sie über die Fehler und Mängel informieren, die mir aufgefallen sind. In meinem inneren Gefühl gab es nicht viele von ihnen, was auf die gute Qualität des Codes dieses Projekts hinweist. Von den interessanten Dingen können wir feststellen, dass mehrere Fragmente des falschen Codes gefunden wurden, was dennoch korrekt funktioniert :). Was ich meine, wird durch weitere Erzählungen klarer.
Zusammenfassend ist das PMDK eine Sammlung von Open-Source-Bibliotheken und -Tools, die die Entwicklung, das Debuggen und die Verwaltung nichtflüchtiger speicherfähiger Anwendungen vereinfachen sollen. Weitere Details hier: PMDK Einführung . Quellen hier: pmdk .
Mal sehen, welche Fehler und Mängel ich darin finden kann. Ich muss sofort sagen, dass ich bei der Analyse des Berichts alles andere als aufmerksam war und viel übersehen haben könnte. Daher fordere ich die Autoren des Projekts dringend auf, sich bei der Behebung von Fehlern nicht ausschließlich von diesem Artikel leiten zu lassen, sondern den Code selbst zu überprüfen. Und um einen Artikel zu schreiben, reicht mir das, was ich geschrieben habe, wenn ich mir die Liste der Warnungen ansehe :).
Falscher Code, der funktioniert
Speicherzuordnungsgröße
Es ist nicht ungewöhnlich, dass Programmierer Zeit damit verbringen, Code zu debuggen, wenn sich das Programm nicht so verhält, wie es sollte. Manchmal gibt es jedoch Situationen, in denen das Programm ordnungsgemäß funktioniert, der Code jedoch einen Fehler enthält. Der Programmierer hat einfach Glück und der Fehler manifestiert sich nicht. Im PMDK-Projekt bin ich auf mehrere solche interessanten Situationen gleichzeitig gestoßen und habe mich daher entschlossen, sie in einem separaten Kapitel zusammenzufassen.
int main(int argc, char *argv[])
{
....
struct pool *pop = malloc(sizeof(pop));
....
}PVS-Studio-Warnung: V568 Es ist seltsam, dass der Operator 'sizeof ()' die Größe eines Zeigers auf eine Klasse auswertet, nicht jedoch die Größe des Klassenobjekts 'pop'. util_ctl.c 717
Ein klassischer Tippfehler, bei dem die falsche Speichermenge zugewiesen wird. Der Operator sizeof gibt nicht die Größe der Struktur zurück, sondern die Größe des Zeigers auf diese Struktur. Die richtige Option wäre:
struct pool *pop = malloc(sizeof(pool));oder
struct pool *pop = malloc(sizeof(*pop));Dieser falsch geschriebene Code funktioniert jedoch hervorragend. Der Punkt ist, dass die Poolstruktur genau einen Zeiger enthält:
struct pool {
struct ctl *ctl;
};Es stellt sich heraus, dass die Struktur genau so viel einnimmt wie der Zeiger. Alles ist gut.
Linienlänge
Fahren wir mit dem nächsten Fall fort, in dem der Fehler erneut mit dem Operator sizeof gemacht wurde .
typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,
unsigned flags);
static const char *initial_state = "No code.";
static int
test_rwx_prot_map_priv_do_execute(const struct test_case *tc,
int argc, char *argv[])
{
....
char *addr_map = pmem2_map_get_address(map);
map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);
....
}PVS-Studio-Warnung: V579 [CWE-687] Die Funktion memcpy_fn empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das dritte Argument. pmem2_map_prot.c 513
Ein Zeiger auf eine spezielle Kopierfunktion wird zum Kopieren einer Zeichenfolge verwendet. Achten Sie auf den Aufruf dieser Funktion oder vielmehr auf ihr drittes Argument.
Der Programmierer geht davon aus, dass der Operator sizeof die Größe des Zeichenfolgenliteral berechnet. Tatsächlich wird die Größe des Zeigers jedoch erneut berechnet.
Glücklicherweise besteht die Zeichenfolge aus 8 Zeichen und ihre Größe entspricht der des Zeigers beim Erstellen einer 64-Bit-Anwendung. Infolgedessen alle 8 Zeichen der Zeichenfolge "Kein Code". wird erfolgreich kopiert.
In der Tat ist die Situation noch komplizierter und interessanter. Die Interpretation dieses Fehlers hängt davon ab, ob Sie Terminal Null kopieren möchten oder nicht. Betrachten wir zwei Szenarien.
Szenario 1. Es war erforderlich, Terminal Null zu kopieren. Dann irre ich mich, und das ist überhaupt kein harmloser Fehler, der sich nicht manifestiert. Es wurden nicht 9 Bytes kopiert, sondern nur 8. Es gibt keine terminale Null, und die Konsequenzen können nicht vorhergesagt werden. In diesem Fall kann der Code korrigiert werden, indem die Definition der konstanten Zeichenfolge initial_state wie folgt geändert wird :
static const char initial_state [] = "No code.";Jetzt ist der Wert für sizeof (initial_state) 9.
Szenario 2. Terminal Null ist überhaupt nicht erforderlich. Im Folgenden sehen Sie beispielsweise die folgende Codezeile:
UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);Wie Sie sehen können, gibt die strlen- Funktion den Wert 8 zurück und Terminal Null wird nicht in den Vergleich einbezogen. Dann ist es wirklich Glück und alles ist gut.
Bitverschiebung
Das nächste Beispiel bezieht sich auf eine bitweise Verschiebungsoperation.
static int
clo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr)
{
....
uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);
....
}PVS-Studio-Warnung: V610 [CWE-758] Nicht angegebenes Verhalten. Überprüfen Sie den Schaltoperator '>>'. Der linke Operand '~ 0' ist negativ. clo.cpp 205
Das Ergebnis der Verschiebung eines negativen Werts nach rechts hängt von der Compiler-Implementierung ab. Obwohl dieser Code unter allen derzeit vorhandenen Anwendungskompilierungsmodi möglicherweise korrekt und erwartungsgemäß funktioniert, ist er dennoch ein Glücksfall.
Priorität der Operationen
Betrachten Sie den letzten Fall im Zusammenhang mit der Priorität von Operationen.
#define BTT_CREATE_DEF_SIZE (20 * 1UL << 20) /* 20 MB */PVS-Studio-Warnung: V634 [CWE-783] Die Priorität der Operation '*' ist höher als die der Operation '<<'. Es ist möglich, dass im Ausdruck Klammern verwendet werden. bttcreate.c 204
Um eine Konstante von 20 MB zu erhalten, hat der Programmierer Folgendes beschlossen:
- 1 um 20 Bit verschoben, um den Wert 1048576 zu erhalten, d.h. 1 MB.
- 1 MB mit 20 multipliziert.
Mit anderen Worten, der Programmierer glaubt, dass die Berechnungen wie folgt ausgeführt werden: (20 * (1UL << 20))
Tatsächlich ist die Priorität des Multiplikationsoperators jedoch höher als die des Verschiebungsoperators, und der Ausdruck wird wie folgt bewertet: ((20 * 1UL) << 20).
Einverstanden, der Programmierer wollte kaum, dass der Ausdruck in einer solchen Reihenfolge ausgewertet wird. Es macht keinen Sinn, 20 mit 1 zu multiplizieren. Dies ist also der Fall, wenn der Code nicht so funktioniert, wie es der Programmierer beabsichtigt hat.
Dieser Fehler wird sich jedoch in keiner Weise manifestieren. Es spielt keine Rolle, wie Sie schreiben:
- (20 * 1UL << 20)
- (20 * (1UL << 20))
- ((20 * 1UL) << 20)
Das Ergebnis ist immer das gleiche ! Der gewünschte Wert ist immer 20971520 und das Programm funktioniert einwandfrei.
Andere Fehler
Klammern am falschen Ort
#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004
static void
enum_handles(int op)
{
....
NTSTATUS status;
while ((status = NtQuerySystemInformation(
SystemExtendedHandleInformation,
hndl_info, hi_size, &req_size)
== STATUS_INFO_LENGTH_MISMATCH)) {
hi_size = req_size + 4096;
hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,
hi_size);
}
UT_ASSERT(status >= 0);
....
}PVS-Studio-Warnung: V593 [CWE-783] Überprüfen Sie den Ausdruck der Art 'A = B == C'. Der Ausdruck wird wie folgt berechnet: 'A = (B == C)'. ut.c 641
Schauen Sie hier genau hin:
while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))Der Programmierer wollte im speichern Status den variablen Wert , dass die NtQuerySystemInformation Funktion zurückkehrt , und es dann mit einem konstanten zu vergleichen.
Der Programmierer wusste höchstwahrscheinlich, dass der Vergleichsoperator (==) eine höhere Priorität als der Zuweisungsoperator (=) hat, weshalb Klammern verwendet werden sollten. Aber er versiegelte sich und legte sie an die falsche Stelle. Klammern helfen daher in keiner Weise. Richtiger Code:
while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)Aufgrund dieses Fehlers funktioniert das UT_ASSERT- Makro niemals. Tatsächlich ist die Statusvariable enthält immer das Ergebnis des Vergleichs, das heißt, falsch (0) oder wahr (1). Daher ist die Bedingung ([0..1]> = 0) immer wahr.
Möglicher Speicherverlust
static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
char *input = strdup(in);
if (!input)
return POCLI_ERR_MALLOC;
if (!oidp)
return POCLI_ERR_PARS;
....
}PVS-Studio-Warnung: V773 [CWE-401] Die Funktion wurde beendet, ohne den 'Eingabe'-Zeiger loszulassen. Ein Speicherverlust ist möglich. pmemobjcli.c 238
Wenn sich oidp als Nullzeiger herausstellt, geht die Kopie der Zeichenfolge verloren, die durch Aufrufen der Funktion strdup erstellt wurde . Am besten verschieben Sie die Prüfung, bevor Sie Speicher zuweisen:
static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
if (!oidp)
return POCLI_ERR_PARS;
char *input = strdup(in);
if (!input)
return POCLI_ERR_MALLOC;
....
}Oder Sie können den Speicher explizit freigeben:
static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
char *input = strdup(in);
if (!input)
return POCLI_ERR_MALLOC;
if (!oidp)
{
free(input);
return POCLI_ERR_PARS;
}
....
}Möglicher Überlauf
typedef long long os_off_t;
void
do_memcpy(...., int dest_off, ....., size_t mapped_len, .....)
{
....
LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);
....
}PVS-Studio-Warnung: V1028 [CWE-190] Möglicher Überlauf. Betrachten Sie das Casting von Operanden, nicht das Ergebnis. memcpy_common.c 62 Es macht keinen Sinn,
das Ergebnis der Addition explizit in den Typ os_off_t umzuwandeln . Erstens schützt es nicht vor einem möglichen Überlauf, der beim Hinzufügen von zwei int- Werten auftreten kann . Zweitens wäre das Ergebnis der Addition implizit perfekt auf den Typ os_off_t erweitert worden . Explizites Casting ist einfach redundant.
Ich denke, es wäre richtiger, so zu schreiben:
LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);Hier wird ein vorzeichenloser Wert vom Typ size_t in einen vorzeichenbehafteten Wert konvertiert (so dass keine Warnung vom Compiler erfolgt). Gleichzeitig kommt es beim Hinzufügen definitiv nicht zu einem Überlauf.
Falscher Überlaufschutz
static DWORD
get_rel_wait(const struct timespec *abstime)
{
struct __timeb64 t;
_ftime64_s(&t);
time_t now_ms = t.time * 1000 + t.millitm;
time_t ms = (time_t)(abstime->tv_sec * 1000 +
abstime->tv_nsec / 1000000);
DWORD rel_wait = (DWORD)(ms - now_ms);
return rel_wait < 0 ? 0 : rel_wait;
}PVS-Studio-Warnung: V547 [CWE-570] Der Ausdruck 'rel_wait <0' ist immer falsch. Der vorzeichenlose Typwert ist niemals <0. Os_thread_windows.c 359
Ich bin mir nicht ganz sicher , vor welcher Situation die Prüfung schützen soll, aber es funktioniert trotzdem nicht. Die Variable rel_wait ist vom Typ DWORD ohne Vorzeichen . Dies bedeutet, dass der Vergleich rel_wait <0 nicht sinnvoll ist, da das Ergebnis immer wahr ist.
Fehlende Überprüfung, ob der Speicher erfolgreich zugewiesen wurde
Die Überprüfung der Speicherzuweisung erfolgt mithilfe von Assert- Makros , die nichts tun, wenn die Release-Version der Anwendung kompiliert wird. So können wir sagen , dass es keinen Umgang mit der Situation ist , wo malloc Funktionen zurückgeben NULL . Beispiel:
static void
remove_extra_node(TOID(struct tree_map_node) *node)
{
....
unsigned char *new_key = (unsigned char *)malloc(new_key_size);
assert(new_key != NULL);
memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);
....
}PVS-Studio-Warnung: V575 [CWE-628] Der potenzielle Nullzeiger wird an die Funktion 'memcpy' übergeben. Überprüfen Sie das erste Argument. Überprüfen Sie die Zeilen: 340, 338. rtree_map.c 340
An anderer Stelle gibt es nicht einmal eine Behauptung :
static void
calc_pi_mt(void)
{
....
HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);
for (i = 0; i < pending; ++i) {
workers[i] = CreateThread(NULL, 0, calc_pi,
&tasks[i], 0, NULL);
if (workers[i] == NULL)
break;
}
....
}PVS-Studio-Warnung: V522 [CWE-690] Möglicherweise wird ein potenzieller Nullzeiger "Arbeiter" dereferenziert. Kontrollzeilen: 126, 124. S. 126
Ich habe mindestens 37 solcher Codefragmente gezählt. Ich sehe also keinen Grund, alle im Artikel aufzulisten.
Auf den ersten Blick kann das Fehlen von Überprüfungen als schlampig angesehen werden und sagen, dass dies ein Code mit einem Geruch ist. Ich stimme dieser Position nicht zu. Programmierer unterschätzen die Gefahr, solche Überprüfungen nicht durchzuführen. Ein Nullzeiger manifestiert sich nicht unbedingt sofort als Programmabsturz, wenn versucht wird, ihn zu dereferenzieren. Die Folgen können bizarrer und gefährlicher sein, insbesondere bei Multithread-Programmen. Um genauer zu verstehen, was los ist und warum Überprüfungen erforderlich sind, empfehle ich jedem, den Artikel zu lesen. "Warum ist es wichtig zu überprüfen, was die Malloc-Funktion zurückgegeben hat? "
Geruchscode
Rufen Sie CloseHandle doppelt auf
static void
prepare_map(struct pmem2_map **map_ptr,
struct pmem2_config *cfg, struct pmem2_source *src)
{
....
HANDLE mh = CreateFileMapping(....);
....
UT_ASSERTne(CloseHandle(mh), 0);
....
}PVS-Studio-Warnung: V586 [CWE-675] Die Funktion 'CloseHandle' wird zweimal aufgerufen, um die Zuordnung derselben Ressource aufzuheben. pmem2_map.c 76
Wenn man sich diesen Code und die PVS-Studio-Warnung ansieht , ist klar, dass nichts klar ist. Wo kann ich CloseHandle erneut anrufen ? Um die Antwort zu finden, schauen wir uns die Implementierung des UT_ASSERTne- Makros an .
#define UT_ASSERTne(lhs, rhs)\
do {\
/* See comment in UT_ASSERT. */\
if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\
UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\
UT_ASSERTne_rt(lhs, rhs);\
} while (0)Es wurde nicht viel klarer. Was ist UT_ASSERT_COMPILE_ERROR_ON ? Was ist UT_ASSERTne_rt ?
Ich werde den Artikel nicht mit einer Beschreibung jedes Makros überladen und den Leser quälen, wodurch er gezwungen wird, ein Makro in ein anderes in seinem Kopf einzufügen. Lassen Sie uns sofort die endgültige Version des geöffneten Codes sehen, der aus der vorverarbeiteten Datei stammt.
do {
if (0 && 0) (void)((CloseHandle(mh)) != (0));
((void)(((CloseHandle(mh)) != (0)) ||
(ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",
"CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",
(unsigned long long)(0)), 0))); } while (0);Entfernen wir die immer falsche Bedingung (0 && 0) und im Allgemeinen alles, was irrelevant ist. Es stellt sich heraus:
((void)(((CloseHandle(mh)) != (0)) ||
(ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",
....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));Der Griff ist geschlossen. Wenn ein Fehler auftritt, wird eine Debug-Meldung generiert. Um den Fehlercode erneut abzurufen , wird CloseHandle für dasselbe ungültige Handle aufgerufen .
Fehler, irgendwie und nein. Da das Handle ungültig ist, ist es in Ordnung, dass die CloseHandle- Funktion zweimal dafür aufgerufen wird . Dieser Code ist jedoch geruchlos. Es wäre ideologisch korrekter, die Funktion nur einmal aufzurufen und den zurückgegebenen Status zu speichern, um dann bei Bedarf ihren Wert in der Nachricht anzuzeigen.
Nicht übereinstimmende Implementierungsschnittstelle (Entfernen von const)
static int
status_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question)
{
....
} else {
status_msg_info_and_question(st->msg); // <=
st->question = question;
ppc->result = CHECK_RESULT_ASK_QUESTIONS;
st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;
PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);
}
....
}Der Analysator zeigt die folgende Meldung an: V530 [CWE-252] Der Rückgabewert der Funktion 'status_msg_info_and_question' muss verwendet werden. check_util.c 293
Der Grund dafür ist, dass die Funktion status_msg_info_and_question aus Sicht des Analysators den Status von Objekten außerhalb des Analysators, einschließlich der übergebenen konstanten Zeichenfolge, nicht ändert. Jene. Die Funktion berechnet nur etwas und gibt das Ergebnis zurück. Und wenn ja, dann ist es seltsam, das Ergebnis, das diese Funktion zurückgibt, nicht zu verwenden. Und obwohl der Analysator diesmal falsch ist, zeigt er auf einen Code mit einem Geruch. Mal sehen, wie die aufgerufene Funktion status_msg_info_and_question funktioniert .
static inline int
status_msg_info_and_question(const char *msg)
{
char *sep = strchr(msg, MSG_SEPARATOR);
if (sep) {
*sep = ' ';
return 0;
}
return -1;
}Beim Aufrufen der Funktion strchr wird die Konstante implizit entfernt. Tatsache ist, dass es in C folgendermaßen deklariert ist:
char * strchr ( const char *, int );Nicht die beste Lösung. Aber die C-Sprache ist was sie ist :).
Der Analysator war verwirrt und verstand nicht, dass sich die übergebene Zeichenfolge tatsächlich ändert. Wenn ja, dann ist der Rückgabewert nicht das Wichtigste und Sie können ihn nicht verwenden.
Obwohl der Analysator verwirrt ist, zeigt er auf einen Code mit einem Geruch. Was den Parser verwirrt, kann auch die Person verwirren, die den Code verwaltet. Es wäre besser, die Funktion ehrlicher zu deklarieren, indem Sie const entfernen :
static inline int
status_msg_info_and_question(char *msg)
{
char *sep = strchr(msg, MSG_SEPARATOR);
if (sep) {
*sep = ' ';
return 0;
}
return -1;
}Die Absichten sind also sofort klarer und der Analysator schweigt.
Überkomplizierter Code
static struct memory_block
heap_coalesce(struct palloc_heap *heap,
const struct memory_block *blocks[], int n)
{
struct memory_block ret = MEMORY_BLOCK_NONE;
const struct memory_block *b = NULL;
ret.size_idx = 0;
for (int i = 0; i < n; ++i) {
if (blocks[i] == NULL)
continue;
b = b ? b : blocks[i];
ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;
}
....
}PVS-Studio-Warnung: V547 [CWE-571] Der Ausdruck 'Blocks [i]' ist immer wahr. heap.c 1054
Wenn die Blöcke [i] == NULL sind , wird die continue- Anweisung ausgelöst und die Schleife startet die nächste Iteration. Daher ist eine erneute Überprüfung der Elementblöcke [i] nicht sinnvoll und der ternäre Operator ist überflüssig. Der Code kann vereinfacht werden:
....
for (int i = 0; i < n; ++i) {
if (blocks[i] == NULL)
continue;
b = b ? b : blocks[i];
ret.size_idx += blocks[i]->size_idx;
}
....Verdächtige Verwendung eines Nullzeigers
void win_mmap_fini(void)
{
....
if (mt->BaseAddress != NULL)
UnmapViewOfFile(mt->BaseAddress);
size_t release_size =
(char *)mt->EndAddress - (char *)mt->BaseAddress;
void *release_addr = (char *)mt->BaseAddress + mt->FileLen;
mmap_unreserve(release_addr, release_size - mt->FileLen);
....
}PVS-Studio-Warnung: V1004 [CWE-119] Der Zeiger '(char *) mt-> BaseAddress' wurde unsicher verwendet, nachdem er gegen nullptr überprüft wurde. Prüfzeilen: 226, 235. win_mmap.c 235
Der Zeiger mt-> BaseAddress kann null sein, wie aus der Prüfung hervorgeht:
if (mt->BaseAddress != NULL)Darunter wird dieser Zeiger jedoch bereits in arithmetischen Operationen ohne Prüfung verwendet. Zum Beispiel hier:
size_t release_size =
(char *)mt->EndAddress - (char *)mt->BaseAddress;Es wird ein großer ganzzahliger Wert empfangen, der tatsächlich der Wert des Zeigers mt-> EndAddress ist . Dies ist vielleicht kein Fehler, aber alles sieht sehr verdächtig aus, und es scheint mir, dass der Code erneut überprüft werden sollte. Der Geruch liegt in der Tatsache, dass der Code unverständlich ist und eindeutig keine erklärenden Kommentare enthält.
Kurznamen globaler Variablen
Ich glaube, dass der Code riecht, wenn er globale Variablen mit Kurznamen enthält. Es ist einfach, in einigen Funktionen nicht eine lokale, sondern eine globale Variable abzudichten und versehentlich zu verwenden. Beispiel:
static struct critnib *c;PVS-Studio-Warnungen für solche Variablen:
- V707 Das Vergeben von Kurznamen für globale Variablen wird als schlechte Praxis angesehen. Es wird empfohlen, die Variable 'ri' umzubenennen. map.c 131
- V707 Das Vergeben von Kurznamen für globale Variablen wird als schlechte Praxis angesehen. Es wird empfohlen, die Variable 'c' umzubenennen. obj_critnib_mt.c 56
- V707 Das Vergeben von Kurznamen für globale Variablen wird als schlechte Praxis angesehen. Es wird empfohlen, die Variable 'Id' umzubenennen. obj_list.h 68
- V707 Das Vergeben von Kurznamen für globale Variablen wird als schlechte Praxis angesehen. Es wird empfohlen, die Variable 'Id' umzubenennen. obj_list.c 34
Seltsam
Der seltsamste Code, auf den ich gestoßen bin, befindet sich in der Funktion do_memmove . Der Analysator ergab zwei positive Ergebnisse, die entweder auf sehr schwerwiegende Fehler hinweisen oder die ich einfach nicht verstehe, was ich meine. Da der Code sehr seltsam ist, habe ich beschlossen, die Warnungen in einem separaten Abschnitt des Artikels zu berücksichtigen. Hier wird also die erste Warnung ausgegeben.
void
do_memmove(char *dst, char *src, const char *file_name,
size_t dest_off, size_t src_off, size_t bytes,
memmove_fn fn, unsigned flags, persist_fn persist)
{
....
/* do the same using regular memmove and verify that buffers match */
memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);
verify_contents(file_name, 0, dstshadow, dst, bytes);
verify_contents(file_name, 1, srcshadow, src, bytes);
....
}PVS-Studio-Warnung: V549 [CWE-688] Das erste Argument der Funktion 'memmove' entspricht dem zweiten Argument. memmove_common.c 71
Beachten Sie, dass das erste und das zweite Argument der Funktion identisch sind. Somit macht die Funktion tatsächlich nichts. Welche Optionen kommen mir in den Sinn:
- Ich wollte den Speicherblock "berühren". Aber wird das in der Realität passieren? Entfernt der optimierende Compiler den Code, der den Speicherblock in sich selbst kopiert?
- Dies ist eine Art Unit-Test für die Memmove- Funktion .
- Der Code enthält einen Tippfehler.
Und hier ist ein ebenso seltsamer Ausschnitt in derselben Funktion:
void
do_memmove(char *dst, char *src, const char *file_name,
size_t dest_off, size_t src_off, size_t bytes,
memmove_fn fn, unsigned flags, persist_fn persist)
{
....
/* do the same using regular memmove and verify that buffers match */
memmove(dstshadow + dest_off, srcshadow + src_off, 0);
verify_contents(file_name, 2, dstshadow, dst, bytes);
verify_contents(file_name, 3, srcshadow, src, bytes);
....
}PVS-Studio-Warnung: V575 [CWE-628] Die Funktion 'memmove' verarbeitet '0'-Elemente. Untersuchen Sie das dritte Argument. memmove_common.c 82
Die Funktion verschiebt 0 Bytes. Was ist das? Gerätetest? Tippfehler?
Für mich ist dieser Code unverständlich und seltsam.
Warum Code-Analysatoren verwenden?
Es scheint, dass die Einführung des Analysators in den Codeentwicklungsprozess unangemessen ist, da nur wenige Fehler gefunden wurden. Bei der Verwendung statischer Analysewerkzeuge geht es jedoch nicht um einmalige Überprüfungen, sondern um die regelmäßige Erkennung von Fehlern beim Schreiben des Codes. Andernfalls werden diese Fehler teurer und langsamer erkannt (Debuggen, Testen, Benutzerüberprüfungen usw.). Diese Idee wird ausführlicher in dem Artikel " Fehler, den die statische Code-Analyse nicht finden kann, weil sie nicht verwendet wird " beschrieben, den ich zum Kennenlernen empfehle. Besuchen Sie dann unsere Website, um PVS-Studio herunterzuladen und zu testen und Ihre Projekte zu überprüfen.
Vielen Dank für Ihre Aufmerksamkeit!
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Andrey Karpov. Statische Code-Analyse der PMDK-Bibliothekssammlung durch Intel und Fehler, die keine tatsächlichen Fehler sind .