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

Fixing #935: Secure function against random data return. #942

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@susnux
Contributor

susnux commented Aug 13, 2015

As described in bug #935, this will secure
sf::Uint32 factorToGlConstant(sf::BlendMode::Factor blendFactor)
and
sf::Uint32 equationToGlConstant(sf::BlendMode::Equation blendEquation)
from RenderTarget.cpp against returning random data.
The common consent was to apply the patch and add an assert on top.

See also discussion in bug report #935.

Show outdated Hide outdated src/SFML/Graphics/RenderTarget.cpp Outdated
@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 14, 2015

Contributor

@zsbzsb No I need the sf:: namespace.
These functions are not in the sf namespace, if I do not use sf:: I get:
error: ‘err’ was not declared in this scope

Contributor

susnux commented Aug 14, 2015

@zsbzsb No I need the sf:: namespace.
These functions are not in the sf namespace, if I do not use sf:: I get:
error: ‘err’ was not declared in this scope

@TankOs TankOs added this to the 2.3.2 milestone Aug 14, 2015

@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 16, 2015

Contributor

As mentioned I moved the assert behind the error message.
If you want I squash the commits to one.

Contributor

susnux commented Aug 16, 2015

As mentioned I moved the assert behind the error message.
If you want I squash the commits to one.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 17, 2015

Member

The patch looks ideal now, thank you. 👍 🐈

Member

TankOs commented Aug 17, 2015

The patch looks ideal now, thank you. 👍 🐈

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Aug 17, 2015

Member

Looks good 👍 Just needs to be squashed now 😉

Member

zsbzsb commented Aug 17, 2015

Looks good 👍 Just needs to be squashed now 😉

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 17, 2015

Member

I think there should be an empty line after the closing }.

And maybe write "Fall back to sf::BlendMode::Add" instead of "GLEXT_GL_FUNC_ADD", because the user doesn't know about the OpenGL behind the scenes. Same for "sf::BlendMode::Zero"/"GL_ZERO".

Member

Bromeon commented Aug 17, 2015

I think there should be an empty line after the closing }.

And maybe write "Fall back to sf::BlendMode::Add" instead of "GLEXT_GL_FUNC_ADD", because the user doesn't know about the OpenGL behind the scenes. Same for "sf::BlendMode::Zero"/"GL_ZERO".

@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 17, 2015

Contributor

Alright, I see this newline is common code style in SFML, so I have added it ;-)
Hoping for the last one of three thumb-up, then I squash the commits.

Contributor

susnux commented Aug 17, 2015

Alright, I see this newline is common code style in SFML, so I have added it ;-)
Hoping for the last one of three thumb-up, then I squash the commits.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 17, 2015

Member

👍 after squash

Member

Bromeon commented Aug 17, 2015

👍 after squash

@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 17, 2015

Contributor

Ok squashed.

Contributor

susnux commented Aug 17, 2015

Ok squashed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 19, 2015

Member

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

Member

eXpl0it3r commented Aug 19, 2015

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

@eXpl0it3r eXpl0it3r self-assigned this Aug 24, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 24, 2015

Member

Merged in 0f1dc5a on branch 2.3.x

Member

eXpl0it3r commented Aug 24, 2015

Merged in 0f1dc5a on branch 2.3.x

@eXpl0it3r eXpl0it3r closed this Aug 24, 2015

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