Re: [vdr] [PATCH] Fix undefined behaviour
On 05.12.22 16:54, Marko Mäkelä wrote: If NumCamSlots is 0, SlotPriority[] is never accessed. So why allocate memory for it if it is never used? Allocating a variable-length array of length 0 is undefined behaviour. The compiler is allowed to assume NumCamSlots>0 and optimize something based on that assumption. I think this is because some compilers avoid zero-sized data structures, so that every data structure has an unique address. In this case, if NumCamSlots is 0, the address of SlotPriority would be the same as of NumUsableSlots. (or whatever follows it in case of variable-length local arrays.) The alternative would be to ensure a minimum length of 1 (as its done for empty structs), but that would add notable overhead for a corner case, so its left 'undefined'. Cheers, Udo ___ vdr mailing list vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Fix undefined behaviour
On 06.12.22 14:25, Marko Mäkelä wrote:
...
Maybe the simplest way to silence the warning would be to bloat the
variable-length array with 1 extra element, wasting sizeof(int) bytes of stack
space:
int SlotPriority[NumCamSlots + 1];
OK, so this is it:
--- device.c2022/01/24 16:53:45 5.5
+++ device.c2022/12/06 17:01:41
@@ -249,7 +249,7 @@
{
// Collect the current priorities of all CAM slots that can decrypt the
channel:
int NumCamSlots = CamSlots.Count();
- int SlotPriority[NumCamSlots];
+ int SlotPriority[NumCamSlots + 1]; // +1 to keep the compiler from doing crazy
"optimizations" if NumCamSlots==0
int NumUsableSlots = 0;
bool InternalCamNeeded = false;
if (Channel->Ca() >= CA_ENCRYPTED_MIN) {
Klaus
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Fix undefined behaviour
On 06.12.22 12:24, Marko Mäkelä wrote:
...
diff --git a/dvbsubtitle.c b/dvbsubtitle.c
index c1dfef4d..2d22d963 100644
--- a/dvbsubtitle.c
+++ b/dvbsubtitle.c
@@ -1770,6 +1770,8 @@ void cDvbSubtitleConverter::FinishPage(cDvbSubtitlePage
*Page)
return;
int NumAreas;
tArea *Areas = Page->GetAreas(NumAreas);
+ if (!Areas)
+ return;
tArea AreaCombined = Page->CombineAreas(NumAreas, Areas);
tArea AreaOsd = Page->ScaleArea(AreaCombined, osdFactorX, osdFactorY);
int Bpp = 8;
OK, let's settle for this (if there are no areas, the check for 'NumAreas > 0'
is obsolete):
--- dvbsubtitle.c 2021/03/17 15:24:34 5.1
+++ dvbsubtitle.c 2022/12/06 16:44:02
@@ -1770,11 +1770,13 @@
return;
int NumAreas;
tArea *Areas = Page->GetAreas(NumAreas);
+ if (!Areas)
+ return;
tArea AreaCombined = Page->CombineAreas(NumAreas, Areas);
tArea AreaOsd = Page->ScaleArea(AreaCombined, osdFactorX, osdFactorY);
int Bpp = 8;
bool Reduced = false;
- if (osd && NumAreas > 0) {
+ if (osd) {
while (osd->CanHandleAreas(, 1) != oeOk) {
dbgoutput("CanHandleAreas: %d\n", osd->CanHandleAreas(,
1));
int HalfBpp = Bpp / 2;
... > @@ -74,7 +74,8 @@ cGlyph::cGlyph(uint CharCode, FT_GlyphSlotRec_
*GlyphData)
rows = GlyphData->bitmap.rows;
pitch = GlyphData->bitmap.pitch;
bitmap = MALLOC(uchar, rows * pitch);
- memcpy(bitmap, GlyphData->bitmap.buffer, rows * pitch);
+ if (int bytes = rows * pitch)
+ memcpy(bitmap, GlyphData->bitmap.buffer, bytes);
}
cGlyph::~cGlyph()
OK, 'man malloc' says "If size is 0, then malloc() returns either NULL, or a
unique pointer value that can later
be successfully passed to free()". Since memcpy() must not be called with a
NULL pointer, you win.
Klaus
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Fix undefined behaviour
Tue, Dec 06, 2022 at 01:24:09PM +0200, Marko Mäkelä wrote:
The first attached patch includes your suggested fixes and nothing that
you opposed so far. The second attached patch fixes the following 2
issues. I agree that the NumCamSlots==0 case could be solved in a nicer
way.
I tried to make sense out of the generated code, and I did not find any
evidence of a subsequent unsafe optimization. Still, I think that it is
a good practice to address any compiler warnings even when they are
genuinely bogus, because often such warnings can identify real trouble.
The only question should be whether silencing the warnings could
introduce an unacceptable amount of runtime overhead.
Maybe the simplest way to silence the warning would be to bloat the
variable-length array with 1 extra element, wasting sizeof(int) bytes of
stack space:
int SlotPriority[NumCamSlots + 1];
That would avoid adding a conditional branch, like the one that would
have been part of my initially suggested std::max(NumCamSlots, 1). It
might not even translate into an extra machine instruction, if the
constant can be embedded in an instruction that would be generated
anyway.
I tested the following alternative patch. It is duplicating a lot of
source code, which I usually find a bad idea, including in this case.
Some "imp <<= 1" or "imp <<= 8" are unnecessary and could be omitted. I
retained the statements to keep the transformed code more similar to
what it was copied and adapted from.
Marko
diff --git a/device.c b/device.c
index 4b9c9cc7..1c1b4d28 100644
--- a/device.c
+++ b/device.c
@@ -249,6 +249,57 @@ cDevice *cDevice::GetDevice(const cChannel *Channel, int Priority, bool LiveView
{
// Collect the current priorities of all CAM slots that can decrypt the channel:
int NumCamSlots = CamSlots.Count();
+ if (!NumCamSlots) {
+ bool NeedsDetachReceivers = false;
+ const bool InternalCamNeeded = Channel->Ca() >= CA_ENCRYPTED_MIN;
+ cDevice *d = NULL;
+ uint32_t Impact = 0x; // we're looking for a device with the least impact
+ for (int i = 0; i < numDevices; i++) {
+ if (Channel->Ca() && Channel->Ca() <= CA_DVB_MAX && Channel->Ca() != device[i]->DeviceNumber() + 1)
+continue; // a specific card was requested, but not this one
+ const bool HasInternalCam = device[i]->HasInternalCam();
+ if (InternalCamNeeded && !HasInternalCam)
+continue; // no CAM is able to decrypt this channel and the device uses vdr handled CAMs
+ bool ndr;
+ if (device[i]->ProvidesChannel(Channel, Priority, )) { // this device is basically able to do the job
+// Put together an integer number that reflects the "impact" using
+// this device would have on the overall system. Each condition is represented
+// by one bit in the number (or several bits, if the condition is actually
+// a numeric value). The sequence in which the conditions are listed corresponds
+// to their individual severity, where the one listed first will make the most
+// difference, because it results in the most significant bit of the result.
+uint32_t imp = 0;
+imp <<= 1;
+imp <<= 1; imp |= LiveView && (!device[i]->IsPrimaryDevice() || ndr); // prefer the primary device for live viewing if we don't need to detach existing receivers
+imp <<= 1; imp |= !device[i]->Receiving() && (device[i] != cTransferControl::ReceiverDevice() || device[i]->IsPrimaryDevice()) || ndr; // use receiving devices if we don't need to detach existing receivers, but avoid primary device in local transfer mode
+imp <<= 1; imp |= device[i]->Receiving(); // avoid devices that are receiving
+imp <<= 5; imp |= GetClippedNumProvidedSystems(5, device[i]) - 1; // avoid cards which support multiple delivery systems
+imp <<= 1; imp |= device[i] == cTransferControl::ReceiverDevice(); // avoid the Transfer Mode receiver device
+imp <<= 8; imp |= device[i]->Priority() - IDLEPRIORITY; // use the device with the lowest priority (- IDLEPRIORITY to assure that values -100..99 can be used)
+imp <<= 8;
+imp <<= 1; imp |= ndr; // avoid devices if we need to detach existing receivers
+imp <<= 1; imp |= !InternalCamNeeded && device[i]->HasCi(); // avoid cards with Common Interface for FTA channels
+imp <<= 1; imp |= device[i]->AvoidRecording(); // avoid SD full featured cards
+imp <<= 1;
+imp <<= 1; imp |=
Re: [vdr] [PATCH] Fix undefined behaviour
Mon, Dec 05, 2022 at 04:08:45PM +0100, Klaus Schmidinger wrote:
Instead if typecasting I guess I'll rather do it this way:
This worked as well.
If x2 ever becomes negative, something else must have gone wrong.
The actual culprit is cDvbSubtitleConverter::FinishPage(), which was
invoking this code with NumAreas == 0 and a null pointer Areas. After I
fixed that, the error went away, and Finnish DVB subtitles were still
being displayed.
The first attached patch includes your suggested fixes and nothing that
you opposed so far. The second attached patch fixes the following 2
issues. I agree that the NumCamSlots==0 case could be solved in a nicer
way.
If (rows * pitch) is 0, nothing is copied.
On my test run, this code was not hit until the end, upon pressing the
Setup button (which was followed by Right, OK, OK, to shut down VDR):
#7 0x00278908 in cGlyph::cGlyph (this=0xc6a798, CharCode=32,
GlyphData=) at font.c:77
#8 0x0027b660 in cFreetypeFont::Glyph (this=this@entry=0xc91ea8,
CharCode=3220926570, CharCode@entry=3322475946,
AntiAliased=) at font.c:231
#9 0x0027c21c in cFreetypeFont::Width (s=,
this=) at font.c:261
#10 cFreetypeFont::Width (this=0xc91ea8, s=) at font.c:248
#11 0x00584754 in cSkinLCARSDisplayMenu::SetTitle (this=0xc7a1e8,
Title=0x5d9 )
at skinlcars.c:1559
#12 0x00407f4c in cOsdMenu::Display (this=0xc95178) at osdbase.c:244
#13 0x004137dc in cOsdMenu::AddSubMenu (this=this@entry=0xc13ef8,
SubMenu=SubMenu@entry=0xc95178) at osdbase.c:521
#14 0x00357ca0 in cMenuMain::cMenuMain (this=0xc95db8, State=,
OpenSubMenus=) at menu.c:4489
#15 0x0007c9f0 in main (argc=, argv=)
at vdr.c:1276
I guess that the bitmap for character code 32 (space) is empty. I dug a
bit further in another run to find the string in question:
#12 0x00407f2c in cOsdMenu::Display (this=0xc592c8) at osdbase.c:244
244 displayMenu->SetTitle(title);
(gdb) p *this
..., title = 0xc68400 "Asetukset - VDR 2.6.2", ...
That is the title of the Setup menu in Finnish. I cannot imagine any
other fix of this.
The font was DejaVuSans-Bold.ttf, in case it makes a difference.
If NumCamSlots is 0, SlotPriority[] is never accessed.
True, but as I noted in my other reply, the compiler is actually allowed
to assume that it will be accessed, for the first iteration of the
second loop. I did not check the generated code for any normal optimized
build to see whether my compilers would actually apply that
optimization.
#6 0x7679c26c in __ubsan_handle_vla_bound_not_positive ()
from /lib/arm-linux-gnueabihf/libubsan.so.1
#7 0x0016fb8c in cDevice::GetDevice (Channel=Channel@entry=0xbcb2a8,
Priority=Priority@entry=0, LiveView=LiveView@entry=true,
Query=Query@entry=false) at device.c:292
#8 0x0017e1e0 in cDevice::SetChannel (this=this@entry=0xbf5cf0,
Channel=Channel@entry=0xbcb2a8, LiveView=LiveView@entry=125)
at device.c:942
#9 0x0017fbb8 in cDevice::SwitchChannel (this=0xbf5cf0, Channel=0xbcb2a8,
LiveView=LiveView@entry=true) at device.c:815
#10 0x000acfb8 in cChannels::SwitchTo (
this=this@entry=0xaa9044 , Number=)
at device.h:148
#11 0x0007991c in main (argc=, argv=)
at vdr.c:918
Best regards,
Marko
diff --git a/dvbplayer.c b/dvbplayer.c
index 2ee846b6..b6620b5c 100644
--- a/dvbplayer.c
+++ b/dvbplayer.c
@@ -981,8 +981,10 @@ bool cDvbPlayer::GetReplayMode(bool , bool , int )
// --- cDvbPlayerControl -
cDvbPlayerControl::cDvbPlayerControl(const char *FileName, bool PauseLive)
-:cControl(player = new cDvbPlayer(FileName, PauseLive))
+:cControl(NULL, PauseLive)
{
+ player = new cDvbPlayer(FileName, PauseLive);
+ SetPlayer(player);
}
cDvbPlayerControl::~cDvbPlayerControl()
diff --git a/dvbsubtitle.c b/dvbsubtitle.c
index c1dfef4d..2d22d963 100644
--- a/dvbsubtitle.c
+++ b/dvbsubtitle.c
@@ -1770,6 +1770,8 @@ void cDvbSubtitleConverter::FinishPage(cDvbSubtitlePage *Page)
return;
int NumAreas;
tArea *Areas = Page->GetAreas(NumAreas);
+ if (!Areas)
+ return;
tArea AreaCombined = Page->CombineAreas(NumAreas, Areas);
tArea AreaOsd = Page->ScaleArea(AreaCombined, osdFactorX, osdFactorY);
int Bpp = 8;
diff --git a/player.h b/player.h
index 22c748bd..d4b8f127 100644
--- a/player.h
+++ b/player.h
@@ -107,6 +107,7 @@ public:
///< Deletion of the marks themselves is handled separately, calling
///< this function merely tells the player to no longer display the
///< marks, if it has any.
+ void SetPlayer(cPlayer *Player) { player = Player; }
double FramesPerSecond(void) const { return player ? player->FramesPerSecond() : DEFAULTFRAMESPERSECOND; }
bool GetIndex(int , int , bool SnapToIFrame = false) const { return player ? player->GetIndex(Current, Total, SnapToIFrame) : false; }
bool GetFrameNumber(int , int ) const { return player ? player->GetFrameNumber(Current, Total) : false; }
diff --git
Re: [vdr] [PATCH] Fix undefined behaviour
Hi Klaus,
Tue, Dec 06, 2022 at 12:05:02AM +0100, Klaus Schmidinger wrote:
In cDevice::GetDevice() SlotPriority[] is never touched if NumCamSlots
is 0. So the compiler may assume whatever it wants in that case, it
won't matter. Or can you show a case where it actually misbehaves?
Because I am not deeply familiar with the implementation of compiler
optimization passes, I can't provide such an example. I think that many
optimizations revolve around dead code elimination. In this particular
case, the compiler may assume NumCamSlots>0 because no valid program
contains undefined behaviour. A compiler could further infer that at
least one iteration of the following loop will be executed:
for (int j = 0; j < NumCamSlots || !NumUsableSlots; j++) {
That is, it could optimize away the loop condition for the first
iteration (because initially jwhich trivially holds from the compiler's point of view), transforming
it into
int j = 0;
do ... while (++j < NumCamSlots || !NumUsableSlots);
That in turn could cause an uninitialized and unallocated first element
of the empty variable-length array to be accessed in the loop.
On a quick look, it looks like a valid and feasible optimization to me,
because both NumCamSlots and NumUsableSlots will stay constant during
the loop.
I can recommend this EuroLLVM 2022 talk on a creative way of testing
compilers, revolving around dead code elimination:
https://www.youtube.com/watch?v=jD0WDB5bFPo
cDvbPlayerControl is the class that creates and deletes the player.
cControl is only given the player to control it in an abstract manner.
Since cControl does not own the cControl::player but its derived classes
do, cDvbPlayerControl could simply use cControl::player for storing the
object, which it is responsible for creating and deleting. Yes, saving a
wasted pointer might not be worth violating some design principles.
Well, IMHO whoever implemented such an "optimization" should be banned
from programming for life!
This is not an optimization, it's an insidious TRAP!
The man page on memcpy() doesn't say that the size can't be 0.
Your reaction resembles mine when I first encountered this.
First, the issue is not size=0 but the null pointers. In my patch, I
checked for nonzero size because checking if the pointers are null could
cause even more surprises or hide actual bugs.
The manual page on my system does not say anything about null pointers
either. Neither does a draft copy of the standard that I found in
https://iso-9899.info/wiki/Web_resources#Secondary_materials
But the GNU libc header /usr/include/string.h on my system declares that
the pointers must not be null:
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
size_t __n) __THROW __nonnull ((1, 2));
So, it would seem that the fact that this behaviour is undefined is
actually nonstandard. Still, I think that a piece of software should try
to obey each strange rule of the environment that it is targeting.
Sometimes, I have encountered special handling in code that invokes
malloc(), to ensure that at least 1 byte is always allocated, so that a
nonnull pointer would be returned. I do not know the motivation of that;
it has always been allowed to invoke free() or delete on a null pointer.
One more thing worth noting about memcpy() and memset() is that
compilers often optimize such calls when the size is known at
compilation time, transforming them into some plain load or store
instructions. I believe that these library functions are the only
portable way to perform unaligned loads of stores (say, reading or
writing a 32-bit or 64-bit value at an unaligned address). It is often
educational to experiment with https://godbolt.org/ which allows one to
test a large number of compiler versions and target processors.
Sorry, my bad. I missed that pfd is passed to poll()
Please try this:
Thank you, I will, and will report the results later.
Marko
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
