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:
If NumCamSlots is 0, SlotPriority[] is never accessed. So why allocate memory for it if it is never used?
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; }
If (rows * pitch) is 0, nothing is copied. Why the extra check?
If x2 ever becomes negative, something else must have gone wrong. So I think this check here is moot.
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.
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:
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.
cDvbPlayerControl is the class that creates and deletes the player. cControl is only given the player to control it in an abstract manner.
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.
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:
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.
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
On 05.12.22 16:54, Marko Mäkelä wrote:
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
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:
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 14:25, Marko Mäkelä wrote:
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 06.12.22 12:24, Marko Mäkelä wrote:
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;
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
-
Klaus Schmidinger -
Marko Mäkelä -
Udo Richter