Sonar-dotnet: False postive on S4158: variable mismatch

Created on 11 Dec 2018 · 4Comments · Source: SonarSource/sonar-dotnet

Description

public virtual object Clone()
{
    var clonedEntry = (Entry)MemberwiseClone();
    clonedEntry._id = Guid.NewGuid();
    clonedEntry._lines = new List<Line>();
   _lines.ForEach(line => clonedEntry.AddLine(line.Clone() as Line));
    return clonedEntry;
}

Will falsely raise. It seems to be different from earlier raised issue.

Workarround

public virtual object Clone()
{
    var clonedEntry = (Entry)MemberwiseClone();
    clonedEntry._id = Guid.NewGuid();
    clonedEntry._lines = new List<Line>();
    forach(var line in clonedEntry._lines )
   {
       clonedEntry.AddLine(line.Clone() as Line);
   }
    return clonedEntry;
}

Related information

7.9.1.7622
Visual Studio 14.0

C# CFG False Positive

Source

Corniel

👍3

Most helpful comment

As we are currently working on CFG/Symbolic Execution, I investigated a bit more this issue to understand the underlying problem.
For future reference: symbolic execution currently only keeps track of fields in the current class being analyzed. However, symbolic values of objects (variables, fields, parameters, ...) are mapped to their symbols (https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.isymbol?view=roslyn-dotnet). This is what is causing the issue: when an instance of the current class being analyzed is created within a method, the fields of the newly created instance and the current one are aliased since they are represented by the same ISymbol object. Fixing this implies changing how symbolic values are stored and mapped inside the ProgramState.

Here are two reproducers to illustrate this:

    class A
    {
        public List<string> _lines;

        public void doSomething()
        {
            var obj1 = new A();
            obj1._lines = new List<string>(); // obj1._lines and this._lines are both represented by the 'A#List<string>_lines' symbol
            _lines.ForEach(line => obj1._lines.Add(line)); // FP
        }
    }

    class B
    {
        public List<string> _lines;

        public void doSomething()
        {
            var obj1 = new B();
            var obj2 = new B();
            obj1._lines = new List<string>();
            obj1._lines.Add("str");
            obj2._lines = new List<string>();
            obj1._lines.ForEach(line => obj2._lines.Add(line)); // FP
        }
    }

christophe-zurn-sonarsource on 6 Aug 2019

👍5

All 4 comments

Here are two reproducers to illustrate this:

    class A
    {
        public List<string> _lines;

        public void doSomething()
        {
            var obj1 = new A();
            obj1._lines = new List<string>(); // obj1._lines and this._lines are both represented by the 'A#List<string>_lines' symbol
            _lines.ForEach(line => obj1._lines.Add(line)); // FP
        }
    }

    class B
    {
        public List<string> _lines;

        public void doSomething()
        {
            var obj1 = new B();
            var obj2 = new B();
            obj1._lines = new List<string>();
            obj1._lines.Add("str");
            obj2._lines = new List<string>();
            obj1._lines.ForEach(line => obj2._lines.Add(line)); // FP
        }
    }

christophe-zurn-sonarsource on 6 Aug 2019

👍5

Another false positive case for S4158 involving local method and event:

        public void RemoveErrorsTest()
        {
            // Arrange
            var module = new TestModule();

            var errorsToRemove = new List<int>();
            var RemovedErrors = new List<ApplicationModuleError>();

            void Module_ErrorsRemovingEnded(object sender, ApplicationModuleErrorsRemovedEventArgs e)
            {
                errorsToRemove.AddRange(e.ErrorsToRemove);
                RemovedErrors.AddRange(e.RemovedErrors);
            }

            module.ErrorsRemovingEnded += Module_ErrorsRemovingEnded;

            // Act
            module.RemoveErrors(TestErrors.TestError0);

            // Assert
            Assert.AreEqual((int)TestErrors.TestError0, errorsToRemove[0]); // <-- False positive here
            Assert.AreEqual(0, RemovedErrors.Count);
        }

Thieum on 13 Feb 2020

Had the same problem! When can I expect a fix?
The issue is very old (2018) and a little bit annoying!

var clone = MemberwiseClone() as MyClass;
clone.myList= new List<Item>();
myList.ForEach(item => clone.myList.Add(item.Clone() as Item));

stebla27 on 8 Feb 2021

Hi @stebla27,

We don't have any ETA to fix reported issues. Prerequisite for this to be fixed is reimplementing our Symbolic Execution Engine that will be done in MMF-2229 in 2021. That will directly fix or unblock all issues related to CFG and SE.