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

Initialize Font::m_stroker #1059

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@texus
Contributor

texus commented Feb 10, 2016

The following code crashes on android with the latest SFML version. The bug was introduced in 957cabb, earlier sfml versions don't crash on this code.

#include <SFML/Graphics.hpp>
#include <memory>

int main()
{
    std::shared_ptr<sf::Font> font3;

    sf::Font font1;
    font1.loadFromFile("sansation.ttf");

    font3 = std::make_shared<sf::Font>(font1);

    sf::Font font2;
    font2.loadFromFile("sansation.ttf");

    font3 = std::make_shared<sf::Font>(font2);
}

The backtrace in adb logcat:

02-09 18:10:35.218   949   949 F DEBUG   : backtrace:
02-09 18:10:35.218   949   949 F DEBUG   :     #00 pc 00058eef  /data/app/com.example.tgui-1/lib/x86/libsfml-graphics.so (FT_Stroker_Done+31)
02-09 18:10:35.218   949   949 F DEBUG   :     #01 pc 00019ea6  /data/app/com.example.tgui-1/lib/x86/libsfml-graphics.so (sf::Font::cleanup()+310)
02-09 18:10:35.218   949   949 F DEBUG   :     #02 pc 0001b91e  /data/app/com.example.tgui-1/lib/x86/libsfml-graphics.so (sf::Font::~Font()+30)
02-09 18:10:35.218   949   949 F DEBUG   :     #03 pc 00002dee  /data/app/com.example.tgui-1/lib/x86/libtgui-example.so (std::__1::__shared_ptr_emplace<sf::Font, std::__1::allocator<sf::Font> >::__on_zero_shared()+30)
02-09 18:10:35.218   949   949 F DEBUG   :     #04 pc 00089315  /data/app/com.example.tgui-1/lib/x86/libc++_shared.so (std::__1::__shared_weak_count::__release_shared()+53)
02-09 18:10:35.218   949   949 F DEBUG   :     #05 pc 00002b26  /data/app/com.example.tgui-1/lib/x86/libtgui-example.so (main+422)
02-09 18:10:35.218   949   949 F DEBUG   :     #06 pc 00002c98  /data/app/com.example.tgui-1/lib/x86/libtgui-example.so (sf::priv::main(sf::priv::ActivityStates*)+152)
02-09 18:10:35.218   949   949 F DEBUG   :     #07 pc 00002fd0  /data/app/com.example.tgui-1/lib/x86/libtgui-example.so (sf::priv::ThreadFunctorWithArg<void* (*)(sf::priv::ActivityStates*), sf::priv::ActivityStates*>::run()+16)
02-09 18:10:35.218   949   949 F DEBUG   :     #08 pc 0000e392  /data/app/com.example.tgui-1/lib/x86/libsfml-system.so (sf::Thread::run()+18)
02-09 18:10:35.218   949   949 F DEBUG   :     #09 pc 0000f54b  /data/app/com.example.tgui-1/lib/x86/libsfml-system.so
02-09 18:10:35.218   949   949 F DEBUG   :     #10 pc 00080a93  /system/lib/libc.so (__pthread_start(void*)+56)
02-09 18:10:35.218   949   949 F DEBUG   :     #11 pc 00021952  /system/lib/libc.so (__start_thread+25)
02-09 18:10:35.218   949   949 F DEBUG   :     #12 pc 000170b6  /system/lib/libc.so (__bionic_clone+70)

After looking at the SFML source code the following seems to happen: When font1 is loaded, its m_stroker member variable is set, but when making a copy and putting it in font3, the variable is left uninitialized. When reassigning font3, the font that it contained is destructed and it crashes in the cleanup function because m_stroker was not initialized.

The fix is to simply initialize m_stroker in the constructor and also copying its value in the copy constructor.

Forum post: http://en.sfml-dev.org/forums/index.php?topic=19776.msg142320#msg142320

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 10, 2016

Member

Looks good to me. No other attribute seems to be uninitialised. 👍

Member

mantognini commented Feb 10, 2016

Looks good to me. No other attribute seems to be uninitialised. 👍

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Feb 10, 2016

Member

Don't forget operator=.

Member

Bromeon commented Feb 10, 2016

Don't forget operator=.

@texus

This comment has been minimized.

Show comment
Hide comment
@texus

texus Feb 10, 2016

Contributor

Ok, the commit has been updated to also copy the variable in operator=

Contributor

texus commented Feb 10, 2016

Ok, the commit has been updated to also copy the variable in operator=

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 10, 2016

Member

Good catch @Bromeon!

Member

mantognini commented Feb 10, 2016

Good catch @Bromeon!

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Feb 12, 2016

Member

👍 from the person that forgot about it 😛

I think this also goes to show how much unit testing could help us as this would have most likely caused a test to fail.

Member

zsbzsb commented Feb 12, 2016

👍 from the person that forgot about it 😛

I think this also goes to show how much unit testing could help us as this would have most likely caused a test to fail.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 18, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 18, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 1, 2016

Member

src/SFML/Graphics/Font.cpp:444: warning: (warning) Member variable 'Font::m_stream' is not assigned a value in 'Font::operator='.

You'll have to assign "something" for m_stream by using the Android #define thingy. See the copy/constructor.

Member

eXpl0it3r commented Mar 1, 2016

src/SFML/Graphics/Font.cpp:444: warning: (warning) Member variable 'Font::m_stream' is not assigned a value in 'Font::operator='.

You'll have to assign "something" for m_stream by using the Android #define thingy. See the copy/constructor.

@texus

This comment has been minimized.

Show comment
Hide comment
@texus

texus Mar 1, 2016

Contributor

It isn't really related to this pull request, but I can add it if you want. As a separate commit or merged with the previous one?

Contributor

texus commented Mar 1, 2016

It isn't really related to this pull request, but I can add it if you want. As a separate commit or merged with the previous one?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 1, 2016

Member

Since this is very specific to Android and not related to m_stroker, I'd say a different commit. But it's worth fixing. :-)

Member

mantognini commented Mar 1, 2016

Since this is very specific to Android and not related to m_stroker, I'd say a different commit. But it's worth fixing. :-)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 2, 2016

Member

Yeah I didn't really think about it. I just saw the line in the static analysis report next to the others and thought it would make sense to report it here. Thanks for adding it!

Comparing operator= and the copy constructor I wonder if the m_stream shouldn't also be set to NULL. I guess we'll need @MarioLiebisch.

Member

eXpl0it3r commented Mar 2, 2016

Yeah I didn't really think about it. I just saw the line in the static analysis report next to the others and thought it would make sense to report it here. Thanks for adding it!

Comparing operator= and the copy constructor I wonder if the m_stream shouldn't also be set to NULL. I guess we'll need @MarioLiebisch.

@texus

This comment has been minimized.

Show comment
Hide comment
@texus

texus Mar 2, 2016

Contributor

It is being set to NULL because temp.m_stream is NULL.
But now you no longer have to change the operator= if you decide to at some point change the code in the copy constructor, so I think this code is better than assigning NULL directly.

Contributor

texus commented Mar 2, 2016

It is being set to NULL because temp.m_stream is NULL.
But now you no longer have to change the operator= if you decide to at some point change the code in the copy constructor, so I think this code is better than assigning NULL directly.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 2, 2016

Member

Ah I see. Sounds good.

Member

eXpl0it3r commented Mar 2, 2016

Ah I see. Sounds good.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 2, 2016

Member

Yes, if possible, copy-and-swap should be preferred over custom logic in assignment operators -- it's more clear as it's a common idiom. Even our inline variant that we use in SFML ;)

Member

Bromeon commented Mar 2, 2016

Yes, if possible, copy-and-swap should be preferred over custom logic in assignment operators -- it's more clear as it's a common idiom. Even our inline variant that we use in SFML ;)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 4, 2016

Member

Merged in 23ea17e

Member

eXpl0it3r commented Mar 4, 2016

Merged in 23ea17e

@eXpl0it3r eXpl0it3r closed this Mar 4, 2016

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