[vlc-devel] [PATCH] pva fixes

Lyndon Brown jnqnfe at gmail.com
Thu Sep 24 22:54:01 CEST 2020


On Thu, 2020-09-24 at 21:33 +0300, Rémi Denis-Courmont wrote:
> Le perjantaina 18. syyskuuta 2020, 0.00.31 EEST Lyndon Brown a écrit
> :
> > a couple of fixes for pva:
> >  - patch #1 fixes an uninit read warning
> >  - patch #2 prevents uninit buffer reads
> 
> I agree that there's a bug but it's not fixed by patch 1.
> 
> And patch 2 looks like a kludge.

Patch #1 fixes the warning output for me, which was the aim of it,
though of course it only partially addresses the underlying buggy
uninitialised read problem. Indeed "kludge" may be a perfectly fair
description for the second.

It may be helpful if I try to briefly explain as best as I can remember
how I came up with those two changes, to help you evaluate things.

Firstly, so this is the block of warnings I see:
```
WARNING : demux/pva.c:414: 44:  'hdr[3]' may be used uninitialized in this function [-Wmaybe-uninitialized]
  414 |                   hdr[0], hdr[1],hdr[2],hdr[3] );
      |                                            ^
WARNING : demux/pva.c:411: 12:  'hdr[0]' may be used uninitialized in this function [-Wmaybe-uninitialized]
  411 |     if( hdr[0] != 0 || hdr[1] != 0 || hdr[2] != 1 )
      |         ~~~^~~
WARNING : demux/pva.c:414: 30:  'hdr[1]' may be used uninitialized in this function [-Wmaybe-uninitialized]
  414 |                   hdr[0], hdr[1],hdr[2],hdr[3] );
      |                              ^
WARNING : demux/pva.c:414: 37:  'hdr[2]' may be used uninitialized in this function [-Wmaybe-uninitialized]
  414 |                   hdr[0], hdr[1],hdr[2],hdr[3] );
      |                                     ^
WARNING : demux/pva.c:414: 44:  'hdr[3]' may be used uninitialized in this function [-Wmaybe-uninitialized]
  414 |                   hdr[0], hdr[1],hdr[2],hdr[3] );
      |                                            ^
WARNING : demux/pva.c:414: 44:  'hdr[3]' may be used uninitialized in this function [-Wmaybe-uninitialized]
```

Examining the function I confirmed the possibility that the bytes could
could be uninitialised.

Searching the codebase for something, I discovered that the
ParsePESDataChain() function of the 'ts' demuxer was doing a similar
thing to pva, also parsing PES data. In that case it was capturing the
output of block_ChainExtract() and checking that at least 4 bytes had
been read, prior to examining the initial bytes of the header. This
seemed to be exactly what pva should be doing also, so I copied that
across and as expected it fixed those warnings, meeting the objective
of the first patch.

I examined the rest of the function out of concern for whether there
was further possibility of uninitialised reads of latter portions of
the buffer. I compared with the ParsePESHeader() helper used by 'ts'
and wondered whether it might be a good idea to change pva to use that,
but pva was doing far less than the helper, and I don't have the
knowledge to evaluate whether such a change to pva would really be a
good thing.

While the ParsePESHeader() helper does a thorough job of checking
length before reading bytes from the buffer, the pva stuff was not.
Initialising the buffer to all zeros was a simple fix to ensuring that
at least further reads were operating on zero values, and I think it
was because the only extra reads are for a skip value and two
timestamps, that I felt pragmatically this was probably sufficient, but
if not it would no doubt be raised in review.

If you feel that buffer length validation checks are worth having in
the rest of the pva code, like in the helper and patch #1, as well as
or instead of the zero initialisation change, or alternatively a switch
to use the helper would be best, I can make such additional change, or
perhaps it would be best to leave that to yourself with your greater
experience and knowledge in this type of area? I happy either way.

Regards,
Lyndon



More information about the vlc-devel mailing list