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
Re: [vdr] [PATCH] Fix undefined behaviour
On 05.12.22 16:54, Marko Mäkelä wrote:
Hi Klaus,
Mon, Dec 05, 2022 at 04:08:45PM +0100, Klaus Schmidinger 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.
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?
You are right, it would be better to treat NumCamSlots==0 as a special case. I only tried
adding a "return NULL", which resulted in an error message that a channel is
unavailable, for a free-to-view channel.
Doing this would never select a device for FTA channels.
...
Related question: Do we need to duplicate cControl::player in
cDvbPlayerControl::player? Perhaps there could be a member function that
returns the protected data member of the base class:
class cDvbPlayerControl {
...
private:
cDvbPlayer *GetPlayer() const { return static_cast(player); }
...
};
cDvbPlayerControl is the class that creates and deletes the player.
cControl is only given the player to control it in an abstract manner.
If (rows * pitch) is 0, nothing is copied.
Why the extra check?
Because invoking memcpy() with null pointers is undefined behaviour, the
compiler is allowed to assume that both pointers are nonnull, and allowed to
optimize subsequent checks based on that assumption. Because this member
function is inlined, the assumption could propagate to lots of other code.
Basically, for code like this:
void copy_and_set(char *a, const char *b, size_t size)
{
memcpy(a, b, size);
if (a)
*a = 1;
}
the compiler is allowed to optimize away the "if (a)" check.
Some years ago, I witnessed this in another codebase, when it was compiled with
a new enough GCC and -O2. It was quite a head-scratcher, because the memcpy()
or memset() call was located far away from the place where the surprising
optimization took place.
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.
...
If NumFilters is 0, pfd[] is never accessed.
So why allocate memory for it if it is never used?
Could "if (NumFilters == 0)" be added to skip the allocation and the subsequent code? On
a quick read of "man 2 poll", I did not find any mention on what it should do if the
array is empty, nor did I check what it would actually do: Wait for the timeout, or return
immediately?
Sorry, my bad. I missed that pfd is passed to poll()
Please try this:
--- ./sections.c2022/01/31 21:21:42 5.3
+++ ./sections.c2022/12/05 22:46:24
@@ -180,6 +180,11 @@
startFilters = false;
}
int NumFilters = filterHandles.Count();
+if (NumFilters == 0) {
+ Unlock();
+ cCondWait::SleepMs(100);
+ continue;
+ }
pollfd pfd[NumFilters];
for (cFilterHandle *fh = filterHandles.First(); fh; fh =
filterHandles.Next(fh)) {
int i = fh->Index();
Klaus
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Fix undefined behaviour
Hi Klaus,
Mon, Dec 05, 2022 at 04:08:45PM +0100, Klaus Schmidinger 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.
You are right, it would be better to treat NumCamSlots==0 as a special
case. I only tried adding a "return NULL", which resulted in an error
message that a channel is unavailable, for a free-to-view channel.
Instead if typecasting I guess I'll rather do it this way:
--- ./dvbplayer.c 2022/01/13 21:41:41 5.1
+++ ./dvbplayer.c 2022/12/05 14:29:50
@@ -981,8 +981,10 @@
// --- cDvbPlayerControl -
cDvbPlayerControl::cDvbPlayerControl(const char *FileName, bool PauseLive)
-:cControl(player = new cDvbPlayer(FileName, PauseLive))
+:cControl(NULL, PauseLive)
{
+ player = new cDvbPlayer(FileName, PauseLive);
+ SetPlayer(player);
}
Sure, that would work too. In my C++ projects, I try to declare as many
data members as possible as const, and initialize them at object
creation time. As far as I understand, static_cast is completely clean
here.
Related question: Do we need to duplicate cControl::player in
cDvbPlayerControl::player? Perhaps there could be a member function that
returns the protected data member of the base class:
class cDvbPlayerControl {
...
private:
cDvbPlayer *GetPlayer() const { return static_cast(player); }
...
};
If (rows * pitch) is 0, nothing is copied.
Why the extra check?
Because invoking memcpy() with null pointers is undefined behaviour, the
compiler is allowed to assume that both pointers are nonnull, and
allowed to optimize subsequent checks based on that assumption. Because
this member function is inlined, the assumption could propagate to lots
of other code.
Basically, for code like this:
void copy_and_set(char *a, const char *b, size_t size)
{
memcpy(a, b, size);
if (a)
*a = 1;
}
the compiler is allowed to optimize away the "if (a)" check.
Some years ago, I witnessed this in another codebase, when it was
compiled with a new enough GCC and -O2. It was quite a head-scratcher,
because the memcpy() or memset() call was located far away from the
place where the surprising optimization took place.
If x2 ever becomes negative, something else must have gone wrong. So I
think this check here is moot.
I tend to agree. I have seen examples of that in an interpreter. It
would first invoke some undefined-behaviour arithmetics, and then the
compiler would optimize away an overflow check that was made later.
I will try to provide a stack trace for this, to see where the garbage
was coming from, so that this bug can be fixed closer to its source.
If NumFilters is 0, pfd[] is never accessed.
So why allocate memory for it if it is never used?
Could "if (NumFilters == 0)" be added to skip the allocation and the
subsequent code? On a quick read of "man 2 poll", I did not find any
mention on what it should do if the array is empty, nor did I check what
it would actually do: Wait for the timeout, or return immediately?
Marko
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Fix undefined behaviour
On 04.12.22 13:19, Marko Mäkelä wrote:
...
0001-Fix-GCC-8.3.0-fsanitize-undefined.patch
From b69ff7105d4bb8d933f0214f34b103fda8e8b155 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?=
Date: Sun, 4 Dec 2022 13:42:57 +0200
Subject: [PATCH] Fix GCC 8.3.0 -fsanitize=undefined
...
device.c:251:31: runtime error: variable length array bound evaluates to
non-positive value 0
...
diff --git a/device.c b/device.c
index 4e987389..a770aa90 100644
--- a/device.c
+++ b/device.c
@@ -248,7 +248,7 @@ 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();
- int SlotPriority[NumCamSlots];
+ int SlotPriority[std::max(NumCamSlots, 1)];
int NumUsableSlots = 0;
bool InternalCamNeeded = false;
if (Channel->Ca() >= CA_ENCRYPTED_MIN) {
If NumCamSlots is 0, SlotPriority[] is never accessed.
So why allocate memory for it if it is never used?
dvbplayer.c:984:11: runtime error: member access within address 0x02a388d0
which does not point to an object of type 'cDvbPlayerControl'
...
diff --git a/dvbplayer.c b/dvbplayer.c
index 2ee846b6..72bc46ad 100644
--- a/dvbplayer.c
+++ b/dvbplayer.c
@@ -981,8 +981,9 @@ bool cDvbPlayer::GetReplayMode(bool , bool , int
)
// --- cDvbPlayerControl -
cDvbPlayerControl::cDvbPlayerControl(const char *FileName, bool PauseLive)
-:cControl(player = new cDvbPlayer(FileName, PauseLive))
+:cControl(new cDvbPlayer(FileName, PauseLive))
{
+ player = static_cast(cControl::player);
}
cDvbPlayerControl::~cDvbPlayerControl()
...
transfer.c:71:11: runtime error: member access within address 0x020f0428 which
does not point to an object of type 'cTransferControl'
diff --git a/transfer.c b/transfer.c
index 88931e58..b888910a 100644
--- a/transfer.c
+++ b/transfer.c
@@ -68,8 +68,9 @@ void cTransfer::Receive(const uchar *Data, int Length)
cDevice *cTransferControl::receiverDevice = NULL;
cTransferControl::cTransferControl(cDevice *ReceiverDevice, const cChannel *Channel)
-:cControl(transfer = new cTransfer(Channel), true)
+:cControl(new cTransfer(Channel), true)
{
+ transfer = static_cast(player);
ReceiverDevice->AttachReceiver(transfer);
receiverDevice = ReceiverDevice;
}
Instead if typecasting I guess I'll rather do it this way:
--- ./dvbplayer.c 2022/01/13 21:41:41 5.1
+++ ./dvbplayer.c 2022/12/05 14:29:50
@@ -981,8 +981,10 @@
// --- 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()
--- ./player.h 2020/05/18 16:47:29 5.0
+++ ./player.h 2022/12/05 14:30:24
@@ -107,6 +107,7 @@
///< 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; }
--- ./transfer.c2017/12/07 15:00:33 5.0
+++ ./transfer.c2022/12/05 14:36:39
@@ -68,8 +68,10 @@
cDevice *cTransferControl::receiverDevice = NULL;
cTransferControl::cTransferControl(cDevice *ReceiverDevice, const cChannel
*Channel)
-:cControl(transfer = new cTransfer(Channel), true)
+:cControl(NULL, true)
{
+ transfer = new cTransfer(Channel);
+ SetPlayer(transfer);
ReceiverDevice->AttachReceiver(transfer);
receiverDevice = ReceiverDevice;
}
diff --git a/font.c b/font.c
index 8b37798c..c78b1a15 100644
--- a/font.c
+++ b/font.c
@@ -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()
If (rows * pitch) is 0, nothing is copied.
Why the extra check?
osd.h:301:37: runtime error: signed integer overflow: -2147483647 - 2147483647
cannot be represented in type 'int'
...
diff --git a/osd.h b/osd.h
index 77722662..7a293321 100644
--- a/osd.h
+++ b/osd.h
@@ -298,8 +298,8 @@ public:
struct tArea {
int x1, y1, x2, y2;
int bpp;
- int Width(void) const { return x2 - x1 + 1; }
- int Height(void) const
[vdr] [PATCH] Fix undefined behaviour
Another day, another sanitizer.
After fixing issues reported by -fsanitize=address yesterday, I gave
-fsanitize=undefined a try. The GCC documentation points to the clang
documentation:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
The issues related to cControl::player were tricky. In the end, I
figured it out after setting UBSAN_OPTIONS=print_stacktrace=1 and
setting a breakpoint on _Unwind_Backtrace(). The name of the reporting
function in my system was __ubsan_handle_dynamic_type_cache_miss(),
nothing about "vptr". Also, the diagnostics was misleadingly pointing to
the body of the constructor, and not the initializer list where a data
member was being assigned to before the base class had been initialized.
The -fsanitize=undefined in clang might report more things.
Next I may give -fsanitize=thread a try.
GCC does not implement -fsanitize=memory (checking for the use of
uninitialized memory) at all. It will require clang and libc++ (not
libstdc++) and that all libraries except libc are built with
-fsanitize=memory. If you are familiar with Valgrind's default memcheck
tool, it is roughly comparable to the combination of -fsanitize=address
and -fsanitize=memory.
Marko
>From b69ff7105d4bb8d933f0214f34b103fda8e8b155 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?=
Date: Sun, 4 Dec 2022 13:42:57 +0200
Subject: [PATCH] Fix GCC 8.3.0 -fsanitize=undefined
sections.c:183:30: runtime error: variable length array bound evaluates to non-positive value 0
device.c:251:31: runtime error: variable length array bound evaluates to non-positive value 0
osd.h:301:37: runtime error: signed integer overflow: -2147483648 - 2147483647 cannot be represented in type 'int'
osd.h:301:37: runtime error: signed integer overflow: -2147483647 - 2147483647 cannot be represented in type 'int'
transfer.c:71:11: runtime error: member access within address 0x020f0428 which does not point to an object of type 'cTransferControl'
dvbplayer.c:984:11: runtime error: member access within address 0x02a388d0 which does not point to an object of type 'cDvbPlayerControl'
---
device.c| 2 +-
dvbplayer.c | 3 ++-
font.c | 3 ++-
osd.h | 4 ++--
sections.c | 2 +-
transfer.c | 3 ++-
6 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/device.c b/device.c
index 4e987389..a770aa90 100644
--- a/device.c
+++ b/device.c
@@ -248,7 +248,7 @@ 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();
- int SlotPriority[NumCamSlots];
+ int SlotPriority[std::max(NumCamSlots, 1)];
int NumUsableSlots = 0;
bool InternalCamNeeded = false;
if (Channel->Ca() >= CA_ENCRYPTED_MIN) {
diff --git a/dvbplayer.c b/dvbplayer.c
index 2ee846b6..72bc46ad 100644
--- a/dvbplayer.c
+++ b/dvbplayer.c
@@ -981,8 +981,9 @@ bool cDvbPlayer::GetReplayMode(bool , bool , int )
// --- cDvbPlayerControl -
cDvbPlayerControl::cDvbPlayerControl(const char *FileName, bool PauseLive)
-:cControl(player = new cDvbPlayer(FileName, PauseLive))
+:cControl(new cDvbPlayer(FileName, PauseLive))
{
+ player = static_cast(cControl::player);
}
cDvbPlayerControl::~cDvbPlayerControl()
diff --git a/font.c b/font.c
index 8b37798c..c78b1a15 100644
--- a/font.c
+++ b/font.c
@@ -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()
diff --git a/osd.h b/osd.h
index 77722662..7a293321 100644
--- a/osd.h
+++ b/osd.h
@@ -298,8 +298,8 @@ public:
struct tArea {
int x1, y1, x2, y2;
int bpp;
- int Width(void) const { return x2 - x1 + 1; }
- int Height(void) const { return y2 - y1 + 1; }
+ int Width(void) const { return x2 < 0 ? 0 : x2 - x1 + 1; }
+ int Height(void) const { return y2 < 0 ? 0 : y2 - y1 + 1; }
bool Intersects(const tArea ) const { return !(x2 < Area.x1 || x1 > Area.x2 || y2 < Area.y1 || y1 > Area.y2); }
};
diff --git a/sections.c b/sections.c
index 51a2823c..4d90b19c 100644
--- a/sections.c
+++ b/sections.c
@@ -180,7 +180,7 @@ void cSectionHandler::Action(void)
startFilters = false;
}
int NumFilters = filterHandles.Count();
-pollfd pfd[NumFilters];
+pollfd pfd[std::max(NumFilters, 1)];
for (cFilterHandle *fh = filterHandles.First(); fh; fh = filterHandles.Next(fh)) {
int i = fh->Index();
pfd[i].fd = fh->handle;
diff --git a/transfer.c b/transfer.c
index 88931e58..b888910a 100644
--- a/transfer.c
+++ b/transfer.c
@@ -68,8 +68,9 @@ void cTransfer::Receive(const uchar *Data, int Length)
cDevice
