AddressSanitizer find bugs in axmol engine

[crazyhappygame]

• axmol version: 4e664e6
• devices test on: Windows 10
• developing environments
  • NDK version: r19c
  • Xcode version: 12.4
  • Visual Studio:
    • VS version: 2022 (17.6.3)
    • MSVC version: 1929, 1934
    • Windows SDK version: 10.0.22621.0
  • cmake version: 3.25.2
    Steps to Reproduce:
    Windows, Visual studio 2022

1. Enable adress sanitizer for VS
   https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170
   by adding on the top of to CMakeLists.txt

add_compile_options(/fsanitize=address)

2. build and start cpp-test
3. Press "Start AutoTest"
4. After some time application crashed with

Sample errors:

1. ActionsProgressTests
   Address Sanitizer Error: Use of out-of-scope stack memory

void ProgressTimer::updateColor()
{
    if (!_sprite)
        return;

    if (!_vertexData.empty())
    {
        const Color4B& sc = _sprite->getQuad().tl.colors;
        for (int i = 0; i < _vertexData.size(); ++i)
        {
            _vertexData[i].colors = sc;
        }
    }
}

2. TextureCacheUnbindTest
   Address Sanitizer Error: Use of deallocated memory

        // release the asyncStruct
        delete asyncStruct;
        --_asyncRefCount;

Comments:
This kind of problems means that we are in undefined behavior zone and can not reason about program correctness.
This kind of issue could result in the problems seen in #1211

[crazyhappygame]

FYI. @rh101 @halx99 @DelinWorks @kiranb47

Visual studio support only AddressSanitizer https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170

Does any one of can build and check axmol on linux against all available sanitizers [address, memory, thread, undefined]

[rh101]

@crazyhappygame The ActionsProgressTests issue seems like a false positive from ASAN. It can be boiled down to this code which throws this error:

const Color4B& sc = _sprite->getQuad().tl.colors;
_vertexData[i].colors = sc;

If we change it to this, an error is still flagged by ASAN:

const auto& tl = _sprite->getQuad().tl;
_vertexData[i].colors = tl.colors;

For some reason the const reference is flagged as being out of scope, which shouldn't be the case. Unless there is something I'm not seeing, none of the code above should result in no errors.

This version of it does not throw an error:

const auto& quad = _sprite->getQuad();
_vertexData[i].colors = quad.tl.colors;

[rh101]

2. TextureCacheUnbindTest
Address Sanitizer Error: Use of deallocated memory

@crazyhappygame I couldn't reproduce this issue. Did you test it with the 32bit of 64bit build?

Once I did the change to fix issue 1 in ProgressTimer::updateColor(), the entire cpp_test auto test passed without any further ASAN exceptions (besides a few crashes that were fixed in #1259 that are unrelated to ASAN).

[crazyhappygame]

@rh101 ASAN does not produce false positive.
Below line is a real bug (not a false positive)

const Color4B& sc = _sprite->getQuad().tl.colors;

Clarification:
sprite returns by value V3F_C4B_T2F_Quad

V3F_C4B_T2F_Quad getQuad() const { return _quad; }

V3F_C4B_T2F_Quad temporary value is destroyed immediately and we keep reference to const Color4B& sc that points to non existing part of V3F_C4B_T2F_Quad

Possible fixes:

1. Store copy of Color4B

const Color4B sc = _sprite->getQuad().tl.colors;

2. Extend lifetime of rvalue

const auto& quad = _sprite->getQuad();

and later use quad

_vertexData[i].colors = quad.tl.colors;

Please check "Reference Lifetime Extension" e.g.:
https://abseil.io/tips/107

Generally:
This is OK. Reference Lifetime Extension.

const auto& temp = _sprite->getQuad();

This is WRONG. Reference Lifetime Extension does not work for any sub expression.

const auto& temp1 = _sprite->getQuad().tl;
const auto& temp2 = _sprite->getQuad().tl.color;

[halx99]

Universal Reference is better fix it: auto&& xxx = 获取Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: CHP ***@***.***> Sent: Sunday, July 9, 2023 3:31:36 PM To: axmolengine/axmol ***@***.***> Cc: Deal(一线灵) ***@***.***>; Mention ***@***.***> Subject: Re: [axmolengine/axmol] AddressSanitizer find bugs in axmol engine (Issue #1257) @rh101<https://github.com/rh101> ASAN does not produce false positive. Below line is a real bug (not a false positive) const Color4B& sc = _sprite->getQuad().tl.colors; Clarification: sprite returns by value V3F_C4B_T2F_Quad V3F_C4B_T2F_Quad getQuad() const { return _quad; } V3F_C4B_T2F_Quad temporary value is destroyed immediately and we keep reference to const Color4B& sc that points to non existing part of V3F_C4B_T2F_Quad Possible fixes: 1. Store copy of Color4B const Color4B sc = _sprite->getQuad().tl.colors; 1. Extend lifetime of rvalue const auto& quad = _sprite->getQuad(); and later use quad _vertexData[i].colors = quad.tl.colors; Please check "Reference Lifetime Extension" e.g.: https://abseil.io/tips/107 Generally: This is OK. Reference Lifetime Extension. const auto& temp = _sprite->getQuad(); This is WRONG. Reference Lifetime Extension does not work for any sub expression. const auto& temp1 = _sprite->getQuad().tl; const auto& temp2 = _sprite->getQuad().tl.color; ― Reply to this email directly, view it on GitHub<#1257 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABVHOJ7WYWJEYFUNPUYXKCDXPJM5RANCNFSM6AAAAAA2DBMBOQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[crazyhappygame]

2. TextureCacheUnbindTest
   Address Sanitizer Error: Use of deallocated memory

This looks like some threading problem. Most probably there is some race. We should run tread sanitizer to increase chance to reproduce this problem. I reproduce this problem on win 64 bit build

[rh101]

Clarification: sprite returns by value V3F_C4B_T2F_Quad

V3F_C4B_T2F_Quad getQuad() const { return _quad; }

V3F_C4B_T2F_Quad temporary value is destroyed immediately and we keep reference to const Color4B& sc that points to non existing part of V3F_C4B_T2F_Quad

You're absolutely right; I completely missed that the return value of getQuad isn't a reference.

[crazyhappygame]

Universal Reference is better fix it: auto&& xxx =

1. @halx99 Universal reference will not work. (update: double checked Universal reference && ASAN report error)
2. @rh101 getQuad returning by reference will work

[halx99]

auto&& quad = _sprite->getQuad();
temp._vertexData[i].colors = quad.tl.colors;

not work?

[crazyhappygame]

I checked below:

auto&& t = helpArrow->getQuad().tl.colors;

That should work

auto&& quad = _sprite->getQuad();
_vertexData[i].colors = quad.tl.colors;

There should be no difference how Reference Lifetime Extension works for below cases

auto&& t1= _sprite->getQuad();
const auto& t2 = _sprite->getQuad();
const V3F_C4B_T2F_Quad& t2 = _sprite->getQuad();

[rh101]

auto&& quad = _sprite->getQuad();
temp._vertexData[i].colors = quad.tl.colors;

Sorry, yes that does work, which is equivalent to:

const auto& quad = _sprite->getQuad();
_vertexData[i].colors = quad.tl.colors;

Which would be preferred?

1 - Change getQuad to return a const reference? So it would become:
const V3F_C4B_T2F_Quad& getQuad() const { return _quad; }

or

2 - Update it's usage in void ProgressTimer::updateColor() to:

auto&& quad = _sprite->getQuad();
_vertexData[i].colors = quad.tl.colors;

[crazyhappygame]

Both works for me.

Not sure how big can _vertexData vector and what compiler code is generated here but there is a chance that below could be the faster. There is no indirection in body loop quad.tl.colors

        const Color4B sc = _sprite->getQuad().tl.colors;
        for (int i = 0; i < _vertexData.size(); ++i)
        {
            _vertexData[i].colors = sc;
        }

And most probably changing to below will be some small performance and readability improvement

const Color4B sc = _sprite->getQuad().tl.colors;
for(auto& d: _vertexData){
   d.color = sc;
}

[rh101]

Not sure how big can _vertexData vector and what compiler code is generated here but there is a chance that below could be the faster. There is no indirection in body loop quad.tl.colors

        const Color4B sc = _sprite->getQuad().tl.colors;
        for (int i = 0; i < _vertexData.size(); ++i)
        {
            _vertexData[i].colors = sc;
        }

That would be an extra copy for the const Color4B sc = _sprite->getQuad().tl.colors;

If getQuads returned a const &:
const V3F_C4B_T2F_Quad& getQuad() const { return _quad; }

then the old code in ProgressTimer::updateColor() can remain as it is, and it will work, plus it won't be doing an extra copy since it's just getting the reference to colors.

const auto& sc = _sprite->getQuad().tl.colors;
for (int i = 0; i < _vertexData.size(); ++i)
{
    _vertexData[i].colors = sc;
}

Also, the other places that use getQuads get a copy of it, like this:

V3F_C4B_T2F_Quad quad = _sprite->getQuad();

All they do is either calculations on the values or copy the contents somewhere else, so if we change getQuads to return a const & we can remove a single copy operation, and just have all usages do:

const V3F_C4B_T2F_Quad& quad = _sprite->getQuad();
or
const auto& quad = _sprite->getQuad(); etc. etc.

[crazyhappygame]

I think this single copy does not matter here:

1. we create V3F_C4B_T2F_Quad (that is much bigger then Color4B)

V3F_C4B_T2F_Quad getQuad() const { return _quad; }

2. We copy Color4B every single loop iteration

_vertexData[i].colors = sc;

Most probably below would be the most performant and readable code

const V3F_C4B_T2F_Quad& getQuad() const { return _quad; }

and then

const Color4B& sc = _sprite->getQuad().tl.colors;
for(auto& d: _vertexData){
   d.color = sc;
}

[halx99]

const V3F_C4B_T2F_Quad& getQuad() const { is best