SDL-1.2.5 divide-by-zero patch for SDL_WarpMouse (Uint16 x, Uint16 y) [src/video/SDL_cursor.c]

Hi,

I’m porting a game from GNU/Linux to PS2Linux v1.0. I found this
divide-by-zero bug, and have attatched a patch for it. I do not know
the SDL procedures for applying patches to the CVS, so could someone
review/add my patch please?

In SDL_Cursor.c, consistently 2 pointers are created/set to the same
struct in each function:
SDL_VideoDevice *video = current_video;
SDL_VideoDevice *this = current_video;

I can only assume this is a design decision, could somone tell me why 2
pointers are necessary in each function?

Program received signal SIGTRAP, Trace/breakpoint trap.
0x2aacffd8 in SDL_WarpMouse (x=320, y=240) at SDL_cursor.c:302
302 x += (this->screen->offset % this->screen->pitch) /
Current language: auto; currently c
(gdb)
(gdb) next
error
$

Add this if you wish to replicate this bug and see the times when it
would have caused an exception.

else {
printf(“x: %d y: %d offset: %d pitch: %d\n”, x, y,
this->screen->offset,this->screen->pitch);
}

Regards

JG

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed…
Name: jg-SDL_cursor1.patch
URL: http://lists.libsdl.org/pipermail/sdl-libsdl.org/attachments/20030206/9832038f/attachment.txt

else {
printf(“x: %d y: %d offset: %d pitch: %d\n”, x, y,
this->screen->offset,this->screen->pitch);
}

It doesn’t make much sense for the pitch of a surface to be 0; that
implies it has a zero size (null surface). Are you sure the problem isn’t
something else?

  • x += (this->screen->offset % this->screen->pitch) /
    
  •       this->screen->format->BytesPerPixel;
    
  • y += (this->screen->offset / this->screen->pitch);
    
  • if((this->screen->offset > 0) && (this->screen->pitch > 0)) {
    

Offset being 0 shouldn’t cause any problems here, so it’s better not
to check for it (less code).On Thu, Feb 06, 2003 at 10:06:20PM +0000, J. Grant wrote:

  • 	x += (this->screen->offset % this->screen->pitch) /
    
  • 	      this->screen->format->BytesPerPixel;
    
  • 	y += (this->screen->offset / this->screen->pitch);
    


Glenn Maynard

Hi,

It doesn’t make much sense for the pitch of a surface to be 0; that
implies it has a zero size (null surface). Are you sure the problem isn’t
something else?

I tracked down the problem in the source code, it was calling SDL_Warp()
before SDL_SetVideoMode(), so the pitch was not known.

It is strange that this bug was not visible on my x86 Linux machine.

  • x += (this->screen->offset % this->screen->pitch) /
    
  •       this->screen->format->BytesPerPixel;
    
  • y += (this->screen->offset / this->screen->pitch);
    
  • if((this->screen->offset > 0) && (this->screen->pitch > 0)) {
    

Offset being 0 shouldn’t cause any problems here, so it’s better not
to check for it (less code).

Yes, I agree only pitch should be checked. I think it is essential to
check possible divide-by-zero opportunities. I have attached an updated
patch for review.

I have not seen error msg output from SDL, would something as follows be
appropriate to add at the end of that if()? I have not seen assert() in
the source code so far, is this used in SDL?

else {
sprintf(stderr, "divide by zero error pitch: %d\n"this->screen->pitch);
}

Regards

JG

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed…
Name: SDL_cursor-2.patch
URL: http://lists.libsdl.org/pipermail/sdl-libsdl.org/attachments/20030206/fc88db3c/attachment.asc

Yes, I agree only pitch should be checked. I think it is essential to
check possible divide-by-zero opportunities. I have attached an updated
patch for review.

If it’s guaranteed that pitch will never, under normal (eg. non-buggy)
circumstances, be zero, then the check is just extra code taking up
space.

I have not seen error msg output from SDL, would something as follows be
appropriate to add at the end of that if()? I have not seen assert() in
the source code so far, is this used in SDL?

If the pitch is always supposed to be non-zero, then the most it should
do is an assert:

assert(foo->pitch != 0);

I don’t know the circumstances that are leading to a zero pitch, and
which bit of code is at fault. (It doesn’t make much sense for a surface
to have a zero pitch–even a 0x0 surface can have a pitch of 1–so adding
a check for this every time the pitch is used as a divisor seems very ugly.)On Fri, Feb 07, 2003 at 04:41:39AM +0000, J. Grant wrote:


Glenn Maynard

Hi,

It doesn’t make much sense for the pitch of a surface to be 0; that
implies it has a zero size (null surface). Are you sure the problem isn’t
something else?

I tracked down the problem in the source code, it was calling SDL_Warp()
before SDL_SetVideoMode(), so the pitch was not known.

Yeah, this is a no-no. I’ve added code to CVS to prevent this.

Thanks!
-Sam Lantinga, Software Engineer, Blizzard Entertainment

Hi Glen,

Thanks for the reply.

If it’s guaranteed that pitch will never, under normal (eg. non-buggy)
circumstances, be zero, then the check is just extra code taking up
space.

perhaps a check is not necessary then.

I have not seen error msg output from SDL, would something as follows be
appropriate to add at the end of that if()? I have not seen assert() in
the source code so far, is this used in SDL?

If the pitch is always supposed to be non-zero, then the most it should
do is an assert:

assert(foo->pitch != 0);

Something like this would be useful, then at least if NODEBUG was
defined it would be skipped.

I don’t know the circumstances that are leading to a zero pitch, and
which bit of code is at fault. (It doesn’t make much sense for a surface
to have a zero pitch–even a 0x0 surface can have a pitch of 1–so adding
a check for this every time the pitch is used as a divisor seems very ugly.)

yes, I think so too considering it again. The SDL_Surface was created,
but not initalised, that was why pitch was 0 in this game I am porting.
I fixed the bug now, but an assert would have made identification of
the problem simpler.

Regards

JG