Re: [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 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

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