Re: [PATCH] Fix undefined behaviour

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
vdr mailing list
vdr@xxxxxxxxxxx
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr



[Index of Archives]     [Linux Media]     [Asterisk]     [DCCP]     [Netdev]     [Xorg]     [Util Linux NG]     [Xfree86]     [Big List of Linux Books]     [Fedora Users]     [Fedora Women]     [ALSA Devel]     [Linux USB]

  Powered by Linux