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
  

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?= <marko.makela@iki.fi>
> 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 &Play, bool &Forward, int &Speed)
>   // --- cDvbPlayerControl -----------------------------------------------------
>   
>   cDvbPlayerControl::cDvbPlayerControl(const char *FileName, bool PauseLive)
> -:cControl(player = new cDvbPlayer(FileName, PauseLive))
> +:cControl(new cDvbPlayer(FileName, PauseLive))
>   {
> +  player = static_cast<cDvbPlayer*>(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<cTransfer*>(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 &Current, int &Total, bool SnapToIFrame = false) const { return player ? player->GetIndex(Current, Total, SnapToIFrame) : false; }
    bool GetFrameNumber(int &Current, int &Total) const { return player ? player->GetFrameNumber(Current, Total) : false; }
--- ./transfer.c        2017/12/07 15:00:33     5.0
+++ ./transfer.c        2022/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 { 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 &Area) const { return !(x2 < Area.x1 || x1 > Area.x2 || y2 < Area.y1 || y1 > Area.y2); }
>     };
>   

If x2 ever becomes negative, something else must have gone wrong.
So I think this check here is moot.

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

If NumFilters is 0, pfd[] is never accessed.
So why allocate memory for it if it is never used?

Klaus
  

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<cDvbPlayer*>(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
  

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<cDvbPlayer*>(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.c        2022/01/31 21:21:42     5.3
+++ ./sections.c        2022/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
  

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 j<NumCamSlots evaluates to 0<NumCamSlots 
which 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
  

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=<optimized out>) at font.c:77
#8  0x0027b660 in cFreetypeFont::Glyph (this=this@entry=0xc91ea8, 
     CharCode=3220926570, CharCode@entry=3322475946, 
     AntiAliased=<optimized out>) at font.c:231
#9  0x0027c21c in cFreetypeFont::Width (s=<optimized out>, 
     this=<optimized out>) at font.c:261
#10 cFreetypeFont::Width (this=0xc91ea8, s=<optimized out>) at font.c:248
#11 0x00584754 in cSkinLCARSDisplayMenu::SetTitle (this=0xc7a1e8, 
     Title=0x5d9 <error: Cannot access memory at address 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=<optimized out>, 
     OpenSubMenus=<optimized out>) at menu.c:4489
#15 0x0007c9f0 in main (argc=<optimized out>, argv=<optimized out>)
     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 <cChannels::channels>, Number=<optimized out>)
     at device.h:148
#11 0x0007991c in main (argc=<optimized out>, argv=<optimized out>)
     at vdr.c:918

Best regards,

	Marko
  

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
  

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(&AreaOsd, 1) != oeOk) {
             dbgoutput("CanHandleAreas: %d<br>\n", osd->CanHandleAreas(&AreaOsd, 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
  

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.c    2022/01/24 16:53:45     5.5
+++ device.c    2022/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
  

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