To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Analyzing the Code Quality of Microsoft…

Analyzing the Code Quality of Microsoft's Open XML SDK

Nov 27 2020

My first encounter with Open XML SDK took place when I was looking for a library that I could use to create some accounting documents in Word. After more than 7 years of working with Word API, I wanted to try something new and easier-to-use. That's how I learned that Microsoft offered an alternative solution. As tradition has it, before our team adopts any program or library, we check them with the PVS-Studio analyzer.

0777_OpenXML_SDK/image1.png

Introduction

Office Open XML, also known as OpenXML or OOXML, is an XML-based format for representing office documents, including text documents, spreadsheets, presentations, as well as charts, figures, and other types of graphical content. The specification was developed by Microsoft and approved by ECMA International in 2006. In June 2014, Microsoft released Open XML SDK as an open-source project. The source files are currently available on GitHub under the MIT license.

I scanned the library's source code with the static analyzer PVS-Studio. This is a tool for detecting software bugs and potential vulnerabilities in the source code of programs in C, C++, C#, and Java. The analyzer runs on 64-bit Windows, Linux, and macOS.

The project is fairly small, so the number of warnings is small too. But they were prominent enough to inspire my choice of the image for this post. You see, there are too many useless conditional statements in this project. I believe that refactoring all such spots would help make the code much shorter and therefore clearer.

Why still Word API and not Open XML SDK?

As you have guessed from this title, I'm still using Word API in my work. There are a lot of downsides to this approach:

  • The API is old and cumbersome;
  • You have to have Microsoft Office installed on your computer;
  • You have to ship the distribution with the Office libraries included;
  • Word API's operation depends on the system's locale settings;
  • Low performance.

There's a funny story concerning the locale in particular. Windows provides a dozen of regional settings. We found that one of our servers was for some reason using a mishmash of the USA and UK locales, which caused our Word documents to substitute the ruble sign for the dollar sign, while the pound sign wasn't displayed at all. We solved the problem by tweaking the system's settings.

Now as I'm telling you all this, I'm once again asking myself why I keep using it....

But no, I still like Word API more, and I'll tell you why.

Here's what OOXML format looks like:

<?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>

Here, <w:r> (Word Run) is not a sentence or even a word – it's any block of text whose attributes are different from those of adjacent blocks.

This is programmed through code that looks something like this:

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

A document has its own special inner structure, and the same elements must be created in the code. In my opinion, the abstraction level of data access in Open XML SDK isn't deep enough. Creating a document using Word API is more comprehensible and takes less time – especially when you deal with spreadsheets and other complex data structures.

On the other hand, Open XML SDK helps solve a wide range of tasks. It can be used to create not only Word documents but Excel and PowerPoint documents as well. This library might well be a more preferable choice for some tasks, but I've decided to stick with Word API for now. We can't abandon Word altogether anyway since we are developing a plugin for Word for our corporate needs, and this task can be accomplished only using the Word API.

Two values of string

V3008 The '_rawOuterXml' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 164, 161. OpenXmlElement.cs 164

internal string RawOuterXml
{
    get => _rawOuterXml;

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

        _rawOuterXml = value;
    }
}

The string type can have two types of values: null and a text value. To use the latter is definitely a safer approach, but either is acceptable. In this particular project, the null value can't be used and the programmer overwrites it with string.Empty... at least, that was the idea. There's a mistake in RawOuterXml that makes it possible to assign the value null to the field and then get a NullReferenceException when attempting to access it.

V3022 Expression 'namespaceUri != null' is always 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)  // <=
        {
            ....
        }
        ....
    }
    ....
}

The same approach is used in this snippet as well. It's not a severe mistake, but you can still smell the bad refactoring. I'm almost sure one of the checks can be safely removed – that would make the code narrower and therefore easier to read.

On code compactness

0777_OpenXML_SDK/image2.png

V3009 It's odd that this method always returns one and the same value of '".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";
    }
}

I'm not sure if the programmer made some typo or simply wrote what they believed to be "neat" code. If you ask me, it doesn't make much sense to return so many similar values and the code can be simplified quite a bit.

It's not the only warning of this type. Here are two more:

  • V3009 It's odd that this method always returns one and the same value of '".xml"'. CustomPropertyPartTypeInfo.cs 25
  • V3009 It's odd that this method always returns one and the same value of '".bin"'. EmbeddedControlPersistenceBinaryDataPartTypeInfo.cs 22

I wonder how the programmer would explain their decision to write the code that way.

V3139 Two or more case-branches perform the same actions. 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;
        ....
    }
    ....
}

This snippet is less controversial than the previous one. I think the identical cases can be merged to make the code shorter and clearer.

Here are a few more issues of that kind:

  • V3139 Two or more case-branches perform the same actions. OpenXmlMiscNode.cs 312
  • V3139 Two or more case-branches perform the same actions. CustomPropertyPartTypeInfo.cs 30
  • V3139 Two or more case-branches perform the same actions. CustomXmlPartTypeInfo.cs 15
  • V3139 Two or more case-branches perform the same actions. OpenXmlElement.cs 1803

The infamous always true/false

We have finally reached the section covering examples that determined my choice of the picture for this article.

Warning 1

V3022 Expression 'Complete()' is always 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;
}

The IsComplete property is used twice, and it's clear from the code that the property's value won't change between the two checks. It means you can have the function simply return the second value of the ternary operator, i.e. true.

Warning 2

V3022 Expression '_elementStack.Count > 0' is always true. 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;
    }
    ....
}

If the number of elements on the _elementStack stack is different from 0, then it's obviously larger than 0. It means the code can be made at least 8 lines shorter.

Warning 3

V3022 Expression 'rootElement == null' is always 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(....);
  }
  ....
}

The CreateElement function can't return null. If the company has adopted the rule that xml nods be created using methods that either return a valid object or throw an exception, users that employ these methods don't have to overuse additional checks.

Warning 4

V3022 Expression 'nameProvider' is always not null. The operator '?.' is excessive. 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();
}

Here is the pattern of the is operator:

expr is type varname

If the result of the is expression is true, a nonnull reference will be written in varname. So its additional check for null is redundant.

Warning 5

V3022 Expression 'extension == ".xlsx" || extension == ".xlsm"' is always 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;
        }
        ....
    }
    ....
}

This is quite an interesting case. The programmer first filters off all documents whose extensions are different from .pptx, .pptm, .potx, and .potm, and then – just in case – decides to make sure there are no .xlsx and .xlsm documents left among those. The PresentationDocument function is definitely a victim of refactoring.

Warning 6

V3022 Expression 'OpenSettings.MarkupCompatibilityProcessSettings == null' is always 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;
        }
    }
}

The MarkupCompatibilityProcessSettings property never returns null. If the getter finds that the class's field has the null value, the object will be overwritten with a new one. Also, note that this is not a recursive call of one and the same property but rather properties of the same name from different classes. This confusion may have caused the developer to add the extra checks.

Other warnings

Warning 1

V3080 Possible null dereference. Consider inspecting '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;    // <=
    ....
}

Contrary to the previous examples, this one does require an additional check. The PreviousSibling method can return the value null, and it will be used right away without any check.

Two more potential null dereferences:

  • V3080 Possible null dereference. Consider inspecting 'prevNode'. OpenXmlCompositeElement.cs 489
  • V3080 Possible null dereference. Consider inspecting 'prevNode'. OpenXmlCompositeElement.cs 497

Warning 2

V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. 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;
            }
        }
    }
    ....
}

Some developers love applying the '&' operator to logical expressions without good reason. But whatever value its first operand evaluates to, the second operand will be evaluated anyway. In this particular case, it's not a critical mistake, but such careless code may start throwing NullReferenceExceptions after refactoring.

Warning 3

V3097 Possible exception: type marked by [Serializable] contains non-serializable members not marked by [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; } // <=
}

Serialization of the OpenXmlPackageValidationEventArgs class may fail because one of the properties is not marked as serializable. Alternatively, this can be fixed by making the property's return type serializable; otherwise, you risk getting an exception at runtime.

Conclusion

We, PVS-Studio developers, are fans of Microsoft projects and technologies. We even have a separate section dedicated to Microsoft on our page listing all open-source projects checked with PVS-Studio. That section already includes 21 projects covered in 26 articles. This one is the 27th.

I bet you are wondering if Microsoft is our client. Yes, it is! But keep in mind it's a huge corporation operating all over the world. Some of its subdivisions surely use PVS-Studio in their work, but many more don't! As our experience with open-source projects shows, the latter are obviously in need of a good bug-detecting tool ;).

GetFreeTrialImage

Those who follow news on analysis of C++, C#, and Java code may also be interested to know that we have recently added support of the OWASP standard and are actively covering it with our diagnostics.

Popular related articles
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept