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);
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);
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