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

Fix wrong types passed to XChangeProperty #1171

Merged
merged 1 commit into from Jan 27, 2017

Conversation

Projects
None yet
6 participants
@binary1248
Member

binary1248 commented Nov 26, 2016

Fixes #1168.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Nov 26, 2016

Member

Could you elaborate? I can't find how your new code differs from the previous one, except on big-endian systems but I doubt this is the case in #1168.

Member

LaurentGomila commented Nov 26, 2016

Could you elaborate? I can't find how your new code differs from the previous one, except on big-endian systems but I doubt this is the case in #1168.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Nov 26, 2016

Member

sizeof(long) is 8 on LP64 systems (this includes Unix). Xlib was reading past the end of the allocated buffer because even though it only reads the low-order 32-bits out of each element, it still expects elements that are sizeof(long) large. Don't ask me why... it's Xlib.

Member

binary1248 commented Nov 26, 2016

sizeof(long) is 8 on LP64 systems (this includes Unix). Xlib was reading past the end of the allocated buffer because even though it only reads the low-order 32-bits out of each element, it still expects elements that are sizeof(long) large. Don't ask me why... it's Xlib.

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

tried bugfix/xlib_set_icon, crashes on XCreateImage (btw its call not affected by commit). building in steam runtime --i386 (https://github.com/ValveSoftware/steam-runtime).

#include <SFML/Graphics.hpp>

int main() {
	sf::Image *res = new sf::Image();
	res->loadFromFile("icon.png");

	sf::Window *win = new sf::Window(sf::VideoMode(800, 600), "test");
	win->setIcon(res->getSize().x, res->getSize().y, res->getPixelsPtr());

	delete res;
	delete win;
}

achpile commented Dec 1, 2016

tried bugfix/xlib_set_icon, crashes on XCreateImage (btw its call not affected by commit). building in steam runtime --i386 (https://github.com/ValveSoftware/steam-runtime).

#include <SFML/Graphics.hpp>

int main() {
	sf::Image *res = new sf::Image();
	res->loadFromFile("icon.png");

	sf::Window *win = new sf::Window(sf::VideoMode(800, 600), "test");
	win->setIcon(res->getSize().x, res->getSize().y, res->getPixelsPtr());

	delete res;
	delete win;
}
@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

Wow, just tried one thing and it not crashes anymore.

    Uint8* iconPixels = static_cast<Uint8*>(std::malloc(width * height * 4));
    for (std::size_t i = 0; i < width * height; ++i)
    {
        iconPixels[8 + i * 4 + 0] = pixels[i * 4 + 2];
        iconPixels[8 + i * 4 + 1] = pixels[i * 4 + 1];
        iconPixels[8 + i * 4 + 2] = pixels[i * 4 + 0];
        iconPixels[8 + i * 4 + 3] = pixels[i * 4 + 3];
    }

here we have width * height * 4 array, but [8 + i * 4 + 0]. So

Uint8* iconPixels = static_cast<Uint8*>(std::malloc(width * height * 4 + 8));

fixed crash

achpile commented Dec 1, 2016

Wow, just tried one thing and it not crashes anymore.

    Uint8* iconPixels = static_cast<Uint8*>(std::malloc(width * height * 4));
    for (std::size_t i = 0; i < width * height; ++i)
    {
        iconPixels[8 + i * 4 + 0] = pixels[i * 4 + 2];
        iconPixels[8 + i * 4 + 1] = pixels[i * 4 + 1];
        iconPixels[8 + i * 4 + 2] = pixels[i * 4 + 0];
        iconPixels[8 + i * 4 + 3] = pixels[i * 4 + 3];
    }

here we have width * height * 4 array, but [8 + i * 4 + 0]. So

Uint8* iconPixels = static_cast<Uint8*>(std::malloc(width * height * 4 + 8));

fixed crash

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 1, 2016

Member

Consider me confused.

Ah, missed the dimensions at the beginning.

Should this really use unsigned long rather than an explicit length like uint32? Might that be the culprit here?

Member

MarioLiebisch commented Dec 1, 2016

Consider me confused.

Ah, missed the dimensions at the beginning.

Should this really use unsigned long rather than an explicit length like uint32? Might that be the culprit here?

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

btw this works too. so there's simple going out of array boundaries

    Uint8* iconPixels = static_cast<Uint8*>(std::malloc(width * height * 4));
    for (std::size_t i = 0; i < width * height; ++i)
    {
        iconPixels[i * 4 + 0] = pixels[i * 4 + 2];
        iconPixels[i * 4 + 1] = pixels[i * 4 + 1];
        iconPixels[i * 4 + 2] = pixels[i * 4 + 0];
        iconPixels[i * 4 + 3] = pixels[i * 4 + 3];
    }

achpile commented Dec 1, 2016

btw this works too. so there's simple going out of array boundaries

    Uint8* iconPixels = static_cast<Uint8*>(std::malloc(width * height * 4));
    for (std::size_t i = 0; i < width * height; ++i)
    {
        iconPixels[i * 4 + 0] = pixels[i * 4 + 2];
        iconPixels[i * 4 + 1] = pixels[i * 4 + 1];
        iconPixels[i * 4 + 2] = pixels[i * 4 + 0];
        iconPixels[i * 4 + 3] = pixels[i * 4 + 3];
    }
@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

I mean that the segfault can be caused by memory corruption after going out of boundaries and two issues caused by one problem

achpile commented Dec 1, 2016

I mean that the segfault can be caused by memory corruption after going out of boundaries and two issues caused by one problem

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 1, 2016

Member

Could you try this PR but replace unsigned long with sf::Uint32?

Member

MarioLiebisch commented Dec 1, 2016

Could you try this PR but replace unsigned long with sf::Uint32?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 1, 2016

Member

btw this works too. so there's simple going out of array boundaries

Question is: why is there this 8 padding in the first place? Some X-related format?

Member

mantognini commented Dec 1, 2016

btw this works too. so there's simple going out of array boundaries

Question is: why is there this 8 padding in the first place? Some X-related format?

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

@MarioLiebisch in my case app crashed before any changes made in this commit. I got crash at line 789, but this commit starts at line 838. And there's obvious going out of boundaries.

the array is widthheight4, but last element in the loop accessed is
(8 + (width * height - 1) * 4) = 4 * (width * height - 1 + 2) = 4 * (width * height + 1), so next 8 bytes after the end of the array are overwritten by this.

achpile commented Dec 1, 2016

@MarioLiebisch in my case app crashed before any changes made in this commit. I got crash at line 789, but this commit starts at line 838. And there's obvious going out of boundaries.

the array is widthheight4, but last element in the loop accessed is
(8 + (width * height - 1) * 4) = 4 * (width * height - 1 + 2) = 4 * (width * height + 1), so next 8 bytes after the end of the array are overwritten by this.

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

@mantognini I think it's copy&paste from line 841

achpile commented Dec 1, 2016

@mantognini I think it's copy&paste from line 841

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 1, 2016

Member

@mantognini The offset skips the bytes used to store the image dimensions., 2 times 4 bytes.

Member

MarioLiebisch commented Dec 1, 2016

@mantognini The offset skips the bytes used to store the image dimensions., 2 times 4 bytes.

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

@MarioLiebisch the offset is needed only here:

// ICCCM also wants the first 2 unsigned 32-bit values to be width and height

but not at the converting to BGRA.

achpile commented Dec 1, 2016

@MarioLiebisch the offset is needed only here:

// ICCCM also wants the first 2 unsigned 32-bit values to be width and height

but not at the converting to BGRA.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 1, 2016

Member

The offset is no longer there that way in the new commit.

Have to try that myself I guess. How do I set up the Steam Runtime?

Member

MarioLiebisch commented Dec 1, 2016

The offset is no longer there that way in the new commit.

Have to try that myself I guess. How do I set up the Steam Runtime?

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile commented Dec 1, 2016

@MarioLiebisch you're looking wrong way :) offset used 2 times. This commit removed second one.

https://github.com/SFML/SFML/blob/bugfix/xlib_set_icon/src/SFML/Window/Unix/WindowImplX11.cpp#L777

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 1, 2016

I'm sorry guys for lots of messages. So here's the last (i hope :) )

In the commit ca03b64 iconPixels was expanded to (width * height * 4 + 8) and assignment in the loop was offset by 8.

After in commit 9996b7a iconPixels' size was decreased back to (width * height * 4), but offset was left. So the bug is here.

@binary1248 thank you very much for returning to Xlib again. Yeah, that patch is huge, so it was easy to miss something

achpile commented Dec 1, 2016

I'm sorry guys for lots of messages. So here's the last (i hope :) )

In the commit ca03b64 iconPixels was expanded to (width * height * 4 + 8) and assignment in the loop was offset by 8.

After in commit 9996b7a iconPixels' size was decreased back to (width * height * 4), but offset was left. So the bug is here.

@binary1248 thank you very much for returning to Xlib again. Yeah, that patch is huge, so it was easy to miss something

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Dec 3, 2016

Member

Fixed.

Member

binary1248 commented Dec 3, 2016

Fixed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 5, 2016

Member

@achpile Can you give this a try again?

Member

eXpl0it3r commented Dec 5, 2016

@achpile Can you give this a try again?

@achpile

This comment has been minimized.

Show comment
Hide comment
@achpile

achpile Dec 5, 2016

@eXpl0it3r i think there's no need to do it :) because I made same changes for steam release and it looks like it works fine :)

achpile commented Dec 5, 2016

@eXpl0it3r i think there's no need to do it :) because I made same changes for steam release and it looks like it works fine :)

@eXpl0it3r

Tested on my Ubuntu VM works fine.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 9, 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 Dec 9, 2016

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

Fixed Xlib crashing in sf::Window:setIcon because it expects the elem…
…ent data type passed to XChangeProperty to be unsigned long (architecture dependent 32-bit or 64-bit) instead of sf::Uint32 (architecture independent 32-bit) (#1168). Also adjusted other occurrences of wrong types passed to XChangeProperty with format set to 32.

@eXpl0it3r eXpl0it3r merged commit 7fe96d1 into 2.4.x Jan 27, 2017

15 checks passed

android-armeabi-v7a-api13 Build #74 done.
Details
debian-gcc-64 Build #347 done.
Details
freebsd-gcc-64 Build #309 done.
Details
osx-clang-el-capitan Build #194 done.
Details
static-analysis Build #316 done.
Details
windows-gcc-492-tdm-32 Build #207 done.
Details
windows-gcc-492-tdm-64 Build #204 done.
Details
windows-gcc-610-mingw-32 Build #139 done.
Details
windows-gcc-610-mingw-64 Build #141 done.
Details
windows-vc11-32 Build #319 done.
Details
windows-vc11-64 Build #317 done.
Details
windows-vc12-32 Build #316 done.
Details
windows-vc12-64 Build #316 done.
Details
windows-vc14-32 Build #319 done.
Details
windows-vc14-64 Build #320 done.
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/xlib_set_icon branch Jan 27, 2017

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