Crash couse SDL_GetRGB()

SDL Development
1

Hi there, fellows!
Iv try to check bitmap for pixels color using this code:

SDL_Color WrongPixelsFinder::get_pixelColor(SDL_Surface *surface, int x, int y)
{
SDL_Color theKey = { -1, -1, -1, -1 };
if (x < 0 || y< 0 || y > surface->h || x > surface->w)
{
return theKey;
}
Uint32 pixels = (Uint32)surface->pixels;
SDL_GetRGB(pixels[(y * surface->w) + x], surface->format, &theKey.r, &theKey.g, &theKey.b);
return theKey;
}

Code where i used this method:

Code

comprColors is just function where i compere, like if(rgb.b == rgb1.b) return true

After some time, loop got crash and that kind of error shows up:
“Unhandled exception at 0x01056F0E in PixelFinder.exe: 0xC0000005: Access violation reading location 0x7EFD3000.”

Anyone know why?

2

If you have an image that is 640x480, and a y of 480, you get this: pixels[(480 * 640) + x]. No matter what x is, you will be past the memory limit allocated for the surface. If you ensure that y is never equal to the height of the surface, it should work. y >= surface->h

if (comprColors(get_pixelColor(surface, x, y), get_pixelColor(surface, x, (y + 1))) == false && comprColors(get_pixelColor(surface, x, y), get_pixelColor(surface, x, (y - 1))) == false && comprColors(get_pixelColor(surface, x, y), get_pixelColor(surface, (x + 1), y)) == false && comprColors(get_pixelColor(surface, x, y), get_pixelColor(surface, (x - 1), y)) == false)

In that if statement, you’re adding 1 to y and later x. Your for loops are under the bounds of the arrays, but when the if statement is hit, it’s going out of bounds and causing your segfault.

1 Like
3

Okay, so iv added those changes to my code. “y >= surface->h” to if at get_pixelColor, “y<= surface->h” at loop, but it still doesn’t work.

Program checked image 2048x2048, reached 1561 for y, and 1022 for x. After this, its got crash, always in the same time.

Its so weird, don’t get it why.

4

I just looked at the second part of your Paste ofCode again and noticed this:

if (maxW_PX < 1000)
  {
    w_pixel[maxW_PX - 1].x = x;
    w_pixel[maxW_PX - 1].y = y;
    DAMChanger(&w_pixel, maxW_PX, 1, 1);
    maxW_PX += 1;
  }

I’m not sure where maxW_PX comes into play, but if it’s ever 0, then the program might be crashing there. 0 - 1 will result in a negative array position, so it’s possible that you’re trying to access memory that is outside of the bounds of your array.
Are negative array indexes allowed in C? Second answer.

If that doesn’t help, then how do you know when the program crashes? Are you using a debugger and watching the variables to know where it’s stopping or are you printf debugging some more? //cout << "Znaleziono" << endl; I saw that in your code, and if you’re printf debugging, remember to flush stdout after each time you print to it. I personally prefer that to normal debuggers; but I’m a glutton for punishment.

1 Like
5

No, no. maxW_PX started with default value 1.

I Just start in debug, not realese. When its crashed, got message and place where it crashed.
Visual points at this SDL_GetRGB(pixels[(y * surface->w) + x], surface->format, &theKey.r, &theKey.g, &theKey.b);

I dont know, maybe its DAMChanger fault…Iv made it, and im not a profesional.
https://paste.ofcode.org/wPqWfcnvjXW3mHcBj76wF7

6

Iv made it, and im not a profesional.
Neither am I. In fact, I failed the course I took in college for C and never took any C++ courses at all; doesn’t mean you can’t learn or use them.

Your DAMChanger function and how you’re handling the memory is a bit inefficient as you’re constantly reallocating memory and the heap could be getting cluttered because of it. It’s also doing the same thing for mode no matter what it is. Issues for another time though.

Now for the current issue, the 1561 and 1022 that you see when it crashes, where and when do you get those numbers? Are you reading them from your debugger or somewhere else? Your debugger says that it’s crashing at that SDL_GetRGB, so it’s trying to access that surface and running out of bounds for some reason.

Give this a try: download Dr. Memory and run your program using it to see where your memory issue is. It’s like Valgrind for Windows.

1 Like
7

Okay, so i’ve checked my program, and most interesting things i’ve found are two types of errors:

UNADDRESSABLE ACCESS: reading 0x0000000c-0x00000010 4 byte(s)
WrongPixelsFinder::checkPixels
Note: @0:00:03.068 in thread 3240
Note: instruction: cmp 0x0c(%esi) %ebx

LEAK 8 direct bytes 0x00a21b98-0x00a21ba0 + 0 indirect bytes
replace_operator_new_array

but im not quite sure, what this replace operator mean, maybe something like “new/delete” from dynamics allocation?

8

The UNADDRESSABLE ACCESS errors are why your program is crashing. The LEAK errors are ignorable for now, as they just tell us that you allocated them with new but did not delete them. The replace_operator_new_array just means that Dr. Memory replaced your calls to new with replace_operator_new_array.

checkPixels did you post that function yet? Your Paste of Code has expired and I can’t look at that code anymore. It’s probably crashing in there somewhere.

1 Like
9

Oh, this dr Memory looks very handy now.
Yea the code has expired. I’ve refresh it, here - > checkPixels

So, i guess its a problem with that DAMChanger.
I will replace it with an static memory, and see if its works.
Maybe you know some way to increase or decrease arrays that i can use anywhere, without losing a data?

10

Did Dr. Memory say which line of checkPixels was the cause of the issue? If you’re compiling in debugging mode, then it should say which file and line of the file that the error occurred on. Keep in mind that Dr. Memory can be confusing to use at first as errors may occur inside of functions that you didn’t write but called from different libraries. So where it says your UNADDRESSABLE ACCESS is happening, look for the top most one you created, which should be your checkPixels function. Then post that line.

1 Like
11

Its pointed at this line:
“for (int y = 0;y < surface->h; y++)”
That tell me nothing, everything seems okay to me.

I’ve tried without DAMChanger, but when i’ve created an array[1000] using new, nothing changed.
So i think, its not the case. I start to thought, maybe sdl couse this error…

Here i put all important code lines together - > Code

12

If it’s saying that your for command is the issue, that would imply that the surface is being destroyed somewhere in your code while it’s still being used. Or it could be that you made changes to your code after running Dr. Memory and aren’t looking at the right line anymore. It’s probably best to compile your code again to ensure everything is up to date and run Dr. Memory again.

For DAMChanger, you probably want to use a vector since it’s better suited for what you’re doing. So change SDL_Point* w_pixel = new SDL_Point[1]; into vector<SDL_Point> w_pixel; and then change your code to add to the vector when you have found a pixel. Your found = 1; statement can be replaced with everything inside of if ( maxW_PX < 1000 ) if you don’t use a vector. If you do use a vector then you can just use: w_pixel.push_back({x, y}); C++ website - std::vector

Try this as well, instead of a surface that is 2048x2048, try something smaller like 512x512 or 1024x1024 to see if the function can complete itself successfully.

1 Like
13

Okay, so… iv found out that sizes 181 works, but any greater values couse the crash.
Dr. Memory for 100% points at “for (int y = 0;y < surface->h; y++)”.
I’ve deleted damchanger, and tried without new/delete. Make normal array with size 1000.

There’s two reasons for that crash:
a) sdl got f**** up something.
b) Uint32 type is wrong and i need to use something different, but what then.

Anyway, thank you very much for such a great help. Can’t expect more.
Your advice and program you’ve recommended, will be very useful for me.
May force be with you.

14

Sounds like you’re freeing the surface somewhere then. If you split the for loop like this:

for(
      int y = 0;
      y < surface->h;
      y++
    )

Does it point to y < surface->h;? If it does, you’re either freeing the memory for it while it’s still needed, or you’re creating the surface in a function and returning it incorrectly. How are you creating the surface?

1 Like
15

Hah, thats odd, its just point at “)” end of for.
I use IMG to load

int main(int argc, char *argv[])
{
        AllocConsole();
        SetConsoleTitle("Wrong Pixel Finder");
        freopen("CONIN$", "r", stdin);
        freopen("CONOUT$", "w", stdout);
        freopen("CONOUT$", "w", stderr);
 
        SDL_Init(SDL_INIT_VIDEO | SDL_INIT_EVENTS);
        IMG_Init(IMG_INIT_PNG);
 
        int tol = 0;
 
        WrongPixelsFinder wpf;
 
        SDL_Surface *surface = IMG_Load("picture.png");
       
        wpf.checkPixels(surface, tol);
 
        SDL_Window* window = SDL_CreateWindow("Viewport", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 1280, 720, NULL);;
        SDL_Renderer* renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED);
        SDL_Event ev;
 
        rest of the code...
}
16

After SDL_Surface *surface = IMG_Load("picture.png");, check and see if surface is NULL. It might fail to create the surface and the program might not catch it for a bit, which then causes the crash.

if(!surface)
  {
    printf("\nError creating surface: %s.", SDL_GetError();
    return 1;
  };
1 Like
17

I put this if everywhere, after creating surface, in to x loop, and in to get_pixelColor function. Everything seems fine, no message, but still same crashing. : /

I will try to download sdl, and update everything include dll’s files.

18

This should be minor, but you should change SDL_CreateWindow("Viewport", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 1280, 720, NULL); so that NULL is a valid set of flags. NULL is implementation defined: The null pointer value may be something other than 0.

SDL_Color theKey = { -1, -1, -1, -1 }; That’s a bit off now that I look at it again. Those all should be 0 and not -1 since the values are all unsigned ranging from 0-255. Did updating SDL fix your issue?

1 Like
19

Updating image and sdl library changed nothing.
Iv changed those default values of thekey, same thing.
I don’t think that NULL is so wrong, many people used it in cpp, but yea iv change it to 0, still nothing.

I’ve found that in the pixels[(y * surface->w) + x], when [(y * surface->w) + x] is set at 32765- everything is fine, but when index is setting at 32766+ program got crashed.

I will try to create new project in vs, linking sdl and image, copying code. Maybe then it will run.

20

Are you compiling in 16 bit mode, with a 16 bit compiler, or are you using 16 bit shorts? Those numbers imply that you’re hitting the limit of a signed 16 bit integer and it’s wrapping, which then causes the numbers to be negative/undefined.