Memory bug in SDL CVS, thx valgrind

Hello,

There is a bug in SDL CVS in the SDL_OpenAudio call (SLD_audio.c:464) :
The SDL_AudioSpec struct passed to SLD_OpenAudio is memcompared to an
internal struct. The definition of SDL_AudioSpec says some fields have to
be calculated but they are not prior to the memcmp, so there is a read to
an uninitialised memory. It is bad, first it makes valgrind complaints
second it seems to indicate that this memcmp is broken (as there could be
anything in the non initalized memory).

Can someone fix this ?

Thanks

Steph

Hello,

There is a bug in SDL CVS in the SDL_OpenAudio call (SLD_audio.c:464) :
The SDL_AudioSpec struct passed to SLD_OpenAudio is memcompared to an
internal struct. The definition of SDL_AudioSpec says some fields have to
be calculated but they are not prior to the memcmp, so there is a read to
an uninitialised memory. It is bad, first it makes valgrind complaints
second it seems to indicate that this memcmp is broken (as there could be
anything in the non initalized memory).

Can someone fix this ?

This is actually safe. What Valgrind is complaining about is that the
padding in the data structure is not initialized by the application, and
is then used in the memcmp(). What it doesn’t catch is that the desired
audio format is copied to audio->spec, and then later compared with it.
This is a case of read before initialize but then that same value is
compared against the same uninitialized value.

I think I cleaned this up a bit in SDL_audio.c. Can you update code
from CVS and see if that fixes the warning (and still works?) :slight_smile:

Thanks!
-Sam Lantinga, Software Engineer, Blizzard Entertainment

I think I cleaned this up a bit in SDL_audio.c. Can you update code
from CVS and see if that fixes the warning (and still works?) :slight_smile:

Ok, now it works.

Note that I had to remove the !DEFINE in src/main/Makefile.am:h :
endif !TARGET_QTOPIA to endif
endif !TARGET_MACOSX to endif
endif !TARGET_WIN32 to endif

To be able to compile the CVS otherwise make complaints about underminated
ifdef. I’m using debian sid.

If you are interested by potential buggy behaviour seen through valgrind,
there is the following issues :

==14738== Conditional jump or move depends on uninitialised value(s)
==14738== at 0x4031294B: X11_SetKeyboardState (SDL_x11events.c:882)
==14738== by 0x40311C11: X11_DispatchEvent (SDL_x11events.c:243)
==14738== by 0x40311EAD: X11_PumpEvents (SDL_x11events.c:469)
==14738== by 0x4032A932: SDL_PumpEvents (SDL_events.c:354)
==14738== by 0x4032A978: SDL_PollEvent (SDL_events.c:373)
==14738== by 0x811091C: Screen::execute(DrawableSurface*, int)
(GUIBase.cpp:295)

It seems that some entries of key table are not initialized.
The GUIBase code is trivial :

SDL_Event event;

while (SDL_PollEvent(&event)) (line 295)

There is also some memory related issue on SDL video init but it is probably
a problem on X’s side :
==14738== Syscall param writev(vector[…]) contains uninitialised or
unaddressable byte(s)
==14738== at 0x405B56BE: (within /lib/libc-2.3.2.so)
==14738== by 0x406CE8FF: (within /usr/X11R6/lib/libX11.so.6.2)
==14738== by 0x406CF4EE: _X11TransWritev
(in /usr/X11R6/lib/libX11.so.6.2)
==14738== by 0x406AFEB6: _XSend (in /usr/X11R6/lib/libX11.so.6.2)
==14738== by 0x406AB530: XStoreColors (in /usr/X11R6/lib/libX11.so.6.2)
==14738== by 0x403188E8: X11_SetGammaRamp (SDL_x11video.c:1213)
==14738== Address 0x4211DD5F is 19 bytes inside a block of size 2048
alloc’d
==14738== at 0x4002D685: calloc (vg_replace_malloc.c:201)
==14738== by 0x406A0ECC: XOpenDisplay (in /usr/X11R6/lib/libX11.so.6.2)
==14738== by 0x40316D46: X11_VideoInit (SDL_x11video.c:397)
==14738== by 0x4030BFB0: SDL_VideoInit (SDL_video.c:241)
==14738== by 0x402E9395: SDL_InitSubSystem (SDL.c:74)
==14738== by 0x402E93D6: SDL_Init (SDL.c:166)

Thanks,

Stephane

Note that I had to remove the !DEFINE in src/main/Makefile.am:h :
endif !TARGET_QTOPIA to endif
endif !TARGET_MACOSX to endif
endif !TARGET_WIN32 to endif

Can you check CVS? This should be fixed too.

I looked at the Valgrind output. In both cases it looked like it was memory
initialized inside the X11 library.

See ya!
-Sam Lantinga, Software Engineer, Blizzard Entertainment