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

Windows JoystickImpl exception thrown in WindowImpl constructor #701

Closed
skaughtx0r opened this Issue Sep 23, 2014 · 10 comments

Comments

Projects
None yet
5 participants
@skaughtx0r

skaughtx0r commented Sep 23, 2014

The latest commit to the SFML repo seems to have a bug in Windows. It crashes on start up, I traced the culprits to be here:
https://github.com/LaurentGomila/SFML/blob/master/src/SFML/Window/Win32/JoystickImpl.cpp#L76
https://github.com/LaurentGomila/SFML/blob/master/src/SFML/Window/Win32/JoystickImpl.cpp#L99
https://github.com/LaurentGomila/SFML/blob/master/src/SFML/Window/Win32/JoystickImpl.cpp#L116

I'm getting the following error:
Microsoft C++ exception: std::out_of_range at memory location 0x031de940

I'm not really sure what this line is supposed to accomplish, it appears that its trying to erase the 256th character from the string for some reason. The crash makes sense since the string is never set to have 256 characters and ends up having fewer when the string is dynamically created.

Removing these lines from the code fixed the problem for me.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

Interesting. That part of the code should probably be rewritten anyway. It's still using ANSI/wide char macros despite other parts of SFML forcing the wide character versions anyway. Will try to have a closer look at it tomorrow, unless someone else wants to do so.

Member

MarioLiebisch commented Sep 24, 2014

Interesting. That part of the code should probably be rewritten anyway. It's still using ANSI/wide char macros despite other parts of SFML forcing the wide character versions anyway. Will try to have a closer look at it tomorrow, unless someone else wants to do so.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 24, 2014

Member

The reason why the erase is there is to make sure that key names do not exceed 255 characters, as dictated by Microsoft documentation. If you remove the truncation, any time the name would be longer than 255 characters, "things would happen" 😉.

I merely refactored the old code, but forgot that erase is unconditional and throws an exception if there is nothing to erase. I'll fix this shortly, no worries 😉.

Member

binary1248 commented Sep 24, 2014

The reason why the erase is there is to make sure that key names do not exceed 255 characters, as dictated by Microsoft documentation. If you remove the truncation, any time the name would be longer than 255 characters, "things would happen" 😉.

I merely refactored the old code, but forgot that erase is unconditional and throws an exception if there is nothing to erase. I'll fix this shortly, no worries 😉.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

I assumed that, but if you erase the 256th character, wouldn't the 257th simply take its place?

Member

MarioLiebisch commented Sep 24, 2014

I assumed that, but if you erase the 256th character, wouldn't the 257th simply take its place?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 24, 2014

Member

std::string::erase() removes everything between that position and the end.

Member

binary1248 commented Sep 24, 2014

std::string::erase() removes everything between that position and the end.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

Ah okay, makes more sense.

Member

MarioLiebisch commented Sep 24, 2014

Ah okay, makes more sense.

@skaughtx0r

This comment has been minimized.

Show comment
Hide comment
@skaughtx0r

skaughtx0r Sep 24, 2014

It should probably check if the size is greater than 255 characters before attempting to erase then.

Thanks for looking at this!

skaughtx0r commented Sep 24, 2014

It should probably check if the size is greater than 255 characters before attempting to erase then.

Thanks for looking at this!

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

Since a truncated path most likely wouldn't work anyway, I'd just fail in that case. But is the limit still there? Can't check right now. Similar restrictions for file system paths are quite obsolete today.

Member

MarioLiebisch commented Sep 24, 2014

Since a truncated path most likely wouldn't work anyway, I'd just fail in that case. But is the limit still there? Can't check right now. Similar restrictions for file system paths are quite obsolete today.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 24, 2014

Member

This is really more of a sanity check. Such scenarios should never really occur during typical use (I hope 😛).

Member

binary1248 commented Sep 24, 2014

This is really more of a sanity check. Such scenarios should never really occur during typical use (I hope 😛).

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 24, 2014

Member

Aaaand... done. 😛.

Hopefully this time around more people with access to joysticks are able to test 😉.

Member

binary1248 commented Sep 24, 2014

Aaaand... done. 😛.

Hopefully this time around more people with access to joysticks are able to test 😉.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 24, 2014

Member

Superseded by PR #702.

Member

mantognini commented Sep 24, 2014

Superseded by PR #702.

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