Microsoft Open XMLSDKコード品質調査

Open XML SDKの知識は、レポート付きのWordドキュメントを作成するためのライブラリが必要になったときに始まりました。Word APIを7年以上使用した後、私は何か新しくて便利なものを試したかったのです。このようにして、Microsoftには別の解決策があることがわかりました。従来、PVS-Studioアナライザーを使用して、社内で使用されているプログラムとライブラリを事前にチェックしています。

前書き

OpenXMLまたはOOXMLとも呼ばれるOfficeOpen XMLは、ワード処理ドキュメント、スプレッドシート、プレゼンテーション、図、形状、その他のグラフィックを含むOfficeドキュメントのXMLベースの形式です。この仕様はMicrosoftによって開発され、2006年にECMAInternationalによって採用されました。 2014年6月、MicrosoftはOpen XMLSDKをオープンソースでリリースしました。ソースは、MITライセンスの下でGitHubで利用できるようになりました。



ライブラリのソースコードのエラーを見つけるために、PVS-Studioを使用しました。これは、C、C ++、C＃、およびJavaで記述されたプログラムのソースコードのエラーと潜在的な脆弱性を特定するためのツールです。 Windows、Linux、macOSの64ビットシステムで動作します。



プロジェクトは十分に小さく、警告はほとんどありませんでした。しかし、タイトル画像の選択は正確に結果に基づいていました。コードには役に立たない条件演算子がたくさんあります。コード内のそのような場所をすべてリファクタリングすると、ボリュームが著しく減少するように思われます。その結果、コードの読みやすさも向上します。

XMLSDKを開かずにWordAPIを使用する理由

タイトルからお察しのとおり、引き続きWordAPIを使用しました。この方法には多くの欠点があります。

• 古い厄介なAPI;
• MicrosoftOfficeをインストールする必要があります。
• Officeライブラリとともに配布キットを配布する必要性。
• WordAPIのシステムロケール設定への依存。
• 作業速度が遅い。

一般的なロケールでは、面白い事件が発生しました。Windowsには12の地域設定があります。サーバーコンピューターの1つで、米国と英国のロケールからの混乱がありました。このため、ドル記号の代わりにルーブルがあり、ポンドがまったく表示されないWordドキュメントが作成されました。オペレーティングシステムの設定を改善すると、問題が修正されました。



これらすべてをリストして、なぜ私はまだそれを使用しているのかもう一度疑問に思いました...



しかし、いいえ、私はまだWord APIの方が好きです、そしてここに理由があります。



OOXMLは次のようになります。

<?xml version="1.0" encoding="utf-8" standalone="yes"?>
<w:document ....>
  <w:body>
    <w:p w:rsidR="00E22EB6"
         w:rsidRDefault="00E22EB6">
      <w:r>
        <w:t>This is a paragraph.</w:t>
      </w:r>
    </w:p>
    <w:p w:rsidR="00E22EB6"
         w:rsidRDefault="00E22EB6">
      <w:r>
        <w:t>This is another paragraph.</w:t>
      </w:r>
    </w:p>
  </w:body>
</w:document>

ここで、<w：r>（Word Run）は文ではなく、単語でもありませんが、隣接するテキストフラグメントとは異なる属性を持つテキストです。



これは次のようなものでプログラムされています：

Paragraph para = body.AppendChild(new Paragraph());
Run run = para.AppendChild(new Run());
run.AppendChild(new Text(txt));

ドキュメントには特定の内部構造があり、コードで同じ要素を作成する必要があります。私の意見では、Open XMLSDKには十分な抽象データアクセス層がありません。Word APIを使用してドキュメントを作成すると、より明確で短くなります。特にテーブルやその他の複雑なデータ構造に関しては。



次に、Open XMLSDKはさまざまなタスクを解決します。これを使用すると、Wordだけでなく、ExcelやPowerPoint用のドキュメントを作成できます。このライブラリはおそらくいくつかのタスクに適していますが、今のところWordAPIを使用することにしました。いずれにせよ、それを完全に放棄することは不可能です。内部のニーズのために、Word用のプラグインを開発しており、そこではWordAPIのみを使用できます。

文字列の2つの値

V3008'_rawOuterXml '変数には、連続して2回値が割り当てられます。おそらくこれは間違いです。チェック行：164、161。OpenXmlElement.cs 164

internal string RawOuterXml
{
    get => _rawOuterXml;

    set
    {
        if (string.IsNullOrEmpty(value))
        {
            _rawOuterXml = string.Empty;
        }

        _rawOuterXml = value;
    }
}

文字列 タイプには、nullとテキスト値の2種類の値を指定できます。テキストの意味を使用する方が間違いなく安全ですが、どちらのアプローチも有効です。このプロジェクトでは、null値を使用することは許可されておらず、string.Emptyに書き換えられます...少なくともそれが意図された方法です。ただし、RawOuterXmlのエラーのため、nullを書き込んでからこのフィールドを参照し、NullReferenceExceptionを受け取ることができます。



V3022式 'namespaceUri！= Null'は常にtrueです。 OpenXmlElement.cs 497

public OpenXmlAttribute GetAttribute(string localName, string namespaceUri)
{
    ....
    if (namespaceUri == null)
    {
        // treat null string as empty.
        namespaceUri = string.Empty;
    }
    ....
    if (HasAttributes)
    {
        if (namespaceUri != null)  // <=
        {
            ....
        }
        ....
    }
    ....
}

このスニペットは同じアプローチを使用しており、コードの作成者は大きな間違いを犯していませんが、それでも悪いリファクタリングの匂いがします。ほとんどの場合、ここで1つのチェックを削除できます。これにより、コードの幅が狭くなり、読みやすさが向上します。

コードのコンパクトさについて

V3009このメソッドが常に「.xml」の同じ値を返すのは奇妙です。CustomXmlPartTypeInfo.cs 31

internal static string GetTargetExtension(CustomXmlPartType partType)
{
    switch (partType)
    {
        case CustomXmlPartType.AdditionalCharacteristics:
            return ".xml";

        case CustomXmlPartType.Bibliography:
            return ".xml";

        case CustomXmlPartType.CustomXml:
            return ".xml";

        case CustomXmlPartType.InkContent:
            return ".xml";

        default:
            return ".xml";
    }
}

ここに何らかのタイプミスがあるのか​​、それともコードの作成者が彼の意見で「素敵な」コードを書いたのかはわかりません。関数から同じタイプの値をこれほど多く返すことには意味がないと確信しており、コードを大幅に削減できます。



これだけがそのような場所ではありません。これらの警告のいくつかを次に示します。

• V3009このメソッドが常に「.xml」の同じ値を返すのは奇妙です。CustomPropertyPartTypeInfo.cs 25
• V3009このメソッドが常に同じ値の '".bin"'を返すのは奇妙なことです。EmbeddedControlPersistenceBinaryDataPartTypeInfo.cs 22

なぜそのように書くのかを聞くのは興味深いでしょう。



V31392つ以上のケースブランチが同じアクションを実行します。OpenXmlPartReader.cs 560

private void InnerSkip()
{
    Debug.Assert(_xmlReader != null);

    switch (_elementState)
    {
        case ElementState.Null:
            ThrowIfNull();
            break;

        case ElementState.EOF:
            return;

        case ElementState.Start:
            _xmlReader.Skip();
            _elementStack.Pop();
            GetElementInformation();
            return;

        case ElementState.End:
        case ElementState.MiscNode:
            // cursor is end element, pop stack
            _xmlReader.Skip();
            _elementStack.Pop();
            GetElementInformation();
            return;
        ....
    }
    ....
}

このコードに関する質問は少なくなります。ほとんどの場合、同一のケースを組み合わせることができ、コードはより短く、より明白になります。



さらにいくつかのそのような場所：

• V31392つ以上のケースブランチが同じアクションを実行します。OpenXmlMiscNode.cs 312
• V31392つ以上のケースブランチが同じアクションを実行します。CustomPropertyPartTypeInfo.cs 30
• V31392つ以上のケースブランチが同じアクションを実行します。CustomXmlPartTypeInfo.cs 15
• V31392つ以上のケースブランチが同じアクションを実行します。OpenXmlElement.cs 1803

それらは常に真/偽

今こそ、タイトル画像の選択を決定するいくつかのコード例を提供するときです。



警告



1V3022式 'Complete（）'は常にfalseです。ParticleCollection.cs 243

private bool IsComplete => Current is null ||
                           Current == _collection._element.FirstChild;

public bool MoveNext()
{
    ....
    if (IsComplete)
    {
        return Complete();
    }

    if (....)
    {
        return Complete();
    }

    return IsComplete ? Complete() : true;
}

IsComplete プロパティは2回使用され、その値が変更されないことはコードから簡単に理解できます。したがって、関数の最後で、3値演算子の2番目の値であるtrueを返すことができます。



警告



2V3022式 '_elementStack.Count> 0'は常に真です。OpenXmlDomReader.cs 501

private readonly Stack<OpenXmlElement> _elementStack;

private bool MoveToNextSibling()
{
    ....
    if (_elementStack.Count == 0)
    {
        _elementState = ElementState.EOF;
        return false;
    }
    ....
    if (_elementStack.Count > 0) // <=
    {
        _elementState = ElementState.End;
    }
    else
    {
        // no more element, EOF
        _elementState = ElementState.EOF;
    }
    ....
}

明らかに、_elementStackに0個の要素がない場合は、さらに多くの要素があります。コードは少なくとも8行短縮できます。



警告



3V3022式 'rootElement == null'は常にfalseです。OpenXmlPartReader.cs 746

private static OpenXmlElement CreateElement(string namespaceUri, string name)
{
    if (string.IsNullOrEmpty(name))
    {
        throw new ArgumentException(....);
    }

    if (NamespaceIdMap.TryGetNamespaceId(namespaceUri, out byte nsId)
        && ElementLookup.Parts.Create(nsId, name) is OpenXmlElement element)
    {
        return element;
    }

    return new OpenXmlUnknownElement();
}

private bool ReadRoot()
{
  ....
  var rootElement = CreateElement(....);

  if (rootElement == null) // <=
  {
      throw new InvalidDataException(....);
  }
  ....
}

CreateElement 関数はnullを返すことはできません。会社が、有効なオブジェクトを返すか例外をスローするxmlノードを作成するためのメソッドを作成するルールを作成した場合、そのような関数のユーザーは追加のチェックを悪用することはできません。



警告



4V3022式 'nameProvider'は常にnullではありません。オペレーター '？。' 過剰です。OpenXmlSimpleTypeExtensions.cs 50

public static XmlQualifiedName GetSimpleTypeQualifiedName(....)
{
    foreach (var validator in validators)
    {
        if (validator is INameProvider nameProvider &&
            nameProvider?.QName is XmlQualifiedName qname) // <=
        {
            return qname;
        }
    }

    return type.GetSimpleTypeQualifiedName();
}

is演算子のパターンは次のとおりです。

expr is type varname

場合は式exprのに評価され、真の、そしてVARNAMEのオブジェクトが有効になりますと比較する必要はありませんnullで再び、このコードスニペットで行われるように、。



警告



5V3022式 '拡張子== "。xlsx" || extension == ".xlsm" 'は常にfalseです。 PresentationDocument.cs 246

public static PresentationDocument CreateFromTemplate(string path)
{
    ....
    string extension = Path.GetExtension(path);
    if (extension != ".pptx" && extension != ".pptm" &&
        extension != ".potx" && extension != ".potm")
    {
        throw new ArgumentException("...." + path, nameof(path));
    }

    using (PresentationDocument template = PresentationDocument.Open(....)
    {
        PresentationDocument document = (PresentationDocument)template.Clone();

        if (extension == ".xlsx" || extension == ".xlsm")
        {
            return document;
        }
        ....
    }
    ....
}

興味深いコードが判明しました。筆頭著者は、次の拡張子を持つすべてのドキュメントを削除します。.pptx、.pptm、ではありません。ポックスと。potm、次に、それらの間に.xlsxと.xlsmがないことを確認することにしました。PresentationDocument関数は、間違いなくリファクタリングの犠牲になっています。



警告



7V3022式 'OpenSettings.MarkupCompatibilityProcessSettings == null'は常にfalseです。 OpenXmlPackage.cs 661

public MarkupCompatibilityProcessSettings MarkupCompatibilityProcessSettings
{
    get
    {
        if (_mcSettings is null)
        {
            _mcSettings = new MarkupCompatibilityProcessSettings(....);
        }

        return _mcSettings;
    }

    set
    {
        _mcSettings = value;
    }
}

public MarkupCompatibilityProcessSettings MarkupCompatibilityProcessSettings
{
    get
    {
        if (OpenSettings.MarkupCompatibilityProcessSettings == null) // <=
        {
            return new MarkupCompatibilityProcessSettings(....);
        }
        else
        {
            return OpenSettings.MarkupCompatibilityProcessSettings;
        }
    }
}

MarkupCompatibilityProcessSettings プロパティがnullを返すことはありません。ゲッターでクラスフィールドがnullであることが判明した場合、オブジェクトは新しいもので上書きされます。また、これは1つのプロパティへの再帰的な呼び出しではなく、異なるクラスからの同じ名前のプロパティであることに注意してください。おそらく、いくつかの混乱が不必要なチェックの作成につながっています。

その他の警告

警告



1V3080nullの逆参照の可能性。'previousSibling'を調べることを検討してください。OpenXmlCompositeElement.cs 380

public OpenXmlElement PreviousSibling()
{
    if (!(Parent is OpenXmlCompositeElement parent))
    {
        return null;
    }
    ....
}

public override T InsertBefore<T>(T newChild, OpenXmlElement referenceChild)
{
    ....
    OpenXmlElement previousSibling = nextNode.PreviousSibling();
    prevNode.Next = nextNode;
    previousSibling.Next = prevNode;    // <=
    ....
}

そして、これは追加の検証だけでは不十分な例です。PriorSiblingメソッドはnullを返す可能性があり、この関数の結果はチェックせずにすぐに使用されます。



さらに2つの危険な場所：

• V3080nullの逆参照の可能性。'prevNode'を調べることを検討してください。OpenXmlCompositeElement.cs 489
• V3080nullの逆参照の可能性。'prevNode'を調べることを検討してください。OpenXmlCompositeElement.cs 497

警告



2V3093 '＆'演算子は両方のオペランドを評価します。おそらく、代わりに短絡「&&」演算子を使用する必要があります。UniqueAttributeValueConstraint.cs 60

public override ValidationErrorInfo ValidateCore(ValidationContext context)
{
    ....
    foreach (var e in root.Descendants(....))
    {
        if (e != element & e.GetType() == elementType) // <=
        {
            var eValue = e.ParsedState.Attributes[_attribute];

            if (eValue.HasValue && _comparer.Equals(....))
            {
                return true;
            }
        }
    }
    ....
}

一部の人々は、「＆」演算子を論理式に適用すべきではない場所に適用することを好みます。この演算子の場合、最初の結果に関係なく、2番目のオペランドが最初に評価されます。これはここではそれほど深刻なエラーではありませんが、リファクタリング後のこのようなずさんなコードは、潜在的なNullReferenceException例外につながる可能性があります。



警告



3V3097考えられる例外：[Serializable]でマークされたタイプに、[NonSerialized]でマークされていないシリアル化できないメンバーが含まれています。 OpenXmlPackageValidationEventArgs.cs 15

[Serializable]
[Obsolete(ObsoleteAttributeMessages.ObsoleteV1ValidationFunctionality, false)]
[EditorBrowsable(EditorBrowsableState.Never)]
public sealed class OpenXmlPackageValidationEventArgs : EventArgs
{
    private string _message;

    [NonSerialized]
    private readonly object _sender;

    [NonSerialized]
    private OpenXmlPart _subPart;

    [NonSerialized]
    private OpenXmlPart _part;

    ....

    internal DataPartReferenceRelationship
        DataPartReferenceRelationship { get; set; } // <=
}

プロパティの1つがシリアル化不可としてマークされるのを忘れたため 、OpenXmlPackageValidationEventArgsクラスのシリアル化が失敗する場合があります。または、このプロパティの戻りタイプをシリアル化できるように変更する必要があります。そうしないと、実行時に例外が発生する可能性があります。

結論

私たち会社は、マイクロソフトのプロジェクトとテクノロジーが大好きです。PVS-Studioで検証されたオープンソースプロジェクトを一覧表示するセクションでは、Microsoft用に別のセクションを割り当てました。すでに21のプロジェクトが蓄積されており、そのうち26の記事が書かれています。これは27日です。



私たちの顧客の中にマイクロソフトがあるかどうか疑問に思っていると思います。答えはイエスです！しかし、これは世界中の開発をリードする巨大な企業であることを忘れないでください。プロジェクトですでにPVS-Studioを使用している部門は確かにありますが、使用していない部門はさらに多くあります。また、オープンソースプロジェクトでの経験から、バグを見つけるための優れたツールが明らかに不足していることがわかります;）。

C ++、C＃、およびJavaでのコードの分析に関心のある方のためのニュースからのニュース：最近、OWASP標準のサポートを追加し、その適用範囲を積極的に拡大しています。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクSvyatoslavRazmyslovを使用してください。MicrosoftのOpenXMLSDKのコード品質の分析。