Re: [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?= <[email protected]>
> 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


_______________________________________________
vdr mailing list
[email protected]
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr