Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMON: Use appropriate equals() in BaseString #2601

Conversation

@mparnaudeau
Copy link
Contributor

@mparnaudeau mparnaudeau commented Nov 5, 2020

Remove a useless creation of a BaseString object with a C string in
comparison operators of this class, because there is a function equals()
that accepts a C string in parameter. So this was not necessary but
moreover the created BaseString object was never freed.

@phcoder
Copy link
Contributor

@phcoder phcoder commented Nov 5, 2020

Remove a useless creation of a BaseString object with a C string in
comparison operators of this class, because there is a function equals()
that accepts a C string in parameter. So this was not necessary but
moreover the created BaseString object was never freed.

Why would it be never freed? It's a prvalue and so it's destroyed at the end of the statement or do I miss something?

@criezy
Copy link
Member

@criezy criezy commented Nov 5, 2020

There is no memory leak (or I am also missing something).
But it might still be a good idea to avoid the creation of a temporary BaseString.

However the commit message does not follow our rules. Can you please fix that, and also remove the mention of a memory leak since that is incorrect.

@mparnaudeau
Copy link
Contributor Author

@mparnaudeau mparnaudeau commented Nov 5, 2020

I confirm there is a memory leak (reason why I looked at that): in the creation of the BaseString object in == and != operators, initWithValueTypeStr() is called and in this function, there is this dynamic allocation of _str ... that is not freed then:

	if (len >= _builtinCapacity) {
		// Not enough internal storage, so allocate more
		_extern._capacity = computeCapacity(len + 1);
		_extern._refCount = nullptr;
		_str = new value_type[_extern._capacity];
		assert(_str != nullptr);
	}

@criezy OK, I will reformat the commit. I suppose the line size exceeded the limit (that is short, I have to say)

@mparnaudeau mparnaudeau force-pushed the mparnaudeau:fix_memory_leak_comparing_basestring_and_cstring branch from 1f0556f to e2ab39a Nov 5, 2020
@mparnaudeau
Copy link
Contributor Author

@mparnaudeau mparnaudeau commented Nov 5, 2020

I'm sorry, my initial description was about a BaseString object not freed but the leak is on _str allocated dynamically in external storage. I updated my commit description.

@criezy
Copy link
Member

@criezy criezy commented Nov 5, 2020

The main issue with the commit message is the missing subsystem (in this case COMMON) at the start of the first line of the commit message.

And I still think there is no memory leak.
Do you have some information to the contrary? For example is this something reported by valgrind or similar tool?
We know that static analysis tools struggle with this class because of the reference counter, and report false positives. But if you have a more tangible information about there being a memory leak, it would be useful to have the details.

_str is a member variable from the class and it is deleted from its destructor if needed.
There are two instantiations of BaseString, and the destructor for both of those calls decRefCount():

String::~String() {
	decRefCount(_extern._refCount);
}

and

U32String::~U32String() {
	decRefCount(_extern._refCount);
}

And this is where the _str variable is freed (if it is not also being used by another BaseString, which we know by checking the _extern._refCount):

TEMPLATE
void BASESTRING::decRefCount(int *oldRefCount) {
	if (isStorageIntern())
		return;

	if (oldRefCount) {
		--(*oldRefCount);
	}
	if (!oldRefCount || *oldRefCount <= 0) {
		// The ref count reached zero, so we free the string storage
		// and the ref count storage.
		if (oldRefCount) {
			assert(g_refCountPool);
			g_refCountPool->freeChunk(oldRefCount);
		}

		delete[] _str;

		// Even though _str points to a freed memory block now,
		// we do not change its value, because any code that calls
		// decRefCount will have to do this afterwards anyway.
	}
}

Edit: I have now merged the two destructors. So the destructor of BaseString now calls decRefCount() instead of doing that in the destructors of the derived classes.

@mparnaudeau
Copy link
Contributor Author

@mparnaudeau mparnaudeau commented Nov 6, 2020

@criezy Thank you very much for the detailed explanation.

You talked about destructors of String. But it our case (the comparison operators in BaseString), String is not involved as this class inherits from BaseString but we only manipulates BaseString objects.

To detect memory leaks, I built scummvm with compilation and link option -fsanitize=address what reported:

    #0 0x7fa4acdb4ef0 in operator new[](unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xeaef0)
    #1 0x55bd68125c07 in Common::BaseString<char>::initWithValueTypeStr(char const*, unsigned int) common/base-str.cpp:231
    #2 0x55bd6812600a in Common::BaseString<char>::BaseString(char const*) common/base-str.cpp:97
    #3 0x55bd681260c9 in Common::BaseString<char>::operator==(char const*) const common/base-str.cpp:315
    #4 0x55bd673649e3 in OpenGL::ContextGL::initialize(OpenGL::ContextOGLType) graphics/opengl/context.cpp:94
    #5 0x55bd6519b3f0 in OSystem_SDL::detectFramebufferSupport() backends/platform/sdl/sdl.cpp:333
    #6 0x55bd651a311d in OSystem_SDL::initBackend() backends/platform/sdl/sdl.cpp:220
    #7 0x55bd651ab8cb in OSystem_POSIX::initBackend() backends/platform/sdl/posix/posix.cpp:93
    #8 0x55bd651bbaeb in scummvm_main base/main.cpp:473
    #9 0x55bd651ab26b in main backends/platform/sdl/posix/posix-main.cpp:45
    #10 0x7fa4a9fed09a in __libc_start_main ../csu/libc-start.c:308

I've just tested your updated version and leaks have disappeared.

So I update my commit, just to use the appropriate call to equals().

Remove a useless creation of a BaseString object with a C string in
comparison operators of this class, because there is a function
equals() that accepts a C string in parameter.
@mparnaudeau mparnaudeau force-pushed the mparnaudeau:fix_memory_leak_comparing_basestring_and_cstring branch from e2ab39a to 1064c16 Nov 6, 2020
@mparnaudeau mparnaudeau changed the title Fix memory leak comparing a BaseString with a C string COMMON: Use appropriate equals() in BaseString Nov 6, 2020
@criezy
Copy link
Member

@criezy criezy commented Nov 6, 2020

Ah yes. Thank you for the asan info! That was indeed a memory leak.
The BaseString class is not meant to be used directly, so it was assumed that we always had either a String or U32String. But there is one place where we create (and destroy) a BaseString directly: in the BaseString implementation itself. And there it was indeed not freeing the _str memory. The change I made yesterday to merge the String and U32String destructors and move that code to the BaseString destructor should indeed fix that. Thank you for testing that this is indeed the case.

Your commit look good now. So I am merging it.
Thank you again for this pull request and the info you provided.

@criezy criezy merged commit 4880fab into scummvm:master Nov 6, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.