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

Fixed potential buffer overrun in Win32 OpenGL error handling #1246

Merged
merged 1 commit into from Jan 23, 2019

Conversation

MarioLiebisch
Copy link
Member

This fixes issue #1245.

String errMsg(ss.str());

PTCHAR buffer;
FormatMessage(FORMAT_MESSAGE_MAX_WIDTH_MASK | FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, NULL, errorCode, 0, reinterpret_cast<PTCHAR>(&buffer), 0, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is PTCHAR the right type for the cast?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. While you're supposed to pass a TCHAR** to retrieve a system allocated buffer, the signature only accepts a TCHAR* (i.e. PTCHAR). Remember, we're in C land here, so no overloads. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is already a PTCHAR, so the & operator would make it a PTCHAR*?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you basically cast away the second layer of reference for the signature alone. You'll pass a PTCHAR*, because it's a return value here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not cast it to a LPTSTR then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually copied the snippet from SFML's Joystick handling. That would probably be an even "more correct" solution I guess.

Copy link

@GatoRat GatoRat Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still has a serious bug:

The FORMAT_MESSAGE_IGNORE_INSERTS flag should be added to the FormatMessage call.

Be aware that buffer may be null.

(In the original code the above flag should be added, the buffer length set correctly and the return code checked for zero.)

@eXpl0it3r
Copy link
Member

Bump.

Any alternative solution?

@binary1248
Copy link
Member

@MarioLiebisch

String getErrorString(DWORD errorCode)
{
    PTCHAR buffer;
    if (FormatMessage(FORMAT_MESSAGE_MAX_WIDTH_MASK | FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, errorCode, 0, reinterpret_cast<LPTSTR>(&buffer), 256, NULL) != 0)
    {
        String errMsg(buffer);
        LocalFree(buffer);
        return errMsg;
    }

    std::ostringstream ss;
    ss << "Error " << errorCode;
    return String(ss.str());
}

@eXpl0it3r
Copy link
Member

Any update on this one, @MarioLiebisch? Or do you want to take over, @binary1248?

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Nov 26, 2018
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Nov 26, 2018
@eXpl0it3r eXpl0it3r moved this from Discussion to WIP in SFML 2.6.0 Nov 26, 2018
@eXpl0it3r eXpl0it3r moved this from WIP to Review & Testing in SFML 2.6.0 Jan 19, 2019
@eXpl0it3r
Copy link
Member

@binary1248 can you re-review this? I applied the suggested change.

@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.6.0 Jan 19, 2019
@eXpl0it3r eXpl0it3r merged commit 0980e90 into master Jan 23, 2019
SFML 2.6.0 automation moved this from Ready to Done Jan 23, 2019
@eXpl0it3r eXpl0it3r deleted the bugfix/windows-formatmessage branch January 23, 2019 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants