Docotic.Pdf: Welche Probleme entdeckt PVS-Studio in einem ausgereiften Projekt?

    Docotic.Pdf und PVS-Studio

    Qualität ist uns wichtig. Und wir haben von PVS-Studio gehört. All dies führte zu dem Wunsch, Docotic.Pdf zu überprüfen und herauszufinden, was noch verbessert werden kann.

    Docotic.Pdf ist eine Universalbibliothek zum Arbeiten mit PDF-Dateien. In C # geschrieben, kein unsicherer Code, keine externen Abhängigkeiten außer der .NET-Laufzeit. Es funktioniert sowohl unter .NET 4+ als auch unter .NET Standard 2+.

    Die Bibliothek ist seit etwas mehr als 10 Jahren in der Entwicklung und umfasst 110.000 Codezeilen, ohne Tests, Beispiele und andere Dinge zu berücksichtigen. Für die statische Analyse verwenden wir ständig Code Analysis und StyleCop. Mehrere tausend automatische Tests schützen uns vor Regressionen. Unsere Kunden aus verschiedenen Ländern und verschiedenen Branchen vertrauen auf die Qualität der Bibliothek.

    Welche Probleme werden von PVS-Studio erkannt?

    Installation und erster Eindruck


    Ich habe die Testversion von der PVS-Studio-Website heruntergeladen. Angenehm überrascht von der geringen Größe des Installers. Mit den Standardeinstellungen installiert: Analyse-Engines, separates PVS-Studio, Integration in Visual Studio 2017.

    Nach der Installation wurde nichts gestartet und zwei Verknüpfungen mit denselben Symbolen wurden dem Startmenü hinzugefügt: Standalone und PVS-Studio. Für einen Moment dachte ich darüber nach, was ich anfangen sollte. Ich habe Standalone gestartet und war von der Benutzeroberfläche unangenehm überrascht. Die für mein Windows eingestellte Skalierung von 200% wird schief unterstützt. Ein Teil des Textes ist zu klein, ein Teil des Textes passt nicht in den dafür reservierten Bereich. Der Name, das Einhorn und die Aktionsliste werden bei jeder Fenstergröße abgeschnitten. Auch mit Vollbild.



    Okay, ich habe mich entschieden, meine Projektdatei zu öffnen. Plötzlich fand das Menü Datei eine solche Option nicht. Dort wurde mir nur angeboten, separate Dateien zu öffnen. Danke, dachte ich, ich probiere eine andere Option. Ich habe PVS-Studio gestartet - sie zeigten mir ein Fenster mit unscharfem Text. Maßstab 200% machte sich wieder bemerkbar. Textbericht: Suchen Sie in "Three Crowns" nach mir, suchen Sie in Visual Studio nach dem Menü PVS-Studio. Nun, eröffnete das Studio.

    Die Lösung wurde geöffnet. In der Tat gibt es ein PVS-Studio-Menü, in dem sich das aktuelle Projekt befindet. Ich habe das Projekt gemacht, das ich brauchte, und habe den Check gestartet. Unten im Studio wurde ein Fenster mit den Ergebnissen der Analyse angezeigt. Im Hintergrund erschien auch ein Fenster mit dem Fortschritt der Kasse, das ich jedoch nicht sofort fand. Zuerst gab es das Gefühl, dass der Test nicht begonnen hat oder sofort beendet wurde.

    Ergebnis der ersten Überprüfung


    Der Analysator hat alle 1253 Projektdateien in ca. 9 Minuten und 30 Sekunden geprüft. Am Ende der Prüfung hat sich der Dateizähler nicht so schnell wie zu Beginn geändert. Möglicherweise gibt es eine nichtlineare Abhängigkeit der Scandauer von der Anzahl der zu scannenden Dateien.

    Im Ergebnisfenster wurden Informationen zu 81 Hoch-, 109 Mittel- und 175 Tief-Warnungen angezeigt. Wenn Sie die Häufigkeit zählen, erhalten Sie 0,06 High-Warnungen / Datei, 0,09 Medium-Warnungen / Datei und 0,14 Low-Warnungen / Datei. Oder
    0,74 Hochwarnungen pro tausend Codezeilen, 0,99 mittlere Warnungen pro tausend Codezeilen und 1,59 Low-Warnungen pro tausend Codezeilen.

    Hier in diesem Artikel wird angegeben, dass der Analysator in CruiseControl.NET mit seinen 256.000 Codezeilen 15 Hoch-, 151 Mittel- und 32 Tief-Warnungen gefunden hat.

    Es stellt sich heraus, dass pro Prozentsatz für Docotic.Pdf in jeder der Gruppen mehr Warnungen ausgegeben wurden.

    Was ist gefunden


    Warnungen Niedrig Ich habe mich entschieden, zu diesem Zeitpunkt nichts zu beachten.

    Ich habe Warnungen in der Spalte Code sortiert und es stellte sich heraus, dass V3022 "Ausdruck ist immer wahr / falsch" und V3063 "Wenn es ausgewertet wird", der absolute Frequenzrekordhalter wurde . Meiner Meinung nach handelt es sich um eine Sache. Insgesamt ergeben diese beiden Warnungen 92 von 190 Fällen. Relative Häufigkeit = 48%.

    Die Aufteilungslogik in Hoch und Mittel ist nicht völlig klar. Ich habe erwartet, dass V3072 "Die Klasse A, die IDisposable-Member enthält, selbst nicht IDisposable implementiert" und V3073 "Nicht alle IDisposable-Member sind ordnungsgemäß entsorgt. Rufen Sie 'Dispose' auf, wenn Sie beispielsweise 'A' class 'in der High-Gruppe entsorgen. Aber es schmeckt natürlich.

    Ich war überrascht, dass V3095 „Das Objekt wurde verwendet, bevor es auf null überprüft wurde. Überprüfen Sie die Zeilen: N1, N2 ”ist zweimal als Hoch und einmal als Mittel markiert. Bug



    Vertrauen aber überprüfen


    Es ist an der Zeit zu prüfen, ob die erhaltenen Warnungen berechtigt sind. Wurden echte Fehler gefunden? Gibt es falsche Warnungen?

    Ich habe die gefundenen Warnungen in die folgenden Gruppen unterteilt.

    Wichtige Warnungen


    Ihre Korrektur erhöhte die Stabilität der Arbeit, löste Probleme mit Speicherverlusten usw. Echte Fehler / Unvollkommenheiten.

    Es gab 16 davon, was etwa 8% aller Warnungen entspricht.

    Ich werde einige Beispiele geben.

    V3019 "Möglicherweise ist die Variable nach der Typumwandlung mit dem Schlüsselwort 'as' möglicherweise falsch im Vergleich zu null. Variablen prüfen 'color', 'indexed' »

    publicoverrideboolIsCompatible(ColorImpl color)
    {
        IndexedColorImpl indexed = color as IndexedColorImpl;
        if (color == null)
            returnfalse;
        return indexed.ColorSpace.Equals(this);
    }
    

    Wie Sie sehen können, wird anstelle der Indexierung die Farbe der Variablen mit Null verglichen. Dies ist falsch und kann zu NRE führen.

    V3080 “Mögliche Null-Dereferenzierung. Betrachten Sie 'cstr_index.tile_index'. ”

    Ein kleines Fragment zur Veranschaulichung:

    if (cstr_index.tile_index == null)
    {
        if (cstr_index.tile_index[0].tp_index == null)
        {
            // ..
        }
    }
    

    Offensichtlich impliziert die erste Bedingung! = Null. In der aktuellen Ansicht ruft der Code bei jedem Aufruf NRE auf.

    V3083 “Unsicherer Aufruf des Ereignisses 'OnProgress', NullReferenceException ist möglich. Erwägen Sie, einer lokalen Variablen ein Ereignis zuzuweisen, bevor Sie sie aufrufen. “

    publicvoidUpdated()
    {
        if (OnProgress != null)
            OnProgress(this, new EventArgs());
    }
    

    Die Warnung half, eine mögliche Ausnahme zu korrigieren. Warum kann es vorkommen? Es gibt eine gute Erklärung für Stackoverflow .

    V3106 “Möglicherweise liegt der Index außerhalb der Grenzen. Der '0'-Index zeigt über' v 'hinaus »

    var result = new List<FontStringPair>();
    for (int i = 0; i < text.Length; ++i)
    {
        var v = new List<FontStringPair>();
        createPairs(text[i].ToString(CultureInfo.InvariantCulture));
        result.Add(v[0]);
    }
    

    Der Fehler ist, dass das Ergebnis von createPairs ignoriert wird und stattdessen die leere Liste angesprochen wird. Anscheinend hat createPairs die Liste anfangs als Parameter akzeptiert, bei den Methodenänderungen wurde jedoch ein Fehler gemacht.

    V3117 „Konstruktorparameter 'validateType' wird nicht verwendet Es

    wurde eine Warnung für einen ähnlichen Code ausgegeben

    publicSomeClass(IDocument document, bool validateType = true)
        : base(document, true)
    {
        m_provider = document;
    }
    

    Die Warnung selbst scheint nicht wichtig zu sein. Das Problem ist jedoch ernster, als es auf den ersten Blick scheint. Beim Hinzufügen des optionalen Parameters validateType wurde vergessen, dass der Aufruf des Konstruktors der Basisklasse behoben wurde.

    V3127 „Zwei ähnliche Codefragmente wurden gefunden. Vielleicht ist dies ein Tippfehler, und anstelle von 'domain' sollte eine 'range'-Variable verwendet werden. "

    privatevoidfillTransferFunction(PdfStreamImpl function)
    {
        // ..
        PdfArrayImpl domain = new PdfArrayImpl();
        domain.AddReal(0);
        domain.AddReal(1);
        function.Add(Field.Domain, domain);
        PdfArrayImpl range = new PdfArrayImpl();
        range.AddReal(0);
        range.AddReal(1);
        function.Add(Field.Range, domain);
        // ....
    }
    

    Möglicherweise wird keine Warnung ausgegeben, wenn sich Teile des Codes etwas unterscheiden. In diesem Fall wurde beim Kopieren / Einfügen jedoch ein Fehler gefunden.

    Theoretische / formelle Warnungen


    Sie sind entweder richtig, aber das Beheben von Fehlern behebt keine spezifischen Fehler und beeinträchtigt nicht die Lesbarkeit des Codes. Oder sie zeigen auf Stellen, an denen ein Fehler auftreten könnte, aber es gibt keine. Beispielsweise wird die Reihenfolge der Parameter absichtlich geändert. Für solche Warnungen brauchen Sie nichts an dem Programm zu ändern.

    Es waren 57 davon, was etwa 30% aller Warnungen entspricht. Ich werde Beispiele für Fälle geben, die Aufmerksamkeit verdienen.

    V3013 „Es ist ungewöhnlich, dass der Body der BeginText-Funktion vollständig mit dem Body der EndText-Funktion übereinstimmt (166, Zeile 171).“

    publicoverridevoidBeginText()
    {
        m_state.ResetTextParameters();
    }
    publicoverridevoidEndText()
    {
        m_state.ResetTextParameters();
    }
    

    Beide Körperfunktionen sind eigentlich gleich. Aber es ist richtig. Und ist es wirklich so merkwürdig, wenn die Funktionskörper einer Zeile zusammenfallen?

    V3106 „Möglicher negativer Indexwert. Der Wert des 'c1'-Index könnte -1 erreichen.

    freq[256] = 1;
    // ....
    c1 = -1;
    v = 1000000000L;
    for (i = 0; i <= 256; i++)
    {
        if (freq[i] != 0 && freq[i] <= v)
        {
            v = freq[i];
            c1 = i;
        }
    }
    // ....
    freq[c1] += freq[c2];
    

    Es stimmt, ich habe ein Stück des nicht eindeutigsten Algorithmus erhalten. Meiner Meinung nach leidet der Analysator in diesem Fall vergeblich.

    V3107 "Identischer Ausdruck" neighsum "links und rechts der zusammengesetzten Zuordnung" Die

    Warnung wurde durch einen vollständig banalen Code verursacht:

    neighsum += neighsum;
    

    Ja, es kann durch Multiplikation überschrieben werden. Aber es ist kein Fehler.

    V3109 „Der Unterausdruck 'l_cblk.data_current_size' Der Ausdruck ist falsch oder kann vereinfacht werden. “

    /* Check possible overflow on size */if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size)
    {
        // ...
    }
    

    Ein Kommentar im Code erklärt die Absicht. Wieder Fehlalarm.

    Angemessene Warnungen


    Ihre Korrektur hatte einen positiven Einfluss auf die Lesbarkeit des Codes. Das heißt, reduzierte unnötige Bedingungen, Prüfungen usw. Die Auswirkungen auf die Funktionsweise des Codes sind nicht offensichtlich.

    Es waren 103 von ihnen, was etwa 54% aller Warnungen entspricht.

    V3008 „Die Variable 'l_mct_deco_data' wird zweimal hintereinander mit Werten belegt. Vielleicht ist das ein Fehler “

    if (m_nb_mct_records == m_nb_max_mct_records)
    {
        ResizeMctRecords();
        l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
    }
    l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
    

    Rights Analyzer: Zuordnung innen, falls nicht notwendig.

    V3009 „Es ist seltsam, dass diese Methode immer einen und denselben Wert zurückgibt.“

    privatestaticboolopj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres)
    {
        if (numres == 1U)
            returntrue;
        // ...returntrue;
    }
    

    Auf Empfehlung des Analysators wurde die Methode geändert und gibt nichts mehr zurück.

    V3022 „Ausdruck '! Hinzufügen' ist immer wahr“

    privatevoidaddToFields(PdfDictionaryImpl controlDict, booladd)
    {
        // ...if (add)
        {
            // ..return;
        }
        if (!add)
        {
            // ...
        }
        // ...
    }
    

    In der Tat hat es keinen Sinn, wenn. Die Bedingung wird immer wahr sein.

    V3029 "Der bedingte Ausdruck der nebeneinander folgenden " if " -Anweisungen ist identisch"

    if (stroke)
        extGState.OpacityStroke = opacity;
    if (stroke)
        state.AddReal(Field.CA, opacity);
    

    Es ist nicht klar, wie ein solcher Code zustande kam. Aber jetzt haben wir es behoben.

    V3031 Anonyme Prüfung kann vereinfacht werden. Das '||' Operator ist von entgegengesetzten Ausdrücken umgeben. “

    Dies ist eine Albtraumbedingung:

    if (!(cp.m_enc.m_tp_on != 0 &&
        ((!opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) && (t2_mode == J2K_T2_MODE.FINAL_PASS)) || opj_codec_t.OPJ_IS_CINEMA(cp.rsiz))))
    {
        // ...
    }
    

    Nach dem Wechsel geht es viel besser

    if (!(cp.m_enc.m_tp_on != 0 &&
        (opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS)))
    {
        // ...
    }
    

    V3063 „Ein Teil des bedingten Ausdrucks ist immer der , wenn er auf true ausgewertet wird: x = null“
    V3022 „‚! X = null‘Der Ausdruck ist immer wahr“

    Hier habe ich die Warnung , dass die Prüfung auf null macht keinen Sinn. Ist es richtig, die Frage ist mehrdeutig. Im Folgenden habe ich das Wesentliche der Ausgabe beschrieben.

    Ungerechtfertigte Warnungen


    Falsche Positive Aufgrund von Fehlern bei der Implementierung eines bestimmten Tests oder aufgrund fehlender Analysegeräte.

    14 von ihnen wurden ausgegeben, was etwa 7% aller Warnungen entspricht.

    V3081 „Der ' i' -Zähler wird nicht innerhalb einer verschachtelten Schleife verwendet. Betrachten Sie die Verwendung von 'j' Counter. '

    Eine etwas vereinfachte Version des Codes, für den diese Warnung ausgegeben wurde:

    for (uint i = 0; i < initialGlyphsCount - 1; ++i)
    {
        for (int j = 0; j < initialGlyphsCount - i - 1; ++j)
        {
            // ...
        }
    }
    

    Natürlich wird i in einer verschachtelten Schleife verwendet.

    V3125 „Das Objekt wurde verwendet, nachdem es gegen null überprüft wurde“

    Der Code, für den eine solche Warnung ausgegeben wird:

    privatestaticintCompare_SecondGreater(cmapEncodingRecord er1, cmapEncodingRecord er2)
    {
        if (er1 == er2)
            return0;
        if (er1 != null && er2 == null)
            return1;
        if (er1 == null && er2 != null)
            return-1;
        return er1.CompareTo(er2);
    }
    

    er1 kann zum Zeitpunkt des Aufrufs von CompareTo () nicht null sein.

    Ein weiterer Code, für den diese Warnung ausgegeben wird:

    privatestaticvoidrealloc(refint[][] table, int newSize)
    {
        int[][] newTable = newint[newSize][];
        int existingSize = 0;
        if (table != null)
            existingSize = table.Length;
        for (int i = 0; i < existingSize; i++)
            newTable[i] = table[i];
        for (int i = existingSize; i < newSize; i++)
            newTable[i] = newint[4];
        table = newTable;
    }
    

    Tabelle kann in einer Schleife nicht null sein.

    V3134 „Verschiebung um [32.255] Bits ist größer als die Größe des Ausdruckstyps 'UInt32' '(uint) 1'“

    Ein Code, für den diese Warnung ausgegeben wird:

    byte bitPos = (byte)(numBits & 0x1F);
    uint mask = (uint)1 << bitPos;
    

    Es ist ersichtlich, dass bitPos einen Wert aus dem Bereich [0..31] haben kann, der Analysator glaubt jedoch, dass er einen Wert aus dem Bereich [0..255] haben kann, der falsch ist.

    Ich werde keine anderen ähnlichen Fälle geben, da sie gleichwertig sind.

    Zusätzliche Gedanken zu einigen Prüfungen


    Es schien nicht wünschenswert zu warnen, dass 'x! = Null' immer wahr ist, wenn x das Ergebnis einer Methode ist. Beispiel:

    private X GetX(int y)
    {
        if (y > 0)
           returnnew X(...);
        if (y == 0)
           returnnew X(...);
        thrownew ArgumentOutOfRangeException(nameof(x));
    }
    privateMethod()
    {
        // ...
        X x = GetX(..);
        if (x != null)
        {
           // ...
        }
    }
    

    Ja, formal hat der Analysator Recht: x ist immer nicht null, da GetX entweder eine vollständige Instanz zurückgibt oder eine Ausnahme auslöst. Aber verbessert der Code das Entfernen von Nullprüfungen? Was ist, wenn sich GetX später ändert? Ist Method erforderlich, um die GetX-Implementierung zu kennen?

    Innerhalb des Teams sind die Meinungen geteilt. Es wurde vorgeschlagen, dass die aktuelle Methode einen Vertrag hat, in dem sie nicht null zurückgeben sollte. Und es macht keinen Sinn, bei jedem Aufruf redundanten Code "für die Zukunft" zu schreiben. Und wenn sich der Vertrag ändert, ist es notwendig, den aufrufenden Code zu aktualisieren.

    Zur Stützung dieser Stellungnahme wurde das folgende Argument angeführt: Bei der Überprüfung auf null wird festgelegt, wie die einzelnen Aufrufe in try / catch eingeschlossen werden, falls die Methode in der Zukunft Ausnahmen auslöst.

    Als Ergebnis nach dem Prinzip YAGNIbeschloss, die Schecks nicht zu behalten und löschte sie. Alle Warnungen wurden von theoretisch / formal zu vernünftig verschoben.

    Ich würde mich freuen, Ihre Meinung in den Kommentaren zu lesen.

    Schlussfolgerungen


    Statische Analyse ist eine nützliche Sache. Mit PVS-Studio können Sie echte Fehler finden.

    Ja, es gibt unbegründete / falsche Warnungen. Dennoch hat PVS-Studio echte Fehler im Projekt gefunden, in denen bereits Code Analysis verwendet wird. Unser Produkt ist ziemlich gut mit Tests abgedeckt, unsere Kunden testen es auf die eine oder andere Weise, aber die statische Analyse kommt den Robotern immer noch zugute.

    Zum Schluss noch ein paar Statistiken.

    Top 3 ungerechtfertigte Warnungen


    V3081 „Der ' X' -Zähler wird nicht innerhalb einer Loop-Schleife verwendet. Betrachten Sie die Verwendung des ' Y' -Zählers "
    1 von 1 als unbegründet.

    V3125 " Das Objekt wurde dagegen verwendet. Kontrollieren Sie die Zeilen: N1, N2 “
    9 von 10 als unbegründet

    befunden V3134 „ Verschiebung um N Bits ist größer als die Größe des Typs “
    4 von 5 werden als unzumutbar befunden

    Top 3 Wichtige Warnungen


    V3083 Unsicherer Aufruf des Ereignisses, NullReferenceException ist möglich. Betrachten Ereignis zu einer lokalen Variablen zugewiesen wird, bevor es „Aufruf
    5 von 5 anerkannt als wichtigem

    V3020 “ einer unbedingten ‚break - Anweisung /‘ continue ‚/ return - Anweisung / GOTO‘ Innerhalb einer Schleife „
    V3080 “ das Mögliche null dereferenzieren „
    2 von 2 anerkannt als wichtigem

    V3019 “ Es ist möglich , dass Eine falsche Variable wird mit der Konvertierung nach dem Typ mit dem Schlüsselwort 'as' verglichen.
    V3127 „Es wurden zwei ähnliche Codefragmente gefunden. Vielleicht ist dies ein Tippfehler, und anstelle von 'Y' sollte eine 'X'-Variable verwendet werden.'
    1 von 1 wird als wichtig erachtet

    Jetzt auch beliebt: