Hi all,
I played a little with Valgrind:
valgrind --vgdb=yes --vgdb-error=0 ./vdr ... and in gdb, (gdb) target remote |vgdb
This seems to give me a false alarm for cRecording::cRecording():
Program received signal SIGTRAP, Trace/breakpoint trap. 0x0811951a in cRecording::cRecording (this=0xbee4d958, FileName=0x0) at recording.c:801 801 if (*(fileName + strlen(fileName) - 1) == '/')
The machine code is doing some magic after the strdup() on the previous line. I suspect that it is gcc that is performing strlen() with some black magic that trips this warning:
==3212== Invalid read of size 4 ==3212== at 0x811951A: cRecording::cRecording(char const*) (recording.c:801) ==3212== by 0x80CEF77: cDvbPlayer::cDvbPlayer(char const*, bool) (dvbplayer.c:268) ==3212== Address 0x4454004 is 76 bytes inside a block of size 78 alloc'd ==3212== at 0x4028308: malloc (vg_replace_malloc.c:263) ==3212== by 0x4315E1F: strdup (strdup.c:43) ==3212== by 0x81194FE: cRecording::cRecording(char const*) (recording.c:800)
Then I got and fixed many warnings in Softdevice, which forgot to initialize some class members in constructors. Finally, I got a real warning for VDR:
==3212== Conditional jump or move depends on uninitialised value(s) ==3212== at 0x4029654: __GI_strcmp (mc_replace_strmem.c:712) ==3212== by 0x813ACD8: cSkinLCARSDisplayReplay::DrawTrack() (skinlcars.c:1786) ==3212== by 0x813AE9F: cSkinLCARSDisplayReplay::Flush() (skinlcars.c:1861) ==3212== by 0x80FCA55: cReplayControl::ShowProgress(bool) (menu.c:4670) ==3212== by 0x80FCBF7: cReplayControl::ShowTimed(int) (menu.c:4583) ==3212== by 0x80FCDEF: cReplayControl::cReplayControl(bool) (menu.c:4510) ==3212== by 0x80AC745: main (vdr.c:1307)
"(gdb) monitor get_vbits" tells me that all of lastTrackId is uninitialized:
(gdb) up #1 0x0813acd9 in cSkinLCARSDisplayReplay::DrawTrack ( this=this@entry=0x457c3a0) at skinlcars.c:1786 1786 if (!Track && *lastTrackId.description || Track && strcmp(lastTrackId.description, Track->description)) { (gdb) p lastTrackId $27 = {id = 0, language = "\000\000\000\000\000\000\000", description = '\000' <repeats 31 times>} (gdb) p &lastTrackId $28 = (tTrackId *) 0x457c434 (gdb) p sizeof lastTrackId $29 = 42 (gdb) monitor get_vbits 0x457c434 42 ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffff
BTW, !Track && x || Track && y should IMO be simpler written Track ? y : x.
It looks like a memset() is missing from the cSkinLCARSDisplayReplay constructor. cSkinLCARSDisplayChannel::cSkinLCARSDisplayChannel() is doing the right thing:
memset(&lastTrackId, 0, sizeof(lastTrackId));
Adding the memset() made this message go away. (Patch attached.)
The next problem is this one, which I get every time by pressing Play, Pause, Menu, Recordings after startup:
==3601== Conditional jump or move depends on uninitialised value(s) ==3601== at 0x810C0DB: cRect::Intersected(cRect const&) const (osd.h:411) ==3601== by 0x810E3D1: cPixmapMemory::DrawRectangle(cRect const&, unsigned int) (osd.c:1333) ==3601== by 0x810AA0B: cOsd::DrawRectangle(int, int, int, int, unsigned int) (osd.c:1922) ==3601== by 0x8130482: cSkinLCARSDisplayMenu::Clear() (skinlcars.c:1463) ==3601== by 0x810651F: cOsdMenu::Display() (osdbase.c:223) ==3601== by 0x80FA3D1: cMenuMain::Set() (menu.c:3432) ==3601== by 0x80FA9FD: cMenuMain::cMenuMain(eOSState, bool) (menu.c:3376) ==3601== by 0x80AC21B: main (vdr.c:1078)
According to "monitor get_vbits", the cRect is totally uninitialized.
Program received signal SIGTRAP, Trace/breakpoint trap. 0x0810c0db in IsEmpty (this=0xbeba08a0) at osd.h:411 411 bool IsEmpty(void) const { return Width() <= 0 || Height() <= 0; } (gdb) up #1 cRect::Intersected (this=this@entry=0xbeba08a0, Rect=...) at osd.c:912 912 if (!IsEmpty() && !Rect.IsEmpty()) { (gdb) up #2 0x0810e3d2 in cPixmapMemory::DrawRectangle (this=0x6d3fe78, Rect=..., Color=2566914048) at osd.c:1333 1333 cRect r = Rect.Intersected(DrawPort().Size());
As far as I can tell, the entirely uninitialized cRect is being passed as the Rect parameter to cPixmapMemory::DrawRectangle(). Unfortunately, gdb cannot show me the stack above that. It would seem to me that cSkinLCARSDisplayMenu::Clear() is passing uninitialized bounds to cOsd::DrawRectangle(), which will lead to funny values like this:
(gdb) p *this $31 = {point = {x = 1418239204, y = 0}, size = {width = -1379480940, height = 201}, static Null = {point = {x = 0, y = 0}, size = {width = 0, height = 0}, static Null = <same as static member of an already seen type>}}
As a workaround, I guess that I will be switching away from the LCARS skin for now. I got into this exercise because vdr sometimes crashed when I pressed the Recordings or Menu button when using the LCARS skin.
I am also attaching my patch against softdevice CVS, in case someone finds it useful. I was unable to figure out how to clear the garbage at the bottom of the OSD screen. It goes away if the dfb:mgatv video display area is high enough (4:3 video instead of 16:9).
Best regards,
Marko
On 15.11.2013 16:08, Marko Mäkelä wrote:
... BTW, !Track && x || Track && y should IMO be simpler written Track ? y : x.
It looks like a memset() is missing from the cSkinLCARSDisplayReplay constructor. cSkinLCARSDisplayChannel::cSkinLCARSDisplayChannel() is doing the right thing:
memset(&lastTrackId, 0, sizeof(lastTrackId));
Adding the memset() made this message go away. (Patch attached.)
Thanks, applied.
The next problem is this one, which I get every time by pressing Play, Pause, Menu, Recordings after startup:
==3601== Conditional jump or move depends on uninitialised value(s) ==3601== at 0x810C0DB: cRect::Intersected(cRect const&) const (osd.h:411) ==3601== by 0x810E3D1: cPixmapMemory::DrawRectangle(cRect const&, unsigned int) (osd.c:1333) ==3601== by 0x810AA0B: cOsd::DrawRectangle(int, int, int, int, unsigned int) (osd.c:1922) ==3601== by 0x8130482: cSkinLCARSDisplayMenu::Clear() (skinlcars.c:1463) ==3601== by 0x810651F: cOsdMenu::Display() (osdbase.c:223) ==3601== by 0x80FA3D1: cMenuMain::Set() (menu.c:3432) ==3601== by 0x80FA9FD: cMenuMain::cMenuMain(eOSState, bool) (menu.c:3376) ==3601== by 0x80AC21B: main (vdr.c:1078)
According to "monitor get_vbits", the cRect is totally uninitialized.
Program received signal SIGTRAP, Trace/breakpoint trap. 0x0810c0db in IsEmpty (this=0xbeba08a0) at osd.h:411 411 bool IsEmpty(void) const { return Width() <= 0 || Height() <= 0; } (gdb) up #1 cRect::Intersected (this=this@entry=0xbeba08a0, Rect=...) at osd.c:912 912 if (!IsEmpty() && !Rect.IsEmpty()) { (gdb) up #2 0x0810e3d2 in cPixmapMemory::DrawRectangle (this=0x6d3fe78, Rect=..., Color=2566914048) at osd.c:1333 1333 cRect r = Rect.Intersected(DrawPort().Size());
As far as I can tell, the entirely uninitialized cRect is being passed as the Rect parameter to cPixmapMemory::DrawRectangle(). Unfortunately, gdb cannot show me the stack above that. It would seem to me that cSkinLCARSDisplayMenu::Clear() is passing uninitialized bounds to cOsd::DrawRectangle(), which will lead to funny values like this:
(gdb) p *this $31 = {point = {x = 1418239204, y = 0}, size = {width = -1379480940, height = 201}, static Null = {point = {x = 0, y = 0}, size = {width = 0, height = 0}, static Null = <same as static member of an already seen type>}}
The constructor of cRect makes sure that all members are initialized to zero. I'm afraid I can't think of a way there could be an uninitialized cRect.
Is there a reproducible set of actions that causes this to happen?
Klaus
Hi Klaus,
On Fri, Nov 15, 2013 at 04:47:10PM +0100, Klaus Schmidinger wrote:
#2 0x0810e3d2 in cPixmapMemory::DrawRectangle (this=0x6d3fe78, Rect=..., Color=2566914048) at osd.c:1333 1333 cRect r = Rect.Intersected(DrawPort().Size());
As far as I can tell, the entirely uninitialized cRect is being passed as the Rect parameter to cPixmapMemory::DrawRectangle(). Unfortunately, gdb cannot show me the stack above that. It would seem to me that cSkinLCARSDisplayMenu::Clear() is passing uninitialized bounds to cOsd::DrawRectangle(), which will lead to funny values like this:
(gdb) p *this $31 = {point = {x = 1418239204, y = 0}, size = {width = -1379480940, height = 201}, static Null = {point = {x = 0, y = 0}, size = {width = 0, height = 0}, static Null = <same as static member of an already seen type>}}
The constructor of cRect makes sure that all members are initialized to zero. I'm afraid I can't think of a way there could be an uninitialized cRect.
Sorry, I used a bit sloppy language. cRect appears to be initialized, but with uninitialized values. You know, Valgrind does not complain when you copy uninitialized data around. It only complains when you are comparing uninitialized data or passing uninitialized data to a system call. The Valgrind V-bits are tracking which bits are uninitialized.
The cRect constructor is not at fault. I tried this twice, but both times gdb would only show me the 3 topmost stack frames, claiming that the rest of the stack is corrupted. Valgrind did show more (quoting from my previous message):
==3601== by 0x810AA0B: cOsd::DrawRectangle(int, int, int, int, unsigned int) (osd.c:1922) ==3601== by 0x8130482: cSkinLCARSDisplayMenu::Clear() (skinlcars.c:1463)
I could obviously not verify this (due to gdb claiming that the stack is corrupted), but I suspect that the parameters that are being passed are uninitialized:
void cSkinLCARSDisplayMenu::Clear(void) { textScroller.Reset(); osd->DrawRectangle(xi00, yi00, xi03 - 1, yi01 - 1, Theme.Color(clrBackground)); }
AFAICT, it is invoking this code in cOsd::DrawRectangle(): pixmaps[0]->DrawRectangle(cRect(x1, y1, x2 - x1 + 1, y2 - y1 + 1), Color); This in turn should be invoking this constructor: cRect(int X, int Y, int Width, int Height): point(X, Y), size(Width, Height) {}
cSkinLCARSDisplayMenu::cSkinLCARSDisplayMenu() is not initializing any of the members xi00, yi00, xi03, yi01.
Is there a reproducible set of actions that causes this to happen?
Yes. Hit Play (to start playing the last played recording), Pause, Menu, Recordings while using the LCARS skin. The first 2 or 3 keypresses ought to be optional. I had to stop the video playback with the 2 first keypresses, because the softdevice framerate is measured in seconds per frame when running under Valgrind :)
Best regards,
Marko
On 15.11.2013 18:17, Marko Mäkelä wrote:
Hi Klaus,
On Fri, Nov 15, 2013 at 04:47:10PM +0100, Klaus Schmidinger wrote:
#2 0x0810e3d2 in cPixmapMemory::DrawRectangle (this=0x6d3fe78, Rect=..., Color=2566914048) at osd.c:1333 1333 cRect r = Rect.Intersected(DrawPort().Size());
As far as I can tell, the entirely uninitialized cRect is being passed as the Rect parameter to cPixmapMemory::DrawRectangle(). Unfortunately, gdb cannot show me the stack above that. It would seem to me that cSkinLCARSDisplayMenu::Clear() is passing uninitialized bounds to cOsd::DrawRectangle(), which will lead to funny values like this:
(gdb) p *this $31 = {point = {x = 1418239204, y = 0}, size = {width = -1379480940, height = 201}, static Null = {point = {x = 0, y = 0}, size = {width = 0, height = 0}, static Null = <same as static member of an already seen type>}}
The constructor of cRect makes sure that all members are initialized to zero. I'm afraid I can't think of a way there could be an uninitialized cRect.
Sorry, I used a bit sloppy language. cRect appears to be initialized, but with uninitialized values. You know, Valgrind does not complain when you copy uninitialized data around. It only complains when you are comparing uninitialized data or passing uninitialized data to a system call. The Valgrind V-bits are tracking which bits are uninitialized.
The cRect constructor is not at fault. I tried this twice, but both times gdb would only show me the 3 topmost stack frames, claiming that the rest of the stack is corrupted. Valgrind did show more (quoting from my previous message):
==3601== by 0x810AA0B: cOsd::DrawRectangle(int, int, int, int, unsigned int) (osd.c:1922) ==3601== by 0x8130482: cSkinLCARSDisplayMenu::Clear() (skinlcars.c:1463)
I could obviously not verify this (due to gdb claiming that the stack is corrupted), but I suspect that the parameters that are being passed are uninitialized:
void cSkinLCARSDisplayMenu::Clear(void) { textScroller.Reset(); osd->DrawRectangle(xi00, yi00, xi03 - 1, yi01 - 1, Theme.Color(clrBackground)); }
AFAICT, it is invoking this code in cOsd::DrawRectangle(): pixmaps[0]->DrawRectangle(cRect(x1, y1, x2 - x1 + 1, y2 - y1 + 1), Color); This in turn should be invoking this constructor: cRect(int X, int Y, int Width, int Height): point(X, Y), size(Width, Height) {}
cSkinLCARSDisplayMenu::cSkinLCARSDisplayMenu() is not initializing any of the members xi00, yi00, xi03, yi01.
Is there a reproducible set of actions that causes this to happen?
Yes. Hit Play (to start playing the last played recording), Pause, Menu, Recordings while using the LCARS skin. The first 2 or 3 keypresses ought to be optional. I had to stop the video playback with the 2 first keypresses, because the softdevice framerate is measured in seconds per frame when running under Valgrind :)
I have verified that cSkinLCARSDisplayMenu::Clear() actually gets called *before* cSkinLCARSDisplayMenu::SetMenuCategory(), where the variables in question are initialized. And in that call to Clear() the values are actually totally bogus.
Thanks for debugging this! Please try whether this fixes it:
--- skinlcars.c +++ skinlcars.c @@ -900,6 +900,15 @@ ys03 = ys04 - Gap; ys05 = yb15;
+ // The item area (just to have them initialized, actual setting will be done in SetMenuCategory(): + + xi00 = 0; + xi01 = 0; + xi02 = 0; + xi03 = 1; + yi00 = 0; + yi01 = 1; + // The color buttons in submenus: xb00 = xa06; xb15 = xa07;
Klaus
Hi Klaus,
Thanks for debugging this! Please try whether this fixes it:
--- skinlcars.c +++ skinlcars.c @@ -900,6 +900,15 @@ ys03 = ys04 - Gap; ys05 = yb15;
- // The item area (just to have them initialized, actual setting will be done in SetMenuCategory():
- xi00 = 0;
- xi01 = 0;
- xi02 = 0;
- xi03 = 1;
- yi00 = 0;
- yi01 = 1;
- // The color buttons in submenus: xb00 = xa06; xb15 = xa07;
After applying this fix, the only Valgrind warnings I got from VDR were bogus-looking ones: the cRecording::cRecording (based on my reading of the disassembly, gcc's built-in version of strcmp() doing a 32-bit access that goes 2 bytes beyond the end of the string), and a few bogus-looking warnings on ioctl(). 3 of them were complaining about "invalid pointer" 0x0 or 0x1 for DirectFB calls, and the 4th one was this:
==7120== Syscall param ioctl(arg) contains uninitialised byte(s) ==7120== at 0x436AA99: ioctl (syscall-template.S:82) ==7120== by 0x80CB31A: cDvbDevice::SetPid(cDevice::cPidHandle*, int, bool) (dvbdevice.c:1394) ==7120== by 0x80C22B4: cDevice::DelPid(int, cDevice::ePidType) (device.c:544) ==7120== by 0x80C4047: cDevice::Detach(cReceiver*) (device.c:1690) ==7120== by 0x8116605: cReceiver::Detach() (receiver.c:89) ==7120== by 0x42B3E45: (below main) (libc-start.c:228)
CHECK(ioctl(Handle->handle, DMX_STOP));
#define CHECK(s) { if ((s) < 0) LOG_ERROR; } // used for 'ioctl()' calls
Given that Handle->handle is a signed integer (file handle), the Valgrind warning should be for DMX_STOP, which does not look like a pointer to me. Maybe the constant happens to coincide with the address of some memory block (not too hard with only 32 bits of address space). Or perhaps Valgrind is misinterpreting the number of arguments to ioctl().
I switched back to using LCARS, and did not experience any crashes yet after applying the patch, no matter how much I played with the Menu and Recordings buttons. So, the patch seems to work. Thank you!
Marko
-
Klaus Schmidinger -
Marko Mäkelä