[mythtv] Null pointer dereference warnings when compiling with -O3

[David Hampton]

On Thu, 2020-09-24 at 11:34 -0400, David Hampton wrote:
> On Wed, 2020-09-23 at 19:58 +0200, Klaas de Waal wrote:
> > To my surprise I do then get a lot of "null pointer dereference"
> > and
> > "possible null pointer dereference" compilation warnings. This is
> > with Fedora-32 and gcc 10.2.1.
> > 
> > I have the impression that these warnings are just wrong.
> > 
> > This is for the following reasons:
> > - The code that is flagged runs already for years and years. If
> > there
> > was a null pointer dereference I think that somebody would have
> > noticed this. The word "segfault" comes to the mind.
> 
> You are misunderstanding what the compiler is saying. It is telling
> you that after inlining and optimization, based on what it can infer
> from the code, there are definitely code paths that will lead to a
> segfault.

Hi Klaus,

I spent some time looking at this yesterday, and here's what I believe
is happening. Looking at this warning:

  mpeg/atscdescriptors.h: In member function
    ‘void   AvFormatDecoder::ScanATSCCaptionStreams(int)’:
  mpeg/atscdescriptors.h:107:39: warning: potential null pointer
     dereference [-Wnull-dereference]
  107 |         { return bool(((Offset(i,-1)[3])) & 1); }
      |                       ~~~~~~~~~~~~~~~~^~~

Line atscdescriptors.h:106-7 is this:

    bool Line21Field(int i) const
        { return bool(((Offset(i,-1)[3])) & 1); }

This is getting back a "const unsigned char *" pointer from Offset,
reading the byte at offset 3, and manipulating that value.
 
Line atscdescriptors.h:125 defines Offset as this:

   const unsigned char *Offset(int i, int j) const
        { return m_ptrs[Index(i,j)]; }

The m_ptrs member variable is a QMap, and this is where the problem
occurs. According to the documentation for QMap::operator[]:

  If the map contains no item with the key, the function inserts a
  default-constructed value into the map with that key, and returns a
  reference to it.

The default constructed value for any pointer is nullptr, so the 
QMap::operator[] function has a code path that returns nullptr.  I
believe that what is happening here is that the optimizer is inlining
this default constructor into AvFormatDecoder::ScanATSCCaptionStreams,
so the compiler now sees a code path where the pointer is set to
nullptr and then the third byte off that pointer is read.

That said, given the way that this function creates, uses, and discards
the CaptionServiceDescriptor object all within a single "for" loop, I
agree with you that it not possible for this code path to be triggered.

Looking at the warning from CaptionServiceDescriptor::toString, I'm not
clear where its called from so don't know if the map is valid.

Looking at the warning from ContentAdvisoryDescriptor::Parse, it's the
two calls to RatedDimensions(i) that have potential nullptr
dereferences. I suspect that this code also can't traverse the nullptr
code path, because it looks like its creating each item Index(i,j+1)
just before the loop tries to reference it.

I'm not sure there's a clean solution here, as every call to Offset()
potentially needs to be checked to see if it returned a null pointer.

If you are convinced that these nullptr dereference code paths can
never happen, you should be able to disable the warnings in
atscdescriptors.h with the following:
 
  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wnull-dereference"
      foo(b);         /* no diagnostic for this one */
  #pragma GCC diagnostic pop

David