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

Static Memory Leaks #194

Open
geometrian opened this issue Aug 20, 2019 · 3 comments
Open

Static Memory Leaks #194

geometrian opened this issue Aug 20, 2019 · 3 comments

Comments

@geometrian
Copy link
Contributor

Similar to #84 but occurs regardless of whether any TinyGLTF function is actually called. Typical output with the CRT debug heap is:

{158} normal block at 0x00000243D5CCDC20, 80 bytes long.
Data: 41 42 43 44 45 46 47 48 49 4A 4B 4C 4D 4E 4F 50
{157} normal block at 0x00000243D5CF75B0, 16 bytes long.
Data: < LN > D8 D8 4C 4E F7 7F 00 00 00 00 00 00 00 00 00 00

I was able to easily track down the first one to line 1714, where a static const std::string is created in a static context.

I notice there are some Clang warnings which detected the problem and have been explicitly disabled, which makes me suspect it was not viewed to be a problem by whoever wrote it. If desired, I can list some other reasons, besides that they make leak checking difficult, why dynamic initialization in static contexts is a bad idea and an anti-pattern, although it is also easily google-able.

Happily, this string is only used in a couple of places, and it's only a std::string because of some std convenience functions run on it. It could maybe be fixed even-more-easily by making the static declaration just a char const* and then making a std::string_view whenever you need to do something with it. The section seems to come from the base64 library, so maybe this should be patched upstream, too.

The other leak is also somewhere in the implementation section, and does not appear to be in stb_image or stb_image_write. My guess is that it's a static allocation somewhere in a function?

@syoyo
Copy link
Owner

syoyo commented Aug 20, 2019

static const std::string is created in a static context.

I am using the original base64 code as is: https://github.com/ReneNyffenegger/cpp-base64/blob/master/base64.cpp#L35

Although I can easily modify it to use chat const*(We can't use string_view since tinygltf is targetting C++11 compiler), I think static const std::string gives negligible impact to the runtime. Do you see some memory impact or runtime side-effects by using static const std::string?

@jrkoonce
Copy link
Contributor

Do you see some memory impact or runtime side-effects by using static const std::string?

Most people will not. But when it is a problem, it is a big problem with no solution but to remove the static initialization usually. Typical use case where heap allocation in static initialization causes significant pain is when a custom memory manager is used instead of the C-runtime's memory manager, or you want explicit control of heap allocations. This is 3rd party code, and this case is easily fixed if it is a problem for someone. But, I wanted to iterate that static initialization in general can be a source of lots of problems and it should be done very carefully.

@syoyo
Copy link
Owner

syoyo commented Aug 24, 2019

Most people will not. But when it is a problem, it is a big problem with no solution but to remove the static initialization usually

Okay, its a easy job to do, so I have removed const static std::string global variable in this commit: ff51570

Another static variable(16bytes of the leak) will be this: https://github.com/syoyo/tinygltf/blob/master/tiny_gltf.h#L226

This is a bit difficult to deal with since we want to support compiling code with no exception. Possible solution would be to introduce std::optional like type for the return type of Value::Get function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants