BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

SDL Development
1

Errr… Shouldn’t that be:

Uint32 * buffer = (Uint32 *) screen->pixels;
buffer[0] = 0xFF;

Yes, but it doesn’t really matter since a pointer fits into the UINT32 (in
VC++). That was just an oversight I made while throwing together the test
program - which didn’t effect the results, I did things correctly in the
program where I initially detected the problem (which cast things to a
Uint16*).

Also, check that the value returned by SetVideoMode is not NULL.

The problem is that SetVideoMode never returns! It stomps memory within the
function.

I did a lot of debugging this afternoon and figured out exactly where SDL is
stumbling, check it out: The failing function within the SetVideoMode call
is (drumroll)… SDL_DrawCursorNoLock(). I discovered that the ‘random’
factor affecting the crashes had to do with how far down the screen I had my
cursor!

I marred the beautiful sdlcursor.c with some of my half-assed debugging code
(since I couldn’t step through these functions in fullscreen mode) and I
found that the memcpy in SDL_DrawCursorNoLock() was causing the allocation
problem. Here are what the variable there (right before the memcpy) look
like on a successful SetVideoMode():

w = 32, h = 16, screenbpp = 2, pitch = 1600

and here’s something typical of what I was seeing during the calls that were
exploding:

w = 130638, h = 16, screenbpp = 2, pitch = 1600

I think we can all see which variable is suspicious here.

So, I figure that SDL_MouseRect() is where the true damage is done. I put
some checks in there and here’s what I found:

successful: SDL_cursor->area.x = 593, .y =380, .w = 16, .h =16,

surface->w = 800, surface->h = 600

b4 failure:  SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,

surface->w = 800, surface->h = 600

Now, can someone look at the logic of SDL_MouseRect() and confirm exactly
what the problem is? It looks as if the logic assumes your cursor will
never be out of bounds of the new fullscreen resolution (not good), eh?
I’ve been staring at this way too long to decide anything tonight or how to
fix it - I’d rather defer that honor to someone more intimate with the code
(since I’ve only been using SDL since last friday ).

Thanks in advance,
Jesse

2

In my experience when an integer becomes some enormous number for no good
reason one of two things has happened: 1) an address is copied instead of a
value, or 2) a small integer is subtracted from an even smaller unsigned
integer and the resulting value is a very large unsigned integer. The second
one is happening.

First look at the SDL_Rect struct definition:

typedef struct {
Sint16 x, y;
Uint16 w, h;
} SDL_Rect;

w and h are unsigned. Now look at the SDL_MouseRect function definition:

void SDL_MouseRect(SDL_Rect *area)
{
int clip_diff;

*area = SDL_cursor->area;
if ( area->x < 0 ) {
    area->w += area->x;
    area->x = 0;
}
if ( area->y < 0 ) {
    area->h += area->y;
    area->y = 0;
}
clip_diff = (area->x+area->w)-SDL_VideoSurface->w;
if ( clip_diff > 0 ) {
    area->w -= clip_diff;
}
clip_diff = (area->y+area->h)-SDL_VideoSurface->h;
if ( clip_diff > 0 ) {
    area->h -= clip_diff;
}

}

The code makes a lot of assumptions on the sign and range of the values,
none of which are commented… Here’s a trace on the function using the
values you gave when it crashes:

SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,
SDL_VideoSurface->w = 800, surface->h = 600

if (area->x < 0) // false
if (area->y < 0) // false

clip_diff = (area->x+area->w)-SDL_VideoSurface->w;
clip_diff = (1017+16)-800;
clip_diff = 1033-800;
clip_diff = 233;

if (clip_diff > 0) // true
area->w -= clip_diff;
area->w = area->w-clip_diff;
area->w = 16-233;
area->w = -217; // fubar

-217 is cast to Uint16. Bad. You could fix the problem by replacing the
offending lines (the area->h line is problematic as well) with:

area->w = area->w < clip_diff ? 0 : area->w-clip_diff;
area->h = area->h < clip_diff ? 0 : area->w-clip_diff;

Then SDL_DrawCursorNoLock will execute the early return statement and the
program continues as expected. So yes, the function assumes the cursor will
always be in the window and that leads to the crashing.________________________
Ajay Ayyagari
@Ajay_Ayyagari

From: “Evil”
Reply-To: sdl at lokigames.com
Date: Tue, 17 Jul 2001 22:53:00 -0500
To:
Subject: [SDL] BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

Errr… Shouldn’t that be:

Uint32 * buffer = (Uint32 *) screen->pixels;
buffer[0] = 0xFF;

Yes, but it doesn’t really matter since a pointer fits into the UINT32 (in
VC++). That was just an oversight I made while throwing together the test
program - which didn’t effect the results, I did things correctly in the
program where I initially detected the problem (which cast things to a
Uint16*).

Also, check that the value returned by SetVideoMode is not NULL.

The problem is that SetVideoMode never returns! It stomps memory within the
function.

I did a lot of debugging this afternoon and figured out exactly where SDL is
stumbling, check it out: The failing function within the SetVideoMode call
is (drumroll)… SDL_DrawCursorNoLock(). I discovered that the 'random’
factor affecting the crashes had to do with how far down the screen I had my
cursor!

I marred the beautiful sdlcursor.c with some of my half-assed debugging code
(since I couldn’t step through these functions in fullscreen mode) and I
found that the memcpy in SDL_DrawCursorNoLock() was causing the allocation
problem. Here are what the variable there (right before the memcpy) look
like on a successful SetVideoMode():

w = 32, h = 16, screenbpp = 2, pitch = 1600

and here’s something typical of what I was seeing during the calls that were
exploding:

w = 130638, h = 16, screenbpp = 2, pitch = 1600

I think we can all see which variable is suspicious here.

So, I figure that SDL_MouseRect() is where the true damage is done. I put
some checks in there and here’s what I found:

successful: SDL_cursor->area.x = 593, .y =380, .w = 16, .h =16,
surface->w = 800, surface->h = 600

b4 failure: SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,
surface->w = 800, surface->h = 600

Now, can someone look at the logic of SDL_MouseRect() and confirm exactly
what the problem is? It looks as if the logic assumes your cursor will
never be out of bounds of the new fullscreen resolution (not good), eh?
I’ve been staring at this way too long to decide anything tonight or how to
fix it - I’d rather defer that honor to someone more intimate with the code
(since I’ve only been using SDL since last friday ).

Thanks in advance,
Jesse
www.eternaldusk.com

3

Thanks Ajay,

I had thought about the possibility of a UInt wrapping down, but I had been
staring at it so long that by the time I checked SDL_Rect, I only looked at
the type of x and y - completely missing the fact that w and h were a
different type.

Anyway, thanks a million for looking at that for me - I’m going to make the
change you suggested. How do we make sure this fix gets put in the main SDL
distribution? I’m new here, so I’m not sure what the process is at this
point.

Thanks again!
-Jesse> ----- Original Message -----

From: random0@u.washington.edu (Ajay Ayyagari)
To:
Sent: Wednesday, July 18, 2001 2:50 AM
Subject: Re: [SDL] BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

In my experience when an integer becomes some enormous number for no good
reason one of two things has happened: 1) an address is copied instead of
a
value, or 2) a small integer is subtracted from an even smaller unsigned
integer and the resulting value is a very large unsigned integer. The
second
one is happening.

First look at the SDL_Rect struct definition:

typedef struct {
Sint16 x, y;
Uint16 w, h;
} SDL_Rect;

w and h are unsigned. Now look at the SDL_MouseRect function definition:

void SDL_MouseRect(SDL_Rect *area)
{
int clip_diff;

*area = SDL_cursor->area;
if ( area->x < 0 ) {
    area->w += area->x;
    area->x = 0;
}
if ( area->y < 0 ) {
    area->h += area->y;
    area->y = 0;
}
clip_diff = (area->x+area->w)-SDL_VideoSurface->w;
if ( clip_diff > 0 ) {
    area->w -= clip_diff;
}
clip_diff = (area->y+area->h)-SDL_VideoSurface->h;
if ( clip_diff > 0 ) {
    area->h -= clip_diff;
}

}

The code makes a lot of assumptions on the sign and range of the values,
none of which are commented… Here’s a trace on the function using the
values you gave when it crashes:

SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,
SDL_VideoSurface->w = 800, surface->h = 600

if (area->x < 0) // false
if (area->y < 0) // false

clip_diff = (area->x+area->w)-SDL_VideoSurface->w;
clip_diff = (1017+16)-800;
clip_diff = 1033-800;
clip_diff = 233;

if (clip_diff > 0) // true
area->w -= clip_diff;
area->w = area->w-clip_diff;
area->w = 16-233;
area->w = -217; // fubar

-217 is cast to Uint16. Bad. You could fix the problem by replacing the
offending lines (the area->h line is problematic as well) with:

area->w = area->w < clip_diff ? 0 : area->w-clip_diff;
area->h = area->h < clip_diff ? 0 : area->w-clip_diff;

Then SDL_DrawCursorNoLock will execute the early return statement and the
program continues as expected. So yes, the function assumes the cursor
will
always be in the window and that leads to the crashing.

Ajay Ayyagari
random0 at u.washington.edu

From: “Evil” <@Evil>
Reply-To: sdl at lokigames.com
Date: Tue, 17 Jul 2001 22:53:00 -0500
To:
Subject: [SDL] BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

Errr… Shouldn’t that be:

Uint32 * buffer = (Uint32 *) screen->pixels;
buffer[0] = 0xFF;

Yes, but it doesn’t really matter since a pointer fits into the UINT32
(in

VC++). That was just an oversight I made while throwing together the
test

program - which didn’t effect the results, I did things correctly in the
program where I initially detected the problem (which cast things to a
Uint16*).

Also, check that the value returned by SetVideoMode is not NULL.

The problem is that SetVideoMode never returns! It stomps memory within
the

function.

I did a lot of debugging this afternoon and figured out exactly where
SDL is

stumbling, check it out: The failing function within the SetVideoMode
call

is (drumroll)… SDL_DrawCursorNoLock(). I discovered that the
’random’

factor affecting the crashes had to do with how far down the screen I
had my

cursor!

I marred the beautiful sdlcursor.c with some of my half-assed debugging
code

(since I couldn’t step through these functions in fullscreen mode) and I
found that the memcpy in SDL_DrawCursorNoLock() was causing the
allocation

problem. Here are what the variable there (right before the memcpy)
look

like on a successful SetVideoMode():

w = 32, h = 16, screenbpp = 2, pitch = 1600

and here’s something typical of what I was seeing during the calls that
were

exploding:

w = 130638, h = 16, screenbpp = 2, pitch = 1600

I think we can all see which variable is suspicious here.

So, I figure that SDL_MouseRect() is where the true damage is done. I
put

some checks in there and here’s what I found:

successful: SDL_cursor->area.x = 593, .y =380, .w = 16, .h =16,
surface->w = 800, surface->h = 600

b4 failure: SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,
surface->w = 800, surface->h = 600

Now, can someone look at the logic of SDL_MouseRect() and confirm
exactly

what the problem is? It looks as if the logic assumes your cursor will
never be out of bounds of the new fullscreen resolution (not good), eh?
I’ve been staring at this way too long to decide anything tonight or how
to

fix it - I’d rather defer that honor to someone more intimate with the
code

(since I’ve only been using SDL since last friday ).

Thanks in advance,
Jesse
www.eternaldusk.com

4

One more thing I wanted to point out - for whomever applies this fix: That
second line should actually read:

area->h = area->h < clip_diff ? 0 : area->h-clip_diff;

instead of “area->h = area->h < clip_diff ? 0 : area->w-clip_diff;”. I made
these changes on my copy of SDL and it seems to have fixed the crashes
completely! Thanks again for looking at that with me Ajay!

-Jesse> ----- Original Message -----

From: @Evil (Evil)
To:
Sent: Wednesday, July 18, 2001 7:50 AM
Subject: Re: [SDL] BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

Thanks Ajay,

I had thought about the possibility of a UInt wrapping down, but I had
been
staring at it so long that by the time I checked SDL_Rect, I only looked
at
the type of x and y - completely missing the fact that w and h were a
different type.

Anyway, thanks a million for looking at that for me - I’m going to make
the
change you suggested. How do we make sure this fix gets put in the main
SDL
distribution? I’m new here, so I’m not sure what the process is at this
point.

Thanks again!
-Jesse

----- Original Message -----
From: “Ajay Ayyagari”
To:
Sent: Wednesday, July 18, 2001 2:50 AM
Subject: Re: [SDL] BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

In my experience when an integer becomes some enormous number for no
good

reason one of two things has happened: 1) an address is copied instead
of
a

value, or 2) a small integer is subtracted from an even smaller unsigned
integer and the resulting value is a very large unsigned integer. The
second
one is happening.

First look at the SDL_Rect struct definition:

typedef struct {
Sint16 x, y;
Uint16 w, h;
} SDL_Rect;

w and h are unsigned. Now look at the SDL_MouseRect function definition:

void SDL_MouseRect(SDL_Rect *area)
{
int clip_diff;

*area = SDL_cursor->area;
if ( area->x < 0 ) {
    area->w += area->x;
    area->x = 0;
}
if ( area->y < 0 ) {
    area->h += area->y;
    area->y = 0;
}
clip_diff = (area->x+area->w)-SDL_VideoSurface->w;
if ( clip_diff > 0 ) {
    area->w -= clip_diff;
}
clip_diff = (area->y+area->h)-SDL_VideoSurface->h;
if ( clip_diff > 0 ) {
    area->h -= clip_diff;
}

}

The code makes a lot of assumptions on the sign and range of the values,
none of which are commented… Here’s a trace on the function using the
values you gave when it crashes:

SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,
SDL_VideoSurface->w = 800, surface->h = 600

if (area->x < 0) // false
if (area->y < 0) // false

clip_diff = (area->x+area->w)-SDL_VideoSurface->w;
clip_diff = (1017+16)-800;
clip_diff = 1033-800;
clip_diff = 233;

if (clip_diff > 0) // true
area->w -= clip_diff;
area->w = area->w-clip_diff;
area->w = 16-233;
area->w = -217; // fubar

-217 is cast to Uint16. Bad. You could fix the problem by replacing the
offending lines (the area->h line is problematic as well) with:

area->w = area->w < clip_diff ? 0 : area->w-clip_diff;
area->h = area->h < clip_diff ? 0 : area->w-clip_diff;

Then SDL_DrawCursorNoLock will execute the early return statement and
the

program continues as expected. So yes, the function assumes the cursor
will
always be in the window and that leads to the crashing.

Ajay Ayyagari
random0 at u.washington.edu

From: “Evil” <@Evil>
Reply-To: sdl at lokigames.com
Date: Tue, 17 Jul 2001 22:53:00 -0500
To:
Subject: [SDL] BUG: SetVideoMode Resize Crash (due to SDL_MouseRect)

Errr… Shouldn’t that be:

Uint32 * buffer = (Uint32 *) screen->pixels;
buffer[0] = 0xFF;

Yes, but it doesn’t really matter since a pointer fits into the UINT32
(in

VC++). That was just an oversight I made while throwing together the
test

program - which didn’t effect the results, I did things correctly in
the

program where I initially detected the problem (which cast things to a
Uint16*).

Also, check that the value returned by SetVideoMode is not NULL.

The problem is that SetVideoMode never returns! It stomps memory
within
the

function.

I did a lot of debugging this afternoon and figured out exactly where
SDL is

stumbling, check it out: The failing function within the SetVideoMode
call

is (drumroll)… SDL_DrawCursorNoLock(). I discovered that the
’random’

factor affecting the crashes had to do with how far down the screen I
had my

cursor!

I marred the beautiful sdlcursor.c with some of my half-assed
debugging
code

(since I couldn’t step through these functions in fullscreen mode) and
I

found that the memcpy in SDL_DrawCursorNoLock() was causing the
allocation

problem. Here are what the variable there (right before the memcpy)
look

like on a successful SetVideoMode():

w = 32, h = 16, screenbpp = 2, pitch = 1600

and here’s something typical of what I was seeing during the calls
that
were

exploding:

w = 130638, h = 16, screenbpp = 2, pitch = 1600

I think we can all see which variable is suspicious here.

So, I figure that SDL_MouseRect() is where the true damage is done. I
put

some checks in there and here’s what I found:

successful: SDL_cursor->area.x = 593, .y =380, .w = 16, .h =16,
surface->w = 800, surface->h = 600

b4 failure: SDL_cursor->area.x = 1017, .y =201, .w = 16, .h =16,
surface->w = 800, surface->h = 600

Now, can someone look at the logic of SDL_MouseRect() and confirm
exactly

what the problem is? It looks as if the logic assumes your cursor
will

never be out of bounds of the new fullscreen resolution (not good),
eh?

I’ve been staring at this way too long to decide anything tonight or
how
to

fix it - I’d rather defer that honor to someone more intimate with the
code

(since I’ve only been using SDL since last friday ).

Thanks in advance,
Jesse
www.eternaldusk.com

5

One more thing I wanted to point out - for whomever applies this fix: That
second line should actually read:

area->h = area->h < clip_diff ? 0 : area->h-clip_diff;

instead of “area->h = area->h < clip_diff ? 0 : area->w-clip_diff;”. I made
these changes on my copy of SDL and it seems to have fixed the crashes
completely! Thanks again for looking at that with me Ajay!

Thanks guys! I’ve incorporated your fix into SDL CVS.

See ya!
-Sam Lantinga, Lead Programmer, Loki Software, Inc.